osfs: mmap, use AddCleanup instead of finalizer and add test#218
osfs: mmap, use AddCleanup instead of finalizer and add test#218jfontan wants to merge 3 commits into
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.
| // 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. |
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.