fix(extractor): null-conditional dispose + cross-member pool release (mined ImageSharp FP #2,#3/4)#84
Conversation
…l release (mined ImageSharp #2,#3/4) Two more field-detector precision fixes from the ImageSharp mining run. #2 — null-conditional disposal. The disposed-field set matched only a plain `field.Dispose()`; the dominant `field?.Dispose()` (a ConditionalAccess whose WhenNotNull is the .Dispose() invocation) was missed, so a field disposed that way was reported as a leak. Mined across ImageSharp: `this.memoryStream?.Dispose()` (ZipExrCompressor, DeflateCompressor, IccDataWriter) and the BufferedStreams benchmark's `[GlobalCleanup]`. #3 — cross-member pool release. The per-member pool pass required the Return in the same member as the Rent, but a FIELD buffer is rented in the ctor and returned in Dispose (different members). A field is now also released if `pool.Return(field)` appears ANYWHERE in the class (a field name is class-unique, so no cross-masking; locals keep per-member scoping), or if the buffer is TRANSFERRED into a `new Guard(field)` that the object stores in a field — the #80 escaping-ctor transfer at field level. Mined: BufferedReadStream (returns this.readBuffer in Dispose(bool)) and SharedArrayPoolBuffer (LifetimeGuard). Regression sample FieldReleaseSample.cs: `field?.Dispose()` and the cross-member / guard-transferred pooled fields stay silent; an undisposed field and a rented-never-returned pooled field still warn. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://cdn.github.com/in/57789?s=60&v=4){kind=small}
| // #2 control: an IDisposable field the class new's but never disposes -> must WARN. | ||
| public sealed class NeverDisposesField | ||
| { | ||
| private readonly MemoryStream stream = new(); |
| { | ||
| private readonly byte[] leakedBuf; | ||
|
|
||
| public PoolFieldLeaked(int n) => this.leakedBuf = ArrayPool<byte>.Shared.Rent(n); |
![chatgpt-codex-connector[bot]](https://cdn.github.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cec7c7e5ac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (model.GetSymbolInfo(arg.Expression).Symbol is IFieldSymbol afs) | ||
| fieldReleased.Add(afs.Name); |
There was a problem hiding this comment.
When a rented field is passed to any object creation whose result is stored in a field, this unconditionally adds the buffer to fieldReleased. That suppresses POOL001 even for non-owning wrappers such as _view = new ReadOnlyMemory<byte>(_buf) or other cached views, where the rented array is still never returned. This should be limited to constructors/types known to own and return the buffer, or otherwise backed by an actual release.
Useful? React with 👍 / 👎.
…eturn only (Codex) Codex P2: treating a rented field passed to ANY field-stored `new X(field)` as released wrongly suppresses a real leak for NON-owning wrappers (`_view = new ReadOnlyMemory<byte>(_buf)` and other cached views), where the array is never returned. Soundly distinguishing an owning guard (that Returns the buffer) from a non-owning view is not worth it for the single SharedArrayPoolBuffer FP, so the guard-transfer is removed entirely. Fix #3 keeps only the sound, high-value part: a pooled FIELD is released if `pool.Return(field)` appears anywhere in the class (cross-member ctor-rent + Dispose-return — BufferedReadStream). SharedArrayPoolBuffer's indirect release via its LifetimeGuard is left as an honest known limitation. Sample/CI drop the PoolFieldTransferredToGuard case accordingly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
9fc863e
into
claude/fp-resolve-aware-disposable
3 participants
Mined from
SixLabors/ImageSharp— fixes 2 & 3 of 4Stacked on #83 (retargets to
mainwhen #83 merges). Two more field-detector precision fixes.#2 — null-conditional disposal
field?.Dispose()The disposed-field set matched only a plain
field.Dispose(). The dominantfield?.Dispose()shape (aConditionalAccessExpressionwhoseWhenNotNullis the.Dispose()invocation) was missed, so a field disposed that way was reported as a leak. Mined across ImageSharp:this.memoryStream?.Dispose()—ZipExrCompressor,DeflateCompressor,IccDataWriterBufferedStreamsbenchmark's[GlobalCleanup](field?.Dispose()×8)#3 — cross-member pool release
The per-member pool pass required the
Returnin the same member as theRent. But a field buffer is rented in the ctor and returned inDispose— different members. A field is now also released if:pool.Return(field)appears anywhere in the class (a field name is class-unique, so no cross-masking; locals keep per-member scoping) —BufferedReadStreamreturnsthis.readBufferinDispose(bool); ornew Guard(field)the object stores in a field — the fix(extractor): pooled buffer passed to an escaping constructor is a transfer, not a leak (mined FP) #80 escaping-ctor transfer at field level —SharedArrayPoolBuffer'sLifetimeGuard.Regression guard
FieldReleaseSample.cs—field?.Dispose()and the cross-member / guard-transferred pooled fields stay silent; an undisposed field and a rented-never-returned pooled field still warn.Status of the 4-bug batch
static classwith static handlers — next PR🤖 Generated with Claude Code
https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Generated by Claude Code