-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Add cancellation token check to BlockingCollection.TakeFromAny fast path #122410
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a bug where BlockingCollection.TakeFromAny and TryTakeFromAny methods ignore pre-cancelled tokens when items are immediately available. The fast path optimization bypassed cancellation checks, allowing operations to succeed even when cancellation was requested before the call.
Key Changes:
- Added early cancellation token check in
TryTakeFromAnyCorebefore the fast path loop executes - Added comprehensive test coverage for
TakeFromAnyandTryTakeFromAnyvariants with pre-cancelled tokens and available items
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| BlockingCollection.cs | Added externalCancellationToken.ThrowIfCancellationRequested() at the start of TryTakeFromAnyCore to honor pre-cancelled tokens before attempting the fast path |
| BlockingCollectionCancellationTests.cs | Added test ExternalCancel_TakeFromAny_PreCanceled_WithAvailableItems covering all three method variants (TakeFromAny, TryTakeFromAny with infinite timeout, TryTakeFromAny with finite timeout) to ensure they throw when given a pre-cancelled token |
BlockingCollection.TakeFromAnyandTryTakeFromAnyignore pre-cancelled tokens when items are immediately available. The fast path bypasses cancellation checks, allowing operations to succeed even when cancellation was requested before the call.Changes
externalCancellationToken.ThrowIfCancellationRequested()at the start ofTryTakeFromAnyCorebefore the fast path loopTakeFromAnyandTryTakeFromAnyvariants with pre-cancelled tokens and available itemsExample
This aligns with standard cancellation semantics where a cancelled token throws immediately regardless of operation feasibility.
Original prompt
This section details on the original issue you should resolve
<issue_title>BlockingCollection.TakeFromAny suggestion</issue_title>
<issue_description>### Description
BlockingCollecction.TakeFromAny do not check state of cancellationToken (3rd argument).
inside of this method :
in call of TryTake(out item)) do not used cancellationToken from method call and inside of TryTake(out item)) used call with empty CancellationToken:
I propose use here :
// Check if the collection is not completed, and potentially has at least one element by checking the semaphore count
if (!collections[i].IsCompleted && collections[i]._occupiedNodes.CurrentCount > 0 && collections[i].TryTake(out item, cancellationToken ))
Reproduction Steps
Expected behavior
Take operation canceled if cancellationToken canceled and any of collection not empty
Actual behavior
Now Take operation do not canceled if cancellationToken canceled and any of collection not empty
Regression?
No response
Known Workarounds
No response
Configuration
No response
Other information
No response</issue_description>
Comments on the Issue (you are @copilot in this section)
@ I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one [area label](https://github.com/dotnet/runtime/blob/master/docs/area-owners.md). @ Tagging subscribers to this area: @dotnet/area-system-collections See info in [area-owners.md](https://github.com/dotnet/runtime/blob/main/docs/area-owners.md) if you want to be subscribed.Issue Details
Description
BlockingCollecction.TakeFromAny do not check state of cancellationToken (3rd argument).
inside of this method :
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.