Fix CPU spin in BatchingChannelReader when source completes#53
Fix CPU spin in BatchingChannelReader when source completes#53electricessence merged 5 commits intomasterfrom
Conversation
Co-authored-by: electricessence <5899455+electricessence@users.noreply.github.com>
The issue was that after Task.WhenAny completed, the code did not await the source task before looping. This caused a tight CPU-consuming loop when the source channel was closed but the buffer was empty, as the closed source would immediately return a completed ValueTask on each WaitToReadAsync call. The fix properly awaits the source task after WhenAny to check its completion status, preventing the tight loop. This matches the pattern used in the base BufferingChannelReader class. Co-authored-by: electricessence <5899455+electricessence@users.noreply.github.com>
- Remove unused System.Text.Json import from test - Use consistent attribute style in test - Add clarifying comment explaining the buffer completion check Co-authored-by: electricessence <5899455+electricessence@users.noreply.github.com>
Updated Open.ChannelExtensions.csproj to increase the project version from 9.1.1 to 9.1.2. No other modifications were made.
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical CPU spin bug in BatchingChannelReader where tasks would hang indefinitely and consume CPU when the source channel completed with an empty buffer. The root cause was that after Task.WhenAny returned, the source task was not awaited before looping, causing a tight spin loop when the source channel closed.
Changes:
- Fixed
BatchingChannelReader.WaitToReadAsyncCoreto await the source task afterWhenAnybefore looping back - Added regression test to verify concurrent batched readers don't hang
- Bumped version from 9.1.1 to 9.1.2
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Open.ChannelExtensions/Readers/BatchingChannelReader.cs | Fixes CPU spin by awaiting source task after WhenAny and checking buffer completion before looping |
| Open.ChannelExtensions.Tests/HangReproTest.cs | Adds test to reproduce and verify the fix for the hanging issue with multiple concurrent batch readers |
| Open.ChannelExtensions/Open.ChannelExtensions.csproj | Version bump from 9.1.1 to 9.1.2 for the bug fix release |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [Fact] | ||
| public static async Task MultipleBatchReadersDoNotHang() | ||
| { | ||
| var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30)); // 30 second timeout |
There was a problem hiding this comment.
The CancellationTokenSource should be disposed using a using declaration to follow the codebase convention and ensure proper resource cleanup. Other tests in this codebase consistently use using var tokenSource = new CancellationTokenSource(...) pattern.
| var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30)); // 30 second timeout | |
| using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30)); // 30 second timeout |
| <GenerateDocumentationFile>true</GenerateDocumentationFile> | ||
| <Version>9.1.1</Version> | ||
| <Version>9.1.2</Version> | ||
| <PackageReleaseNotes>Ensure the current task scheduler is used and that it can be configured.</PackageReleaseNotes> |
There was a problem hiding this comment.
The PackageReleaseNotes should be updated to reflect the actual changes in this release. The current text "Ensure the current task scheduler is used and that it can be configured." appears to be from a previous version and doesn't describe the CPU spin bug fix that is the focus of this PR.
| <PackageReleaseNotes>Ensure the current task scheduler is used and that it can be configured.</PackageReleaseNotes> | |
| <PackageReleaseNotes>Fix a CPU spin issue under high contention when using channel extensions.</PackageReleaseNotes> |
| // If so, return immediately without trying to pipe more items | ||
| if (b.IsCompleted) | ||
| return await b.ConfigureAwait(false); | ||
|
|
There was a problem hiding this comment.
@copilot. after re-reviewing this code, I'm concerned that the assumptions here are wrong.
if (b.IsCompleted) // Assuming it was bufferWait that completed.
{
await tokenSource.CancelAsync().ConfigureAwait(false);
return await b.ConfigureAwait(false);
}
If you make it past the above block due to Task.WhenAny, then you absolutely know that sTask must of completed.
So like 258 is moot.
And 261 might actually change the behavior.
Tasks reading from batched channels would hang indefinitely and consume CPU when the source channel completed but the buffer was empty.
Root Cause
In
BatchingChannelReader.WaitToReadAsyncCore, afterTask.WhenAnyreturned, the source task was not awaited before looping. When the source channel closed, it returned completed ValueTasks immediately, creating a tight spin loop:Changes
WhenAnyto verify completion status before loopingThe fix aligns with the base
BufferingChannelReaderpattern and prevents the infinite loop while preserving timeout/force-batch semantics.Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.