Skip to content
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

🐇 Implement AsyncQueue<T>.TryDequeueAsync #43440

Merged
merged 1 commit into from
Apr 25, 2020

Conversation

sharwell
Copy link
Member

This method uses Optional<T> instead of T for the return value of TryDequeueAsync, allowing the method to support normal termination of the queue without throwing cancellation exceptions.

@sharwell sharwell requested a review from a team as a code owner April 17, 2020 06:30
@sharwell sharwell marked this pull request as draft April 17, 2020 07:17
@sharwell sharwell marked this pull request as ready for review April 17, 2020 15:23
@sharwell sharwell force-pushed the try-dequeue-async branch from 4aac73d to d6b23fb Compare April 17, 2020 15:23
@sharwell sharwell changed the title Implement AsyncQueue<T>.TryDequeueAsync 🐇 Implement AsyncQueue<T>.TryDequeueAsync Apr 17, 2020
@sharwell sharwell force-pushed the try-dequeue-async branch 2 times, most recently from 67843f4 to 561f076 Compare April 17, 2020 18:05
{
return completedEvent;
// When the queue is completed with a pending TryDequeueAsync return then
Copy link
Contributor

Choose a reason for hiding this comment

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

When the queue is completed with a pending TryDequeueAsync return then [](start = 35, length = 70)

I find this wording confusing, consider rewording.

Copy link
Member Author

Choose a reason for hiding this comment

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

I amended the previous commit just to reword this. The difference can be seen in this:
https://github.com/dotnet/roslyn/compare/561f07655fc1c829a0948546691867533051739c..2043e01430be991df037cb94d9f1b4b0fd8713e7

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (iteration 1)

This method uses Optional<T> instead of T for the return value of
TryDequeueAsync, allowing the method to support normal termination of
the queue without throwing cancellation exceptions.
@sharwell sharwell force-pushed the try-dequeue-async branch from 561f076 to 2043e01 Compare April 22, 2020 05:25
@sharwell
Copy link
Member Author

@dotnet/roslyn-compiler for a second review

@jaredpar
Copy link
Member

@gafter can you take a look at this?

@gafter gafter self-requested a review April 25, 2020 00:25
// the Optional<T> will not have a value. This signals the queue has reached
// completion and no more items will be added to it.

// This failure is being tracked by https://github.com/dotnet/roslyn/issues/5962
Copy link
Member

Choose a reason for hiding this comment

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

This failure is being tracked by [](start = 35, length = 32)

This comment appears pretty out of date. Can this assertion be uncommented? At the very least we should have an open issue tracking this potential failure.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll submit this as a follow-up

Copy link
Member

@gafter gafter left a comment

Choose a reason for hiding this comment

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

:shipit:

@sharwell sharwell merged commit 28836d1 into dotnet:master Apr 25, 2020
@ghost ghost added this to the Next milestone Apr 25, 2020
@sharwell sharwell deleted the try-dequeue-async branch April 25, 2020 04:04
@sharwell sharwell modified the milestones: Next, temp, 16.7.P1 Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants