I like the idea!

I'm not sure how many users are using single subsystem. If we agree with this change, we should add comment about Stat function with it is not thread-safety as warning.

cc @AkihiroSuda

It'd be any of the methods that call readUint not just Stat, but yea either a warning or some way to avoid this #275 (comment)

I wanted to get try this.
See: #277

Is this threadsafe? If you called any of the methods that now invoke this in a couple goroutines, there's no guarantee we'll get back the value we're expecting as another read might finish before, you may end up with interleaved data as shown below, or another thread might hit the buf.Reset before another gets to calling buf.String() and you'd end up trying to parse an empty string.

func readUintHelper(path string, expected uint64, buf *bytes.Buffer) error {
	actual, err := readUint(path, buf)
	if err != nil {
		return err
	}
	if expected != actual {
		return fmt.Errorf("expected %d, got %d", expected, actual)
	}
	return nil
}

func TestReadUint(t *testing.T) {
	var (
		buf = bytes.NewBuffer(nil)
		eg  errgroup.Group
	)
	eg.Go(func() error {
		return readUintHelper("testdata/uint100", 100, buf)
	})
	eg.Go(func() error {
		return readUintHelper("testdata/uint200", 200, buf)
	})
	eg.Go(func() error {
		return readUintHelper("testdata/uint300", 300, buf)
	})

	if err := eg.Wait(); err != nil {
		t.Fatal(err)
	}
}

Yields:

dcantah@dcantah ~/go/src/github.com/containerd/cgroups ((HEAD detached at brancz/readuint)) $ go test ./cgroup1/ -v -run TestReadUint
=== RUN   TestReadUint
    utils_test.go:66: expected 100, got 300100
--- FAIL: TestReadUint (0.00s)
FAIL
FAIL    github.com/containerd/cgroups/v3/cgroup1        0.003s
FAIL
dcantah@dcantah ~/go/src/github.com/containerd/cgroups ((HEAD detached at brancz/readuint)) $ go test ./cgroup1/ -v -run TestReadUint
=== RUN   TestReadUint
    utils_test.go:66: expected 100, got 200100
--- FAIL: TestReadUint (0.00s)
FAIL
FAIL    github.com/containerd/cgroups/v3/cgroup1        0.002s
FAIL
dcantah@dcantah ~/go/src/github.com/containerd/cgroups ((HEAD detached at brancz/readuint)) $ go test ./cgroup1/ -v -run TestReadUint
=== RUN   TestReadUint
    utils_test.go:66: strconv.ParseUint: parsing "": invalid syntax
--- FAIL: TestReadUint (0.00s)
FAIL
FAIL    github.com/containerd/cgroups/v3/cgroup1        0.002s
FAIL

Oh nice, I forgot the cg1 code has a lock around most of these for the main *cgroup operations. Let me see if there's any that the above might be a concern

At the very least if you created individual controllers (e.g. cgroup1.NewMemory() -> mem.Stat()) you could run into this

Yeah I was briefly contemplating using a sync.Pool, but the way I understood the flow of the code there is no concurrency in the controllers. It’s entirely possible I missed the code path that does, so if that’s the case I’d happily change it to a pool to make it concurrency safe.

In the controllers themselves there's not, but the library doesn't dissuade you from doing this yourself. How containerd uses this library is mainly through manipulating the main Cgroup interface from New or Load (https://github.com/containerd/cgroups/blob/main/cgroup1/cgroup.go#L37), and Stat and a bunch of others for this object lock as they collect the data from each controller/subsystem, so containerd's usage may be fine.

The library offers you the ability to Create individual controllers however with New{Controller} functions (NewMemory for example). These don't have any locking around them, so if anyone using this library relied on making an individual controller and then spinning up some goroutines to grab/update data, they may run into this now.

You’re absolutely right. How do you propose to move forward? I would say we can either analyze usages of the library and if there are no current concurrent usages we add a code comment saying they are not concurrency safe, or we go straight to the hammer and use a buffer pool?

I generally prefer to not add synchronization if I don’t absolutely have to, but ultimately your call as a maintainer.

You wanna benchmark the buffer pool idea? It'd be a shame if this caused us to not be able to freely call these however we want, so anything to keep that alive would be nice. Thanks for the quick follow ups! This is awesome to see btw, very cool stream idea

I think the problem before was that os.ReadFile returns a byte slice which forces an allocation because it escapes the stack.

You can make this much simpler and still thread safe by creating a slice with make([]byte, 64) and f.Read to read the data. Since that slice isn't returned from the function, it stays on the stack.

Oh fantastic point duh, can confirm it shouldn't escape:

package main

import (
	"log"
	"os"
)

func readFile() error {
	f, err := os.Open("howdy.txt")
	if err != nil {
		return err
	}
	defer f.Close()

	howdy := make([]byte, 64)
	_, err = f.Read(howdy)
	if err != nil {
		return err
	}
	return nil
}

func main() {
	if err := readFile(); err != nil {
		log.Fatal(err)
	}
}

alloc go build -gcflags '-m -l'
# github.com/dcantah/fun/alloc
./main.go:15:15: make([]byte, 64) does not escape
./main.go:25:12: ... argument does not escape

Every so often we must be reminded of why io.Reader's signature is the way it is 😆

@brancz Want to take this for a spin?