fix: Mediator Runner/Waker RunAsync awaits inner work (#4078)#4080
Conversation
thomhurst
requested review from
DevJonny,
holytshirt,
iancooper and
preardon
as code owners
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
thomhurst
force-pushed
the
fix/4078-mediator-runner-fire-and-forget
branch
from
1613019 to
2ecd1ba
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
iancooper
left a comment
iancooper
left a comment
There was a problem hiding this comment.
Thanks @thomhurst for the fixes. All good stuff, just want to note that Mediator is a WiP, and so may change significantly
thomhurst
force-pushed
the
fix/4078-mediator-runner-fire-and-forget
branch
from
2ecd1ba to
e7a2f89
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…d#4078) Task.Factory.StartNew(async () => ...) returned a Task<Task> whose outer task completed at the lambda's first await, so Task.WaitAll signaled "Finished" while ProcessJobs/Wake was still running. Switch RunAsync to async Task and await Task.Run(() => ProcessJobs(ct), ct) so callers actually observe completion. Pinning to TaskScheduler.Default also guards against the deadlock pattern in BrighterCommand#4071. Tests updated to fire-and-forget the now-Task-returning method with a short settle delay before assertions.
Tests now await the runner to completion. For workflows that don't re-enqueue mid-execution (the majority), close the channel after scheduling so the runner exits cleanly when the queue drains. For workflows that do re-enqueue (parallel split, blocking wait), keep fire-and-forget but capture and observe the task at the end of the test so OperationCanceledException isn't unobserved. Also tightens the catch from Exception to OperationCanceledException since that's the only exception the now-async path can surface.
thomhurst
force-pushed
the
fix/4078-mediator-runner-fire-and-forget
branch
from
e7a2f89 to
074af38
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Quality Gate Profile: Clean Code Collective
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
2 participants
Summary
Fixes #4078.
Runner.RunAsyncandWaker.RunAsyncusedTask.Factory.StartNew(async () => ...)without.Unwrap(). The outerTask<Task>signaled completion at the lambda's firstawait, soTask.WaitAllreturned and "Finished" was logged whileProcessJobs/Wakewas still running on the threadpool. Hosts relying onRunAsyncreturning to know when shutdown completes saw a false signal.Changes
Runner.RunAsync/Waker.RunAsyncnow returnTaskandawait Task.Run(() => ProcessJobs(ct), ct), so callers actually observe completion.Task.Runpins toTaskScheduler.Default, also guarding against the deadlock pattern reported in ServiceActivator Dispatcher/Performer can deadlock under non-default TaskScheduler #4071.cancellationToken.ThrowIfCancellationRequested()calls afterawaitand the pointlessif (ct.IsCancellationRequested) ct.ThrowIfCancellationRequested()guard.finallyblock so it fires on both clean exit and cancellation._runner.RunAsync(ct.Token);→_ = _runner.RunAsync(ct.Token); await Task.Delay(100ms);to preserve the implicit settle window the bug was accidentally providing.Test plan
dotnet test --filter "FullyQualifiedName~Workflows" -f net9.0— 12/12 passingdotnet build src/Paramore.Brighter.Mediator— clean