-
Notifications
You must be signed in to change notification settings - Fork 768
Description
It's very easy to accidentally leak objects when using WalkDir, and kind of awkward to do the right thing (because obj can be nil?) This is why https://pkg.go.dev/github.com/cilium/ebpf/btf#HandleIterator.Take exists for handles. It might be good to do another pass over this API.
Originally posted by @lmb in #1651 (comment)
More context from Mahe:
Mmmh I'm slightly confused now, this it the typical use we have in Tetragon:
https://github.com/cilium/tetragon/blob/d72f98aefb8e88c06caec7cfbc687bbeabf97a5f/cmd/tetra/debug/progs.go#L408-L445
https://github.com/cilium/tetragon/blob/d72f98aefb8e88c06caec7cfbc687bbeabf97a5f/pkg/bugtool/maps.go#L148-L176Maybe having an example in the doc would make thing more clear?
As far as I can tell both of these examples leak objects. Not a big deal in these cases since the processes are short lived (I think?) but an indication that the API is bug prone. My gut feeling is that this is because of wrapping WalkDir
which in itself is a really clunky API.
Ideas:
- Drop Walk outright and have callers do the call to Load themselves. That makes it clearer that they are reponsible for cleaning up.
- Only invoke Walk for valid objects so that an unconditional
defer obj.Close()
at the top of the function works.
Since 1.24 is around the corner it might also be interesting to consider what it would look like to turn this API into an iterator.