Skip to content

Don't bubble up linked token cancellation to the batching work queue#61739

Merged
CyrusNajmabadi merged 11 commits intodotnet:mainfrom
CyrusNajmabadi:bubbledCancellation
Jun 7, 2022
Merged

Don't bubble up linked token cancellation to the batching work queue#61739
CyrusNajmabadi merged 11 commits intodotnet:mainfrom
CyrusNajmabadi:bubbledCancellation

Conversation

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

We were bubbling out an interior cancellation operation to our async batching queue, causing it to cancel all further work. This violates the contract about how cancellation is supposed to operate (Where callers should only get cancellation on the tokens they pass into methods). This was leading to us having a race where we might cancel nested work, but end up in a canceled state forever.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner June 7, 2022 00:38
@ghost ghost added the Area-IDE label Jun 7, 2022
@CyrusNajmabadi CyrusNajmabadi requested a review from dibarbet June 7, 2022 00:38
@CyrusNajmabadi CyrusNajmabadi enabled auto-merge June 7, 2022 01:04
@CyrusNajmabadi CyrusNajmabadi disabled auto-merge June 7, 2022 01:07
Comment on lines +148 to +149
// Make sure the task being performed doesn't propagate out a cancellation exception that isn't
// associated with the token we pass into it.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would prefer this comment be inside the catch block, with a comment that we don't want an exception thrown by a work item to break the queue.

@CyrusNajmabadi CyrusNajmabadi enabled auto-merge June 7, 2022 01:37
@CyrusNajmabadi CyrusNajmabadi disabled auto-merge June 7, 2022 04:16
@CyrusNajmabadi CyrusNajmabadi enabled auto-merge June 7, 2022 06:12
@CyrusNajmabadi CyrusNajmabadi merged commit 354364c into dotnet:main Jun 7, 2022
@ghost ghost added this to the Next milestone Jun 7, 2022
@CyrusNajmabadi CyrusNajmabadi deleted the bubbledCancellation branch June 7, 2022 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants