-
Notifications
You must be signed in to change notification settings - Fork 0
fix(extractor): exempt EventSource-owned DiagnosticCounter fields from the field-disposable leak (mined Npgsql) #89
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| using System; | ||
| using System.Diagnostics.Tracing; | ||
| using System.IO; | ||
| using System.Threading; | ||
|
|
||
| namespace Own.Samples; | ||
|
|
||
| // P-004 EventSource diagnostic-counter exemption (mined: Npgsql NpgsqlEventSource). A | ||
| // DiagnosticCounter — EventCounter / PollingCounter / IncrementingEventCounter / | ||
| // IncrementingPollingCounter — constructed with `this` (the parent EventSource) registers | ||
| // itself with that source: the runtime's CounterGroup pins it to the EventSource's lifetime, | ||
| // and an EventSource is a process-lived `static readonly Log` singleton, so the counter is a | ||
| // process-lived diagnostic that is idiomatically NEVER field-disposed (every BCL EventSource — | ||
| // RuntimeEventSource, the HTTP/ASP.NET counters — does exactly this). The counter fields must | ||
| // therefore be SILENT. Contrast `_scratch` below: a plain owned IDisposable in the SAME | ||
| // EventSource STILL leaks, proving the rule keys off the DiagnosticCounter type handed to | ||
| // `this`, not "any field declared in an EventSource". | ||
| [EventSource(Name = "Own-Sample-Counters")] | ||
| internal sealed class SampleEventSource : EventSource | ||
| { | ||
| public static readonly SampleEventSource Log = new SampleEventSource(); | ||
|
|
||
| private long _bytesWritten; | ||
|
|
||
| // The four counter kinds, all built in OnEventCommand with `this` -> owned by the source -> SILENT. | ||
| private IncrementingPollingCounter? _bytesPerSecond; | ||
| private PollingCounter? _totalBytes; | ||
| private EventCounter? _commandDuration; | ||
| private IncrementingEventCounter? _totalCommands; | ||
|
|
||
| // Codex control: a NON-counter owned IDisposable field in the same EventSource is NOT exempt. | ||
| // The exemption keys off the DiagnosticCounter type handed to `this`, not the containing class, | ||
| // so this new'd-but-never-disposed resource STILL raises OWN001 ('disposable field'). | ||
| private readonly OwnedScratch _scratch = new OwnedScratch(); | ||
Check warningCode scanning / Own.NET owned resource not released on all paths (possible leak) Warning
IDisposable field '_scratch' (type 'OwnedScratch') is never disposed — its owner 'SampleEventSource' leaks it (leak) [resource: disposable field]
Check failure on line 34 in frontend/roslyn/samples/EventSourceCountersSample.cs
|
||
|
github-advanced-security[bot] marked this conversation as resolved.
Fixed
|
||
|
|
||
| // Codex control: a field DECLARED as a plain IDisposable that is ALSO assigned a counter once | ||
| // must STILL leak. The exemption requires the DECLARED field type to be a DiagnosticCounter, so | ||
| // the `new MemoryStream()` owned here is not hidden by the later `_mixed = new EventCounter(...)` | ||
| // (a name-only skip would wrongly suppress the whole field). -> OWN001 on the MemoryStream. | ||
| private IDisposable _mixed = new MemoryStream(); | ||
Check warningCode scanning / Own.NET owned resource not released on all paths (possible leak) Warning
IDisposable field '_mixed' (type 'IDisposable') is never disposed — its owner 'SampleEventSource' leaks it (leak) [resource: disposable field]
Check failure on line 40 in frontend/roslyn/samples/EventSourceCountersSample.cs
|
||
|
|
||
|
|
||
| private SampleEventSource() { } | ||
|
|
||
| public void RecordBytes(long n) => Interlocked.Add(ref _bytesWritten, n); | ||
|
|
||
| protected override void OnEventCommand(EventCommandEventArgs command) | ||
| { | ||
| if (command.Command != EventCommand.Enable) | ||
| return; | ||
|
|
||
| _bytesPerSecond = new IncrementingPollingCounter("bytes-per-second", this, () => Interlocked.Read(ref _bytesWritten)) | ||
| { | ||
| DisplayName = "Bytes Written Rate", | ||
| DisplayRateTimeScale = TimeSpan.FromSeconds(1), | ||
| }; | ||
| _totalBytes = new PollingCounter("total-bytes", this, () => Interlocked.Read(ref _bytesWritten)) | ||
| { | ||
| DisplayName = "Total Bytes Written", | ||
| }; | ||
| _commandDuration = new EventCounter("command-duration", this) | ||
| { | ||
| DisplayName = "Command Duration", | ||
| DisplayUnits = "ms", | ||
| }; | ||
| _totalCommands = new IncrementingEventCounter("total-commands", this) | ||
| { | ||
| DisplayName = "Total Commands", | ||
| }; | ||
|
|
||
| // The `_mixed` field (declared IDisposable) is ALSO assigned a counter here. The exemption | ||
| // must NOT suppress it: its declared type is not a DiagnosticCounter, so its earlier | ||
| // `new MemoryStream()` is a real undisposed leak (Codex). | ||
| _mixed = new EventCounter("mixed-counter", this); | ||
| } | ||
| } | ||
|
|
||
| internal sealed class OwnedScratch : IDisposable | ||
| { | ||
| public void Dispose() { } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.