Skip to content

Comments

fix: remove unnecessary async blocking in ProcessExit handler (#4854)#4915

Merged
thomhurst merged 1 commit intomainfrom
fix/async-blocking
Feb 19, 2026
Merged

fix: remove unnecessary async blocking in ProcessExit handler (#4854)#4915
thomhurst merged 1 commit intomainfrom
fix/async-blocking

Conversation

@thomhurst
Copy link
Owner

Summary

  • EngineCancellationToken.cs: Replaced Task.Delay(500).GetAwaiter().GetResult() with Thread.Sleep(500) in the OnProcessExit handler. The handler runs on a synchronous event handler thread and only needs a simple delay — there is no reason to involve the task scheduler. This follows the project rule of never blocking on async (no .Result or .GetAwaiter().GetResult()).
  • DedicatedThreadExecutor.cs: The .GetAwaiter().GetResult() in SynchronizationContext.Send is intentional and safe — Send is synchronous by API contract, the calling thread is verified to not be the dedicated thread, and the Task.Run wrapper ensures no SynchronizationContext is captured. Added detailed comments explaining why this blocking wait is safe from deadlocks.

Closes #4854

Test plan

  • dotnet build TUnit.Core/TUnit.Core.csproj passes with zero warnings/errors across all target frameworks (net8.0, net9.0, net10.0, netstandard2.0)
  • CI pipeline passes

Replace Task.Delay(...).GetAwaiter().GetResult() with Thread.Sleep in
the ProcessExit handler, since it's a synchronous event handler that
just needs a simple delay without involving the task scheduler.

Add detailed comments to the intentional blocking wait in
DedicatedThreadExecutor.Send explaining why it is safe from deadlocks.
@claude
Copy link
Contributor

claude bot commented Feb 19, 2026

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

Summary

This is a clean, focused fix. The two changes are well-reasoned and correct:

EngineCancellationToken.cs — Replacing Task.Delay(TimeSpan.FromMilliseconds(500)).GetAwaiter().GetResult() with Thread.Sleep(500) in the OnProcessExit synchronous event handler is the right call. The old code unnecessarily involved the task scheduler just to block the current thread — Thread.Sleep achieves exactly the same result with less overhead and without violating the no-blocking-on-async principle. The updated comments clearly explain the reasoning.

DedicatedThreadExecutor.cs — The expanded comment accurately documents the four specific reasons why the existing blocking wait cannot deadlock in that context (SynchronizationContext.Send is synchronous by API contract, the dedicated thread is verified not to be the current thread, and Task.Run removes the SynchronizationContext). This is good defensive documentation for a legitimately tricky piece of code.

Both changes improve code correctness and maintainability.

@thomhurst thomhurst merged commit 6a27b94 into main Feb 19, 2026
14 checks passed
@thomhurst thomhurst deleted the fix/async-blocking branch February 19, 2026 01:09
This was referenced Feb 22, 2026
This was referenced Feb 23, 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: blocking on async in ProcessExit handler and DedicatedThreadExecutor

1 participant