fix: replace sync-over-async Task.Delay with Thread.Sleep in sync pumps (#4073)#4074
Merged
iancooper merged 4 commits intoBrighterCommand:masterfrom Apr 26, 2026
Merged
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.
iancooper
approved these changes
Apr 25, 2026
Member
|
Thanks @thomhurst. You are right, mostly these are deliberate and it may be better to express that over panic folks seeing a GetAwaiter().GetResult(). |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…in sync pumps These sites run on dedicated LongRunning threads or the Dispatcher control thread and explicitly block a sync thread — there is nothing to yield to. Task.Delay().GetAwaiter().GetResult() / .Wait() allocates a Task, registers a TimerQueue timer, and parks on a ManualResetEventSlim to do what Thread.Sleep does with a single OS primitive. Switching to Thread.Sleep removes unnecessary allocations and TimerQueue registrations from hot pump loops, and eliminates the TimerQueue as a variable when diagnosing pump-shutdown hangs. Sites changed: - Reactor.cs: broken-circuit retry, channel-failure retry, empty-channel pause - Dispatcher.cs: Start spin-wait for DS_RUNNING - RmqMessageGatewayConnectionPool.cs: connect-retry jitter - RMQ.Sync/PullConsumer.cs: pull-consumer pause Proactor.cs and other async paths (await Task.Delay) are left unchanged. Closes BrighterCommand#4073
MsSqlMessageQueue<T>.TryReceive(topic, timeout) is a sync polling loop calling the sync TryReceive(topic) overload. The Task.Delay().GetAwaiter().GetResult() between polls is the same anti-pattern as BrighterCommand#4073 — no async path to yield to, just unnecessary Task allocation and TimerQueue registration. Not listed in BrighterCommand#4073 but found during audit of src/ for similar patterns. Related to BrighterCommand#4073.
thomhurst
force-pushed
the
fix/4073-sync-sleep
branch
from
1431f68 to
26487ee
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lillo42
approved these changes
Apr 25, 2026
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #4073.
Replaces
Task.Delay(x).GetAwaiter().GetResult()/.Wait()withThread.Sleep(x)at sites running on dedicated sync threads (pump loops, Dispatcher control thread, connection-pool retry jitter). Each of these sites explicitly blocks a sync-only thread — there is nothing to yield to — so the async primitive just allocates aTask, registers aTimerQueuetimer, and parks on aManualResetEventSlimto do whatThread.Sleepdoes with a single OS primitive.Benefits:
TimerQueueregistrations from hot pump loops.TimerQueueas a variable when diagnosing pump-shutdown hangs (see Replace sync-over-async Task.Delay().GetAwaiter().GetResult() with Thread.Sleep in sync pumps #4073 for context on the Linux CI hang investigation).Sites changed
Six from the issue:
src/Paramore.Brighter.ServiceActivator/Reactor.cs:123— broken-circuit retrysrc/Paramore.Brighter.ServiceActivator/Reactor.cs:131— channel-failure retrysrc/Paramore.Brighter.ServiceActivator/Reactor.cs:155— empty-channel pausesrc/Paramore.Brighter.ServiceActivator/Dispatcher.cs:342—Start()spin-wait forDS_RUNNINGsrc/Paramore.Brighter.MessagingGateway.RMQ.Sync/RmqMessageGatewayConnectionPool.cs:151— connect-retry jittersrc/Paramore.Brighter.MessagingGateway.RMQ.Sync/PullConsumer.cs:78— pull-consumer pauseOne additional found during audit of
src/for the same anti-pattern:src/Paramore.Brighter.MessagingGateway.MsSql/SqlQueues/MsSqlMessageQueue.cs:105—TryReceive(topic, timeout)sync polling loopAlso dropped
using System.Threading.Tasks;from the three files where it became unused after the substitution.Proactor.csand all genuineawait Task.Delay(...)paths are unchanged — those correctly yield on the async pump. Test-code uses ofTask.Delay(...).Wait()are out of scope (they run on the test-runner thread, not dedicated pump threads).Test plan
Paramore.Brighter.ServiceActivatorbuilds clean on all TFMs (netstandard2.0, net8.0, net9.0, net10.0) — verified locally.Paramore.Brighter.MessagingGateway.RMQ.Syncbuilds clean on all TFMs — verified locally.Paramore.Brighter.MessagingGateway.MsSqlbuilds clean on all TFMs (including net462) — verified locally.When_A_Message_Dispatcher_Shuts_A_Connectionremains stable in CI after removing theTimerQueuefrom the Reactor pause path.