Skip to content

fix: Playwright BrowserTest resource leaks and thread safety#4930

Merged
thomhurst merged 1 commit intomainfrom
fix/playwright-leaks
Feb 19, 2026
Merged

fix: Playwright BrowserTest resource leaks and thread safety#4930
thomhurst merged 1 commit intomainfrom
fix/playwright-leaks

Conversation

@thomhurst
Copy link
Owner

Summary

  • Browser contexts now always closed on teardown -- previously, BrowserTest.BrowserTearDown only closed contexts when the test passed; on failure, contexts were cleared from the list but never actually closed, leaking browser resources.
  • Resilient disposal loops -- if closing one browser context or disposing one worker service throws, the remaining resources are still cleaned up. Exceptions are collected and re-thrown as an AggregateException after all resources have been processed.
  • Null-safe BrowserService.DisposeAsync -- added a null check before calling Browser.CloseAsync() to guard against cases where the browser was never successfully created.
  • Thread-safe context tracking -- BrowserTest._contexts list is now protected by a lock to prevent concurrent modification if NewContext is called from multiple async continuations.
  • Worker teardown safety -- WorkerAwareTest.WorkerTeardown now handles the case where _currentWorker is null, and if ResetAsync throws during the "test passed" path, it falls back to disposing all services rather than leaking them.
  • Optimized RegisterService -- replaced ContainsKey + indexer double-lookup with a single TryGetValue call.

Closes #4879

Test plan

  • dotnet build TUnit.Playwright/TUnit.Playwright.csproj passes on all target frameworks (netstandard2.0, net8.0, net9.0, net10.0)
  • Verify existing Playwright template tests still pass
  • Manual review: confirm all CloseAsync/DisposeAsync calls are now wrapped in try/catch with proper exception aggregation
  • Manual review: confirm contexts are closed regardless of test pass/fail state

@claude
Copy link
Contributor

claude bot commented Feb 19, 2026

Code Review

Overall, this PR makes meaningful improvements to resource management and thread safety in the Playwright integration. The changes are well-structured and address real issues (resource leaks, missing null guards, resilient disposal with exception aggregation). One bug was found in the exception handling logic.


Bug: Original exception lost when DisposeAllServicesAsync throws in WorkerTeardown

File: TUnit.Playwright/WorkerAwareTest.cs

In the WorkerTeardown method, the catch block calls DisposeAllServicesAsync(worker) before re-throwing:

catch
{
    await DisposeAllServicesAsync(worker).ConfigureAwait(false);
    throw;  // unreachable if DisposeAllServicesAsync throws
}

DisposeAllServicesAsync is explicitly designed to throw an AggregateException when any DisposeAsync() call fails. If that happens, the AggregateException propagates out of the catch block and the throw; on the next line is never reached — the original exception from ResetAsync() is permanently lost.

This is straightforward C# behavior: any exception thrown inside a catch block replaces the in-flight exception; the original is not automatically chained or preserved.

Suggested fix:

catch (Exception resetEx)
{
    try
    {
        await DisposeAllServicesAsync(worker).ConfigureAwait(false);
    }
    catch (AggregateException disposeEx)
    {
        throw new AggregateException(
            "ResetAsync failed and one or more services also failed to dispose.",
            [resetEx, .. disposeEx.InnerExceptions]);
    }
    throw;
}

This ensures both the original reset failure and any disposal failures are surfaced to the caller.


Positive observations

  • The lock-based _contextsLock pattern in BrowserTest is correct and the snapshot-before-clear approach is the right way to handle concurrent teardown.
  • The AggregateException collection pattern in BrowserTearDown and DisposeAllServicesAsync is well-implemented — it correctly continues disposing remaining resources even when one fails.
  • Null-checking Browser before calling CloseAsync() in BrowserService.DisposeAsync is a safe improvement.
  • The TryGetValue optimization in RegisterService is correct and cleaner than the double-lookup.
  • ConfigureAwait(false) is consistently applied throughout.

Browser contexts were not closed on test failure paths in BrowserTest,
leading to leaked browser resources. Additionally, if any single context
or service disposal threw an exception, remaining resources were skipped
entirely. This change ensures all contexts and services are always
disposed via try/catch loops that collect exceptions, adds null-safety
to BrowserService.DisposeAsync, protects the context list with a lock,
and falls back to full disposal when WorkerAwareTest.ResetAsync fails.
@thomhurst thomhurst force-pushed the fix/playwright-leaks branch from 31958e1 to bd8dbde Compare February 19, 2026 11:46
@thomhurst thomhurst merged commit bc0783a into main Feb 19, 2026
13 of 14 checks passed
@thomhurst thomhurst deleted the fix/playwright-leaks branch February 19, 2026 13:54
This was referenced Mar 3, 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.

fix: Playwright BrowserTest resource leaks and thread safety

1 participant