-
Notifications
You must be signed in to change notification settings - Fork 4.6k
client: Roll-forward PR #8278(with changes): Restore the existing behavior to return io.EOF on repeated RecvMsg() calls for client-streaming RPCs #8523
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8523 +/- ##
==========================================
+ Coverage 81.82% 82.06% +0.24%
==========================================
Files 413 412 -1
Lines 40518 40462 -56
==========================================
+ Hits 33153 33205 +52
+ Misses 5989 5888 -101
+ Partials 1376 1369 -7
🚀 New features to boost your workflow:
|
Can you please keep the revert commit as part of the PR? This will make it easier to review only the new changes. Also, could you please update the PR title to accurately reflect what is being changed? Since this is an impure rollforward, the current title is a bit misleading. You can mention that it's rolling forward the reverted changes in the PR description. Finally, please add release notes for these changes. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor grammar nit otherwise LGTM
test/end2end_test.go
Outdated
|
||
// Tests that a client receives a cardinality violation error for client-streaming | ||
// RPCs if the server call SendMsg multiple times. | ||
// RPCs if the server call SendMsg() multiple times. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// RPCs if the server call SendMsg() multiple times. | |
// RPCs if the server calls SendMsg() multiple times. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a minor comment.
stream.go
Outdated
|
||
sentLast bool // sent an end stream | ||
|
||
recvFirstMsg bool // set after the first message is received |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Please expand recv
to received
. The abbreviation is a bit ambiguous and could be interpreted as either receive
or received
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
… behavior to return io.EOF on repeated RecvMsg() calls for client-streaming RPCs (grpc#8523) Partially addresses: grpc#7286 This reverts commit grpc@20bd1e7 Changes: - Modifies client.RecvMsg() so that successive calls after stream ends return io.EOF. - Adds extra state to track calls to client.recvmsg(required to return Cardinality Violation only in case zero response) RELEASE NOTES: * client: Return status code INTERNAL when a server sends 0 response messages for a unary or client streaming RPC.
Partially addresses: #7286
This reverts commit 20bd1e7
Changes:
RELEASE NOTES: