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

Fail SEND_MESSAGE ops if stream is closed for writes #19868

Merged
merged 2 commits into from
Aug 10, 2019

Conversation

yashykt
Copy link
Member

@yashykt yashykt commented Aug 6, 2019

Fixes #14812
Fail SEND_MESSAGE ops if stream is closed for writes.

Earlier, gRPC Core would not fail SEND_MESSAGE ops if the client had already received a status.

Looks like this was done so as to avoid status being overwritten by the error 'Attempt to send message after stream was closed'. This would happen because the surface layer (call.cc) initiates a CANCEL op when any op fails.

In the case of a streaming call, the C++ API depends on Write calls returning false when the stream is closed. If gRPC Core fails this write and cancels the call, we would lose the status reported by the server too.

This approach creates a bug where Writes on streaming calls never fail even if the stream has been closed.

New Approach -
If gRPC Core gets a SEND_MESSAGE op when the stream is closed for writes, we fail the write but we do not cancel the call.

@yashykt yashykt added release notes: yes Indicates if PR needs to be in release notes lang/core labels Aug 6, 2019
@yashykt yashykt requested a review from vjpai August 6, 2019 23:26
@yashykt
Copy link
Member Author

yashykt commented Aug 8, 2019

cc @a11r

@yashykt
Copy link
Member Author

yashykt commented Aug 8, 2019

Issues: #19819, #19422, #19170

@yashykt
Copy link
Member Author

yashykt commented Aug 10, 2019

Thanks for reviewing!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lang/core release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ClientAsyncWriter::Write does not return false on CompletionQueue when stream is closed
2 participants