Skip to content

Pass cancellation token to Dequeue method, and rewrite comment#51794

Merged
davidwengier merged 1 commit intodotnet:mainfrom
davidwengier:DontReportCancellations
Mar 11, 2021
Merged

Pass cancellation token to Dequeue method, and rewrite comment#51794
davidwengier merged 1 commit intodotnet:mainfrom
davidwengier:DontReportCancellations

Conversation

@davidwengier
Copy link
Member

@davidwengier davidwengier commented Mar 11, 2021

If our server got shut down by the client, and we happened to be waiting to dequeue the next item from the queue, we would log it as an error because it came from an unknown cancellation token. This correctly links everything up.

Then I updated the comment because who understands Past Dave™

@davidwengier davidwengier requested a review from dibarbet March 11, 2021 02:39
@davidwengier davidwengier requested a review from a team as a code owner March 11, 2021 02:39
@ghost ghost added the Area-IDE label Mar 11, 2021
sharwell
sharwell approved these changes Mar 11, 2021
View reviewed changes
Copy link
Contributor

@sharwell sharwell left a comment

Choose a reason for hiding this comment

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

I'm assuming we are sure DequeueAsync processes the _cancelSource cancellation before whatever other cancellation was involved to lead to the log.

sharwell
sharwell reviewed Mar 11, 2021
View reviewed changes
src/Features/LanguageServer/Protocol/Handler/RequestExecutionQueue.cs
@@ -230,8 +230,10 @@ private async Task ProcessQueueAsync()
}
catch (OperationCanceledException e) when (e.CancellationToken == _cancelSource.Token)
Copy link
Contributor

@sharwell sharwell Mar 11, 2021

Choose a reason for hiding this comment

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

💡 The other way to do this is:

Suggested change
catch (OperationCanceledException e) when (e.CancellationToken == _cancelSource.Token)
catch (OperationCanceledException) when (_cancelSource.IsCancellationRequested)

@davidwengier
Copy link
Member Author

davidwengier commented Mar 11, 2021

I'm assuming we are sure DequeueAsync processes the _cancelSource cancellation before whatever other cancellation was involved to lead to the log.

I'm not sure exactly what you mean, but there is no "other" cancellation. The log happened because of the call to _queue.Complete() cancels the pending DequeueAsync call, but without a cancellation token in the exception. Or at least not the one we were checking for.

@sharwell
Copy link
Contributor

sharwell commented Mar 11, 2021 •
edited
Loading

I'm not sure exactly what you mean, but there is no "other" cancellation.

The call to _queue.Complete() calls TrySetCanceled() on any remaining DequeueAsync waiters. It's important to ensure that all of those waiters have called TrySetCanceled(token) prior to the call to Complete() so the right cancellation token makes it all the way through to the location where you checked it.

Looking at the code, I believe this is correctly handled right now.

@davidwengier davidwengier merged commit 27913e0 into dotnet:main Mar 11, 2021
@davidwengier davidwengier deleted the DontReportCancellations branch March 11, 2021 22:24
@allisonchou allisonchou added this to the Next milestone Mar 12, 2021
@allisonchou allisonchou modified the milestones: Next, 16.10.P2 Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dibarbet dibarbet Awaiting requested review from dibarbet

1 more reviewer

@sharwell sharwell sharwell approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area-IDE

Projects

None yet

Milestone

16.10.P2

Development

Successfully merging this pull request may close these issues.

3 participants

@davidwengier @sharwell @allisonchou