x/sharedfile: promote public, add Ref and Retire#2239
Open
pjbgf wants to merge 4 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Promotes the shared, ref-counted file-handle primitive into the public x/sharedfile package, adds new lifecycle primitives (Ref, Retire) to support safe concurrent mmap-style usage, and updates x/fdpool eviction behavior to avoid force-evicting pinned members.
Changes:
- Promote
SharedFileintox/sharedfileand update internal import sites accordingly. - Add
SharedFile.Ref()/Reffor durable, single-owner pinning across an operation lifetime. - Add
SharedFile.Retire()for graceful “seal + deferred close” teardown and adjustfdpoolto over-commit when all candidates are pinned.
Reviewed changes
Copilot reviewed 12 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| x/sharedfile/sharedfile.go | Adds sealed state and implements graceful Retire() semantics; updates Acquire/Release behavior accordingly. |
| x/sharedfile/sharedfile_test.go | Adds comprehensive tests for SharedFile core behaviors (grace period, concurrency, ReleaseNow). |
| x/sharedfile/sharedfile_pool_test.go | Adds tests covering pool wiring (LRU touch, eviction, concurrent read/eviction stress). |
| x/sharedfile/sharedfile_bench_test.go | Adds benchmarks to track Acquire/Release overhead (with/without pool, last-drop path). |
| x/sharedfile/retire_test.go | Adds tests validating Retire() sealing + deferred-close behavior. |
| x/sharedfile/ref.go | Introduces the durable Ref type and SharedFile.Ref() constructor. |
| x/sharedfile/ref_test.go | Adds tests ensuring Ref pins through grace period and is idempotent on close. |
| x/sharedfile/doc.go | Adds package documentation for x/sharedfile. |
| x/fdpool/pool.go | Changes eviction policy to over-commit instead of force-evicting when all members are pinned; adds OverCommits stat. |
| x/fdpool/pool_test.go | Updates/extends tests to cover over-commit behavior when all members are pinned. |
| plumbing/format/idxfile/lazy_index.go | Switches import from internal/sharedfile to x/sharedfile. |
| internal/packhandle/testutil_test.go | Switches import from internal/sharedfile to x/sharedfile. |
| internal/packhandle/source.go | Switches import from internal/sharedfile to x/sharedfile. |
| internal/packhandle/packhandle.go | Switches import from internal/sharedfile to x/sharedfile. |
| internal/packhandle/cursor_reader.go | Switches import from internal/sharedfile to x/sharedfile. |
| internal/packhandle/cursor_reader_test.go | Switches import from internal/sharedfile to x/sharedfile. |
Assisted-by: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io>
Assisted-by: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io>
Assisted-by: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io>
When every eviction candidate is pinned, retain all members over capacity and increment Stats.OverCommits rather than calling ReleaseNow on a pinned Member. A pinned Member's ReleaseNow only latches (never closes while in use), so force-eviction drops honest accounting and defeats cross-request fd reuse without freeing anything. The LRU self-corrects as pins release on subsequent Touch calls. Replace TestPool_AllPinned_FallsBackToLRU (old force-evict behaviour) with TestPool_AllPinned_OverCommits and add TestPoolOverCommitsWhenAllPinned to cover the self-correction path (unpinned after over-commit is evicted on the next Touch). Assisted-by: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Paulo Gomes <paulo@entire.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Makes the reference-counted file-handle primitives usable by external
consumers and extends them so a caller can safely share a memory-mapped
file across concurrent readers and tear it down without racing in-flight
reads.
SharedFilealready ref-counts a lazily-opened file with grace-period /pool-driven close, but it was
internaland offered no durablereference or graceful teardown. A consumer that memory-maps large
read-only files (e.g. packfiles) and shares them across concurrent
operations needs to (a) hold a mapping open for an operation's lifetime,
(b) bound open fds/mmaps across many files, and (c) retire a file that
has been superseded without munmapping it out from under a reader that is
still using it. These changes provide exactly those, without altering the
existing
SharedFile/fdpoolcontracts.Four commits:
x/sharedfile: promote frominternal. Moves the package tox/so it can be imported outside the module. Pure relocation —internal/packhandleandplumbing/format/idxfilejust switch importpaths; no behavior change.
x/sharedfile: add durableRef. A single-owner, idempotenthandle over
Acquire/Release. While aRef(or cursor) is live theunderlying fd/mapping cannot be torn down by the grace timer,
ReleaseNow,Retire, or pool eviction. Lets a caller capture a fileonce and read through it for the capture's whole lifetime instead of
re-acquiring per read.
x/sharedfile: add gracefulRetire. A teardown that seals thehandle against new
Acquire/reopen and closes it — immediately if noreferences are held, otherwise deferred to the last
Releasesoin-flight readers ride to completion. It uses a
sealedstatedistinct from
closed, soIsClosed()stays accurate for readersmid-flight. Distinct from
Close(hard, closes under refs) andReleaseNow(permits reopen on nextAcquire). This is theforget-on-publish primitive: retire a superseded file, let current
readers finish, and the fd/mapping drops when the last one releases.
x/fdpool: over-commit instead of force-evicting live pins. Whenevery eviction candidate is pinned, the pool now retains over capacity
(recorded in
Stats.OverCommits) rather than force-evicting a pinnedmember — which frees no fd (a pinned member's
ReleaseNowonlylatches) and would only churn accounting and defeat reuse. Non-pinned
members are evicted as before; the LRU self-corrects as pins release.