fix(extractor): SemaphoreSlim field dispose-optional unless AvailableWaitHandle is read (mined Npgsql)#92
Conversation
…bleWaitHandle is read (mined Npgsql) The sound, scoped version of the SemaphoreSlim FP (per review on #91 + the user's choice). SemaphoreSlim.Dispose() only frees a LAZILY-allocated wait handle — allocated solely when AvailableWaitHandle is read — so a SemaphoreSlim field used purely for Wait/WaitAsync/Release leaks nothing and is dispose-optional. Mined: NpgsqlDataSource._setupMappingsSemaphore. Two guards keep it sound, both flagged by reviewers on the earlier attempt: - FIELD-scoped: the exemption lives in the field-disposable detector, NOT in the shared IsDisposeOptional — so the flow-locals detector and the deliberate method-bounded `semLeak` control (a prior ShareX decision: SemaphoreSlim stays tracked for locals) are left untouched (CodeRabbit). - AvailableWaitHandle-GATED: if `.AvailableWaitHandle` is read on the field, the wait handle exists and Dispose must release it, so the field STAYS tracked (Codex). The gate keys on a this/bare-receiver `.AvailableWaitHandle` access. Regression sample SemaphoreFieldSample.cs: OptionalSemaphore._optionalSem (Wait/ Release only) is SILENT; controls still warn — WaitHandleSemaphore._handleSem (reads AvailableWaitHandle) and HoldsCtsField._ctsControl (a non-SemaphoreSlim CTS). The existing FlowLocalsSample.semLeak local control is unaffected (separate detector path), asserted in the --flow-locals step. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
![github-advanced-security[bot]](https://cdn.github.com/in/57789?s=60&v=4){kind=small}
| // Dispose must release it -> the field must STILL warn OWN001. | ||
| public sealed class WaitHandleSemaphore | ||
| { | ||
| private readonly SemaphoreSlim _handleSem = new SemaphoreSlim(0, 1); |
| // must STILL warn — the exemption is SemaphoreSlim-specific, not a blanket "any field". | ||
| public sealed class HoldsCtsField | ||
| { | ||
| private readonly CancellationTokenSource _ctsControl = new CancellationTokenSource(); |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a SemaphoreSlim-specific dispose-optional suppression to the OwnSharp extractor: it collects per-class SemaphoreSlim field names where ChangesSemaphoreSlim Dispose-Optional Suppression
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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: f545f3dfc8
ℹ️ 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 (ma.Name.Identifier.Text == "AvailableWaitHandle" && ThisFieldName(ma.Expression) is { } whf) | ||
| waitHandleSemaphores.Add(whf); |
There was a problem hiding this comment.
When a SemaphoreSlim field is copied to a local and the local reads the handle, e.g. var sem = _sem; sem.AvailableWaitHandle.WaitOne(), ThisFieldName(ma.Expression) records the local name (sem) rather than the field name (_sem). The later exemption therefore still treats _sem as dispose-optional and suppresses the leak even though AvailableWaitHandle allocated the handle; the alias map built just above needs to be used here to credit aliased field reads.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/roslyn/OwnSharp.Extractor/Program.cs`:
- Around line 2308-2310: In the foreach loop iterating over
MemberAccessExpressionSyntax nodes where the Name is "AvailableWaitHandle",
replace the current approach of using ThisFieldName(ma.Expression) which keys by
bare identifier text. Instead, use symbol binding to resolve ma.Expression to
its actual IFieldSymbol and add the field symbol itself to waitHandleSemaphores
rather than the text-based field name. This will prevent shadowing locals or
parameters from being mistaken for the actual field.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 84c54c6f-364e-4185-a76b-6171a0b630e4
📒 Files selected for processing (3)
.github/workflows/ci.ymlfrontend/roslyn/OwnSharp.Extractor/Program.csfrontend/roslyn/samples/SemaphoreFieldSample.cs
…field + alias reads (Codex + CodeRabbit) The first cut keyed the AvailableWaitHandle gate by bare identifier text via ThisFieldName, which (a) missed a read through a field ALIAS — `var s = _sem; s.AvailableWaitHandle` recorded `s`, not `_sem`, so the field stayed wrongly exempt (Codex) — and (b) could conflate a shadowing local/parameter with a same-named field (CodeRabbit). Resolve the receiver by SYMBOL: credit the field's AvailableWaitHandle read only when the receiver binds to a real field symbol via a this/bare access (excludes `other._f` and shadowing locals), OR to a field-alias local (reusing the #90 aliasToField map). Anything else is ignored. Regression: SemaphoreFieldSample gains AliasedWaitHandleSemaphore._aliasedSem (AvailableWaitHandle read through `var s = _aliasedSem`) which must STILL warn — the field is tracked through the alias. The existing controls (_optionalSem silent, _handleSem direct-read warns, _ctsControl type-scope warns) are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
| // because the read went through the local name). | ||
| public sealed class AliasedWaitHandleSemaphore | ||
| { | ||
| private readonly SemaphoreSlim _aliasedSem = new SemaphoreSlim(0, 1); |
# Conflicts: # .github/workflows/ci.yml
3 participants
The sound, scoped version of the SemaphoreSlim FP — per the review on #91 and your choice of field-scoped + AvailableWaitHandle-gated.
What
SemaphoreSlim.Dispose()only frees a lazily-allocated wait handle — allocated solely whenAvailableWaitHandleis read — so aSemaphoreSlimfield used purely forWait/WaitAsync/Releaseleaks nothing and is dispose-optional. Mined:NpgsqlDataSource._setupMappingsSemaphore.Two guards (both raised on the earlier attempt in #91)
IsDisposeOptional. So the flow-locals detector and the deliberate method-boundedsemLeakcontrol (a prior ShareX decision: SemaphoreSlim stays tracked for locals) are left untouched. (CodeRabbit).AvailableWaitHandleis read on the field, the wait handle exists andDisposemust release it, so the field stays tracked. (Codex) The gate keys on athis/bare-receiver.AvailableWaitHandleaccess.Regression sample (
SemaphoreFieldSample.cs)OptionalSemaphore._optionalSem(Wait/Release only) → silentWaitHandleSemaphore._handleSem(readsAvailableWaitHandle) andHoldsCtsField._ctsControl(a non-SemaphoreSlim CTS — proves the exemption is type-scoped)FlowLocalsSample.semLeaklocal control is unaffected (separate detector path) — its--flow-localsassertion still passes.Relationship to #91
Independent of #91 (which is the
.Close()-release fix, now green & ready to merge). Different code locations in the field detector; whichever merges second takes a trivial ci.yml rebase.🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
Tests
AvailableWaitHandleis read (including alias-based reads), while confirming unrelated leak expectations remain unchanged.New Features