osfs: mmap, use AddCleanup instead of finalizer and add test#218
Conversation
Signed-off-by: Javi Fontan <jfontan@gmail.com>
From go 1.24 this seems the preferred method to clean after GC. The example from the go blog post is also about mmap files. https://go.dev/blog/cleanups-and-weak Signed-off-by: Javi Fontan <jfontan@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR updates mmap cleanup in osfs to use Go’s runtime.AddCleanup instead of finalizers, and adds a Linux-specific test to verify abandoned mmap handles are unmapped after GC.
Changes:
- Adds a
runtime.Cleanuphandle tommapFileand stops it during explicitClose. - Registers mmap cleanup via
runtime.AddCleanup. - Adds a Linux
/proc/<pid>/maps-based cleanup test.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
osfs/mmap_file.go |
Replaces finalizer-based mmap cleanup with runtime.AddCleanup. |
osfs/mmap_file_cleanup_test.go |
Adds Linux coverage for GC-triggered mmap cleanup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // will munmap and close the fd when m becomes unreachable. Close | ||
| // clears this finalizer on the orderly path. | ||
| runtime.SetFinalizer(m, (*mmapFile).Close) | ||
| m.cleanup = runtime.AddCleanup(m, mmapClean, data) |
There was a problem hiding this comment.
I don't understand that from the documentation:
func AddCleanup[T, S any](ptr *T, cleanup func(S), arg S) Cleanup
If ptr is reachable from cleanup or arg, ptr will never be collected and the cleanup will never run. As a protection against simple cases of this, AddCleanup panics if arg is equal to ptr.
There was a problem hiding this comment.
It is a variation of the case right? m holds data and data is passed as argument.
There was a problem hiding this comment.
ptr is not reachable from cleanup or arg. In other words m is not reachable from mmapClean or data. GC must be able to collect m without mmapClean or data referencing it. The clean function with data is called after GC is done.
You can collect the struct *m even if it points to other objects, these will be collected later.
There was a problem hiding this comment.
Added a comment to this effect. The issue is that mmapClean had a direct ref to m's fields. Any ref to a field of an object makes the object itself reachable, and therefore it won't be garbage collected.
| // Belt and braces for callers that forget to Close: the runtime | ||
| // will munmap and close the fd when m becomes unreachable. Close | ||
| // clears this finalizer on the orderly path. |
There was a problem hiding this comment.
fd will be closed by the os.File finalizer but it's true that the comment should be changed or make the cleanup function close the file explicitly. I'll take a look.
| runtime.GC() | ||
|
|
||
| ok, err = isFileMapped(path) | ||
| require.NoError(t, err) | ||
| require.False(t, ok) |
jfontan
force-pushed
the
mmap-addcleanup
branch
2 times, most recently
from
2780a1b to
b0664c8
Compare
Signed-off-by: Javi Fontan <jfontan@gmail.com>
|
Added a new commit with fixes for copilot reviews (but messed up with two different branches in two computers). If needed I can squash the changes in the original two commits or one. |
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestRootOSMmapCleanup(t *testing.T) { |
There was a problem hiding this comment.
Not sure if it is worth creating a test for this, as this is likely going to become intermittent. As per upstream docs:
The cleanup(arg) call is not always guaranteed to run; in particular it is not guaranteed to run before program exit.
...
Note that because cleanups may execute arbitrarily far into the future after an object is no longer referenced
There was a problem hiding this comment.
I've followed the hit from copilot and the test does 10 cycles of GC before failing. If it becomes flaky we can delete it but I believe is good to have a test that checks that we set up the cleanup function.
Maybe we can add a comment stating this.
Co-authored-by: Paulo Gomes <paulo.gomes.uk@gmail.com> Signed-off-by: Javi Fontan <jfontan@gmail.com>
Since go 1.24 it seems that the preferred way to cleanup after GC is
runtime.AddCleanup. The change does basically the same. It's even quite the same case as the example in the go blog introducing this function: https://go.dev/blog/cleanups-and-weakI've also added a test for Linux that checks that the finalizer/cleanup works (works with the two versions) so at least we are sure we have not misplaced the finalizer. The test is in a separate commit in case you are not interested in the
AddCleanupchange.This odd dance with variables was done to not trigger the linter as after cleaning
fwe don't do anything with it.I've found that cleaning the variable is not needed as the compiler is smart enough to know that
fis not used anymore afterf.Name()andruntime.GCtriggers the finalizer. I just wanted to be explicit.