feat(pinner): add Close with ErrClosed lifecycle#1150
Draft
Conversation
Pinner gains Close() error. Close waits for every in-flight operation, including streaming goroutines from RecursiveKeys, DirectKeys, and InternalPins, to finish. After Close, every other Pinner method fails fast with a new ErrClosed sentinel; streaming methods surface it as the Err of a single entry on the returned channel, which is then closed. dspinner gains the matching implementation: Close is idempotent, admission is serialised with sync.Mutex + sync.WaitGroup, and stream sends select on a shutdown channel so a parked consumer cannot stall Close. The panic-recovery and context guards added in #1146 stay in place as defence in depth for hosts that do not wire Close correctly. Hosts that own the backing datastore (e.g. kubo) should call Close on the pinner before closing the datastore to avoid use-after-close panics in pebble and similar stores.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1150 +/- ##
==========================================
+ Coverage 63.10% 63.16% +0.06%
==========================================
Files 267 267
Lines 26777 26832 +55
==========================================
+ Hits 16897 16948 +51
- Misses 8158 8162 +4
Partials 1722 1722
... and 9 files with indirect coverage changes 🚀 New features to boost your workflow:
|
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
1 participant
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.
This is an optional follow-up me and @gammazero discussed during colo this week. We could ass
Close()to pinner to match other places in boxo (that have explicit Closer interface).This PR
Pinner gains Close() error. Close waits for every in-flight operation, including streaming goroutines from RecursiveKeys, DirectKeys, and InternalPins, to finish. After Close, every other Pinner method fails fast with a new ErrClosed sentinel; streaming methods surface it as the Err of a single entry on the returned channel, which is then closed.
dspinner gains the matching implementation: Close is idempotent, admission is serialised with sync.Mutex + sync.WaitGroup, and stream sends select on a shutdown channel so a parked consumer cannot stall Close. The panic-recovery and context guards added in #1146 stay in place as defence in depth for hosts that do not wire Close correctly.
Hosts that own the backing datastore (e.g. kubo) should call Close on the pinner before closing the datastore to avoid use-after-close panics in pebble and similar stores.