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

Fixed AsyncStreamReaderExtensions.ReadAllAsync argument validation #909

Merged
merged 1 commit into from
May 15, 2020

Conversation

gfoidl
Copy link
Contributor

@gfoidl gfoidl commented May 15, 2020

The check for streamReader == null was performed on iterating. With this change the check is performed when getting the IAsyncEnumerable (eager), i.e. when ReadAllAsync is called.

It's an extension method, so it's rather unlikely to be called as null.ReadAllAsync(), but to be on the safe side an fail early I believe this should get in 😉

@thelinuxfoundation
Copy link
Collaborator

Thank you for your pull request. Before we can look at your contribution, we need to ensure all contributors are covered by a Contributor License Agreement.

After the following items are addressed, please respond with a new comment here, and the automated system will re-verify.

Regards,
CLA GitHub bot

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 15, 2020

CLA Check
The committers are authorized under a signed CLA.

@JamesNK
Copy link
Member

JamesNK commented May 15, 2020

Moving the EnumeratorCancellation is interesting. This still works:

var cts = new CancellationTokenSource();

await foreach (var item in call.ResponseStream.ReadAllAsync()
                                              .WithCancellation(cts.Token))
{
    messages.Add(item.Message);

    cts.Cancel();
}

What is suppose to happen if someone specifies both?

var cts1 = new CancellationTokenSource();
var cts2 = new CancellationTokenSource();

await foreach (var item in call.ResponseStream.ReadAllAsync(cts1.Token)
                                              .WithCancellation(cts2.Token))
{
    messages.Add(item.Message);

    cts1.Cancel();
}

@JamesNK JamesNK merged commit 32df65a into grpc:master May 15, 2020
@JamesNK
Copy link
Member

JamesNK commented May 15, 2020

I tested and it is fine. The state machine honors both.

@gfoidl gfoidl deleted the readallasync branch May 16, 2020 17:26
@gfoidl
Copy link
Contributor Author

gfoidl commented May 16, 2020

The C#-compiler guys did a brilliant job 👍
Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants