Convert uses of runtime.SetFinalizer to runtime.AddCleanup #1851
Conversation
Switching to runtime.AddCleanup means that cleanups may be batched and may run concurrently. Since both the perf event and the tracefs event will have a cleanup attached it's not possible to guarantee an ordering. Drop the code which tries to clean up in order when the object is leaked without calling Close(). Signed-off-by: Lorenz Bauer <[email protected]>
It's not possible to call Reader.Close from a Cleanup, which indicates that it's probably not the right thing to do here. Drop the finalizer. Signed-off-by: Lorenz Bauer <[email protected]>
internal/epoll/poller.go
Outdated
| @@ -75,7 +76,10 @@ func New() (_ *Poller, err error) { | |||
| return nil, fmt.Errorf("add flush eventfd: %w", err) | |||
| } | |||
|
|
|||
| runtime.SetFinalizer(p, (*Poller).Close) | |||
| p.cleanup = runtime.AddCleanup(p, func(epollFd int) { | |||
| _ = unix.Close(epollFd) | |||
There was a problem hiding this comment.
Wouldn't this omit a bunch of other behaviours? Poller.Close() wakes wait, takes out two mutexes and closes two channels. It looks like we need to take a different approach here. I also liked the fact that the finalizer and Close() were the same thing. By passing ad-hoc closures to AddCleanup, we're inviting bugs due to updating one but not the other.
One argument in its favor, though: technically, the cleanup only runs if *Poller is unreachable, meaning there can't be any more callers to Wait() or Add(), so the synchronization behaviour should no longer be needed at that point. We just need to make sure to never return p.closeEvent or p.flushEvent to the caller. We'll also rely on the cleanups of the eventFd's underlying os.File, since we're no longer calling eventFd.close() explicitly when the Poller's cleanup runs.
@brycekahle Curious to hear your take on this.
ti-mo
changed the title
ti-mo
force-pushed
the
add-cleanup
branch
3 times, most recently
from
2185da8 to
54da0a0
Compare
Convert all uses of runtime.SetFinalizer to runtime.AddCleanup. The exception is the unsafe memory code, which cannot currently be converted due to fundamental differences with finalizers. Cleanups cannot carry references to the object in question, and cannot resurrect the object, meaning the original object's allocation can be used to service new allocations before the cleanup has run. In our case, this means bpf map memory gets zeroed by the allocator, which is obviously a show stopper. If we want to move off finalizers, we need to implement golang/go#70224 first and switch to that instead. Signed-off-by: Timo Beckers <[email protected]> Suggested-by: Lorenz Bauer <[email protected]>
ti-mo
requested review from
dylandreimerink,
florianl,
mmat11 and
a team
as code owners
No description provided.