Skip to content

feat: add [DetectLeaks] attribute for resource leak detection#4955

Closed
thomhurst wants to merge 1 commit into
mainfrom
feat/leak-detection
Closed

feat: add [DetectLeaks] attribute for resource leak detection#4955
thomhurst wants to merge 1 commit into
mainfrom
feat/leak-detection

Conversation

@thomhurst

Copy link
Copy Markdown
Owner

Summary

  • Adds a new [DetectLeaks] attribute in TUnit.Core that enables opt-in resource leak detection for tests
  • Tracks thread pool worker threads, I/O completion port threads, and active timers (via Timer.ActiveCount on .NET 6+) before and after each test execution
  • Logs warnings to test error output when resource growth exceeds configurable thresholds (default: 10 threads or 10 timers)
  • Implemented as ITestStartEventReceiver + ITestEndEventReceiver, following existing TUnit event receiver patterns
  • Works across all target frameworks (netstandard2.0, net8.0, net9.0, net10.0) with conditional compilation for Timer.ActiveCount

Usage

// On a single test
[Test]
[DetectLeaks]
public async Task MyTest() { }

// On an entire class
[DetectLeaks(ThreadThreshold = 5, TimerThreshold = 5)]
public class MyTestClass
{
    [Test]
    public async Task MyTest() { }
}

Test plan

  • Verify dotnet build TUnit.Core/TUnit.Core.csproj succeeds on all TFMs
  • Verify dotnet build TUnit.Engine/TUnit.Engine.csproj succeeds on all TFMs
  • Apply [DetectLeaks] to a test that intentionally leaks threads and verify warning output
  • Apply [DetectLeaks] to a clean test and verify no warnings appear
  • Verify the attribute works at method, class, and assembly level

Closes #4904

Add opt-in infrastructure for detecting resource leaks during test
execution. The [DetectLeaks] attribute tracks thread pool threads and
active timer counts before/after each test, warning when significant
growth is detected (default threshold: 10).

Closes #4904

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Overall this is a well-structured feature that follows TUnit's event receiver pattern correctly. The ValueTask, ConcurrentDictionary, and #if NET6_0_OR_GREATER guard are all good choices. Two issues worth addressing before merge:


Issue 1 — Missing IScopedAttribute implementation (significant bug)

[AttributeUsage(AttributeTargets.Method | AttributeTargets.Class | AttributeTargets.Assembly, AllowMultiple = false)]
public sealed class DetectLeaksAttribute : TUnitAttribute, ITestStartEventReceiver, ITestEndEventReceiver
{
private static readonly ConcurrentDictionary<string, ResourceSnapshot> Snapshots = new();

DetectLeaksAttribute is applicable at method, class, and assembly level. When a user applies it at more than one level simultaneously — a realistic scenario, e.g. [assembly: DetectLeaks(ThreadThreshold = 50)] as a baseline with [DetectLeaks(ThreadThreshold = 2)] on a specific test — the framework collects both attribute instances and invokes both as event receivers (verified by tracing ReflectionAttributeExtractor.GetAllAttributesScopedAttributeFilter.FilterScopedAttributes).

Because DetectLeaksAttribute does not implement IScopedAttribute, ScopedAttributeFilter does not deduplicate it. Both instances run OnTestStart, and since they share the same static Snapshots dictionary keyed by context.Id, the second OnTestStart overwrites the first's snapshot. When both OnTestEnd handlers run, only the first TryRemove succeeds; the second exits early and its threshold is silently never applied.

Every other TUnit attribute that supports multiple scopes (e.g. RetryAttribute, TimeoutAttribute, NotInParallelAttribute) implements IScopedAttribute so the innermost (most specific) declaration wins. DetectLeaksAttribute should follow the same pattern:

// Before
public sealed class DetectLeaksAttribute : TUnitAttribute, ITestStartEventReceiver, ITestEndEventReceiver

// After
public sealed class DetectLeaksAttribute : TUnitAttribute, ITestStartEventReceiver, ITestEndEventReceiver, IScopedAttribute
{
    // Add:
    public Type ScopeType => typeof(DetectLeaksAttribute);
    // ...
}

With IScopedAttribute, the method-level instance takes precedence and the assembly-level one is filtered out, eliminating the shared-dictionary race.


Issue 2 — I/O completion port delta checked against ThreadThreshold instead of its own threshold

}
if (completionPortDelta >= ThreadThreshold)
{
context.Output.WriteError(
$"[LeakDetection] Test '{testName}' may be leaking I/O completion port threads. " +
$"Available decreased by {completionPortDelta} during execution " +
$"(before: {before.AvailableCompletionPortThreads}, after: {after.AvailableCompletionPortThreads}).");

if (completionPortDelta >= ThreadThreshold)   // uses ThreadThreshold, not TimerThreshold or a dedicated property
{
    context.Output.WriteError(
        $"[LeakDetection] Test '{testName}' may be leaking I/O completion port threads. ...");
}

The class exposes ThreadThreshold (worker threads) and TimerThreshold (timers) as independent knobs, and the ResourceSnapshot struct deliberately separates AvailableWorkerThreads from AvailableCompletionPortThreads. Yet the completion port check silently reuses ThreadThreshold. A user who sets [DetectLeaks(ThreadThreshold = 2, TimerThreshold = 5)] expecting to tune each resource type independently cannot configure the completion port threshold at all — it just follows ThreadThreshold with no documentation or API surface to indicate this.

The fix is either:

Option A — Add a dedicated property (most consistent with the current design):

/// <summary>The minimum completion port thread decrease to trigger a warning. Default is 10.</summary>
public int CompletionPortThreshold { get; set; } = 10;

// then:
if (completionPortDelta >= CompletionPortThreshold) { ... }

Option B — Intentionally bundle both thread types under ThreadThreshold and simplify:

// Combine deltas, single message, single threshold check
var totalThreadDelta = threadDelta + completionPortDelta;
if (totalThreadDelta >= ThreadThreshold) { ... }

Either is reasonable; the current code does neither and leaves the behavior undiscoverable from the public API.

@thomhurst thomhurst closed this Feb 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: test isolation verification and resource leak detection

1 participant