fix(pooled): throw ObjectDisposedException from Count/Keys/Values after Dispose (#296) - #299
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes a documented disposal contract gap in pooled collections where certain read accessors remained usable after Dispose, potentially exposing arrays already returned to ArrayPool.Shared (issue #296).
Changes:
- Guarded
CountonPooledCeleritySetandCount/Keys/ValuesonPooledCelerityDictionarywithThrowIfDisposed(). - Extended existing “use-after-dispose” tests to assert
ObjectDisposedExceptionfor those accessors (and added coverage forEnsureCapacity/TrimExcesson the dictionary). - Documented the fix in
CHANGELOG.mdunder[Unreleased].
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/Celerity/Collections/PooledCeleritySet.cs | Ensures Count throws ObjectDisposedException after disposal via ThrowIfDisposed(). |
| src/Celerity/Collections/PooledCelerityDictionary.cs | Ensures Count, Keys, and Values throw ObjectDisposedException after disposal via ThrowIfDisposed(). |
| src/Celerity.Tests/Collections/PooledCeleritySetTests.cs | Adds regression assertion that Count throws after disposal. |
| src/Celerity.Tests/Collections/PooledCelerityDictionaryTests.cs | Adds regression assertions for Count/Keys/Values and extends use-after-dispose coverage for EnsureCapacity/TrimExcess. |
| CHANGELOG.md | Adds an [Unreleased] “Fixed” entry describing the disposal-contract fix and linking #296. |
Coverage
Files below 100% line coverage
|
Benchmarks6 regressions Highlights
Collections (396)
Hashers (100)
Same-runner A/B (sharded 6-way): main ( |
…er Dispose PooledCelerityDictionary and PooledCeleritySet document that "after Dispose every member throws ObjectDisposedException", but the read accessors Count (both types) and Keys / Values (dictionary) skipped ThrowIfDisposed(). Since Dispose zeroes _count and returns the backing arrays to ArrayPool.Shared, a post-dispose read returned a silent, misleading result (0, or an empty view) over buffers the pool may have re-handed out. Gate all three on ThrowIfDisposed() so the code matches the documented contract, and extend the UseAfterDispose regression tests to cover them. Closes #296 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
marius-bughiu
force-pushed
the
fix/issue-296-pooled-disposed-accessors
branch
from
July 24, 2026 01:08
a3778c9 to
5821262
Compare
Owner
Author
|
Rebased onto |
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.
What & why
PooledCelerityDictionaryandPooledCeleritySetboth document — in the class<remarks>, the_disposedfield comment, and theDispose()summary — that "afterDisposeevery member throwsObjectDisposedException." Several read accessors did not honour that contract:PooledCelerityDictionary.Count,Keys,ValuesPooledCeleritySet.CountDispose()sets_count = 0and returns the backing key/value arrays toArrayPool<T>.Shared. So a post-disposeCountread returned a silent0, andKeys/Valueshanded back a view over arrays already back in the pool — potentially observing buffers the pool has since re-handed to another caller. That is a correctness/robustness gap, not just a doc nit. Fixes #296.The fix routes all four accessors through the existing
ThrowIfDisposed()guard that the rest of each type's surface already uses, so the code now matches the long-documented contract. No behaviour change before disposal.Parity checklist (bug fix)
UseAfterDispose_ShouldThrowObjectDisposedExceptionfact inPooledCelerityDictionaryTests(addsCount/Keys/Values, plus the previously-missingEnsureCapacity/TrimExcess) andPooledCeleritySetTests(addsCount). These assertions fail onmain.IDisposablecollections and there is no shared use-after-dispose test to extend.docs/api/collections.mdalready states "after it every member throwsObjectDisposedException" for both pooled types; this fix makes the code match the already-correct docs.### Fixedbullet under[Unreleased].done.Test plan
dotnet build(net8.0) — clean, 0 warnings.dotnet test(net8.0) — full suite 4263 passed, 0 failed; pooled subset 233 passed.🤖 Generated with Claude Code