|
| 1 | +# CancellationTokenSource field leaks are *dispose-optional* — and why we still flag them |
| 2 | + |
| 3 | +A triage note prompted by the cross-tool oracle on Serilog (see |
| 4 | +[`oracle.md`](oracle.md), [`real-world-mining.md`](real-world-mining.md)). It |
| 5 | +records why one of our headline "own-only" findings is weaker than first claimed, |
| 6 | +and the deliberate decision **not** to add a `CancellationTokenSource` (CTS) |
| 7 | +dispose-optional exemption mirroring the SemaphoreSlim one. |
| 8 | + |
| 9 | +## The finding that prompted it |
| 10 | + |
| 11 | +The Serilog oracle's single own-only leak was |
| 12 | +`BatchingSink._shutdownSignal` — a `CancellationTokenSource` field that is |
| 13 | +`Cancel()`-ed in `Dispose()` but never `Dispose()`-d. We presented it as the clean |
| 14 | +field/owner-lifetime differentiator (CodeQL is local-scoped, Infer# method-scoped; |
| 15 | +neither flags it). On a closer read of the Serilog source that over-sells it. |
| 16 | + |
| 17 | +## Why `_shutdownSignal` is effectively benign |
| 18 | + |
| 19 | +From `BatchingSink.cs` (`dev`): `readonly CancellationTokenSource _shutdownSignal = new();` |
| 20 | + |
| 21 | +- `.Token.WaitHandle` is **never** read → no lazily-allocated kernel event (OS handle); |
| 22 | +- no `CancelAfter(...)`/timer → not rooted in the timer queue; |
| 23 | +- not a linked source (`CreateLinkedTokenSource`) → no registration on a parent token; |
| 24 | +- the token only feeds `Task.Delay(Infinite, token)` and a channel read — registration-based cancellation, cleared on `Cancel()`. |
| 25 | + |
| 26 | +So once `Cancel()` runs, the abandoned CTS holds **no unmanaged resource and is rooted |
| 27 | +nowhere**. `CancellationTokenSource` has no finalizer of its own, so a plain instance is |
| 28 | +simply **collected by the GC**; calling `.Dispose()` on it would be a near no-op. This is |
| 29 | +the *same shape* as the SemaphoreSlim dispose-optional exemption (PR #92): a `SemaphoreSlim` |
| 30 | +is dispose-optional until `.AvailableWaitHandle` is read; a **plain** CTS is dispose-optional |
| 31 | +until `.Token.WaitHandle` is read / `CancelAfter` is used / it is linked. `_shutdownSignal` |
| 32 | +reads none of those. (The owner being process-lived — the `Log.Logger` singleton case — makes |
| 33 | +it doubly moot, but loggers *can* churn per-scope, so the plain-CTS argument is the robust one.) |
| 34 | + |
| 35 | +## Why we did **not** add a CTS dispose-optional gate |
| 36 | + |
| 37 | +The #92 SemaphoreSlim gate was cheap because `SemaphoreSlim` is not used as a canonical |
| 38 | +disposable anywhere else. CTS is the opposite — it is **our go-to "owned IDisposable field"** |
| 39 | +across the test surface. A plain-CTS exemption (exempt unless `.Token.WaitHandle` / `CancelAfter` |
| 40 | +/ linked) would flip **~7 "must warn" assertions** in 4 sample files from warn → silent: |
| 41 | + |
| 42 | +| sample | field | what it actually tests | |
| 43 | +|---|---|---| |
| 44 | +| `SemaphoreFieldSample` | `_ctsControl` | the #92 type-scope control | |
| 45 | +| `AliasDisposeSample` | `_neverDisposed`, `_rebound`, `_refRebound`, `_scopedLeak` | alias / rebound / scoped-alias mechanics | |
| 46 | +| `ResolvedDisposableSample` | `cts` (+ a `MemoryStream` field) | resolve-aware disposable field | |
| 47 | +| `DisposableFieldViewModel` | `ReportViewModel._cts` | the field-detector flagship | |
| 48 | + |
| 49 | +None of those are *about* CTS — they use it as a convenient IDisposable. The gate would force |
| 50 | +rewriting them onto a non-optional type, for little gain: real-world dispose-optional instances |
| 51 | +are a **minority** (Serilog `_shutdownSignal`; Npgsql `GlobalTypeMapper._lock`, a |
| 52 | +`ReaderWriterLockSlim` on a singleton) — a couple per repo, not a flood. And the prevailing |
| 53 | +.NET convention for CTS is the **opposite** of SemaphoreSlim: *always dispose it* (CA2000 is |
| 54 | +insistent precisely because the dangerous `CancelAfter`/linked omissions are common and costly). |
| 55 | +A blanket exemption would fight that convention. |
| 56 | + |
| 57 | +> "Look at the scale first" earned its keep here: the same refinement that was narrow and cheap |
| 58 | +> for SemaphoreSlim is broad and against-the-grain for CTS. |
| 59 | +
|
| 60 | +## Decision |
| 61 | + |
| 62 | +- **Keep the conservative detector.** Flagging undisposed CTS fields follows the convention and |
| 63 | + catches the dangerous (`CancelAfter`/linked/`WaitHandle`) cases; a plain-CTS instance is a |
| 64 | + low-severity *instance*, not a reason to exempt the type. |
| 65 | +- **Reframe the differentiator.** `_shutdownSignal` (Serilog) and `_lock` (Npgsql) are |
| 66 | + low-severity, dispose-optional-class instances. The honest flagship for the field/owner-lifetime |
| 67 | + capability is **Npgsql `PoolingDataSource._pruningTimer`** — a `System.Threading.Timer` holds a |
| 68 | + live timer-queue registration and a rooted callback, a *real* leak until disposed. |
| 69 | +- **Deferred option — severity tiers (not exemption).** If we later want the tool to encode |
| 70 | + criticality, split the disposable-field severity: "holds an OS handle / timer / linked CTS" → |
| 71 | + warning; "plain managed (plain CTS, `ReaderWriterLockSlim`, `MemoryStream`)" → info/hint. That |
| 72 | + keeps recall (the instance still surfaces) without flipping any test (warn → info, not |
| 73 | + warn → silent). It needs a curated OS-handle-vs-managed type classifier (an inverted/extended |
| 74 | + `IsDisposeOptional`) and is only worth it if the criticality signal proves valuable on the corpus. |
0 commit comments