-
Notifications
You must be signed in to change notification settings - Fork 4.6k
grpc: Fix cardinality violations in non-client streaming RPCs. #8385
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
Changes from 29 commits
298edba
4c3a18a
fd5d614
7e4206c
324566b
9fbf6b7
9b9cddf
63565f2
67549e7
6030b90
dff08b3
f4f1b61
83e8664
5acd9ab
8f4f39b
6de922d
5f6c715
990efe1
7f6d31f
41d8328
59ad122
0f5248a
1ff8868
68fd0f8
19e9e71
0d30b18
83fd598
efa8e5a
29f6657
183b1da
749a52c
1b1800d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3740,6 +3740,238 @@ func (s) TestClientStreaming_ReturnErrorAfterSendAndClose(t *testing.T) { | |||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Tests the behavior for server-side streaming when client calls SendMsg twice. | ||||||||||||||
| // Second call to SendMsg should fail with Internal error and result in closing | ||||||||||||||
| // the connection with a RST_STREAM. | ||||||||||||||
| func (s) TestServerStreaming_ClientCallSendMsgTwice(t *testing.T) { | ||||||||||||||
| // To ensure server.recvMsg() is successfully completed. | ||||||||||||||
|
||||||||||||||
| if err := a.transportStream.Write(hdr, payld, &transport.WriteOptions{Last: !cs.desc.ClientStreams}); err != nil { |
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.
Oh yes, since the client knows it's not client-streaming, too, then it will do END_STREAM along with the first call to SendMsg.
So then the reason for the synchronization here is that, if the client does a RST_STREAM, that might propagate to the server and cause its first recv to fail -- even though the first request message was received perfectly normally -- because we cancel the stream's context upon RST_STREAM receipt?
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.
Yes, correct.
So, should we keep the test as it is or change it with alternative approach?
An alternative approach would be to use a fake server implementation, which would allow us to directly observe and assert the error returned from server.RecvMsg().
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.
No, but can you explain more like how I did, to say why the synchronization is needed? Because there would be a race between client cancellation and the server reading the first request message.
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.
Please leave the function-level comment to just a description of the test, and put the explanation of the various steps in intra-function comments.
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.
Outdated
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.
What I was hoping for was more like this:
| // To ensure server.recvMsg() is successfully completed. | |
| // To ensure server.recvMsg() is successfully completed. Otherwise, if the client application | |
| // attempts to send a second request message, that will trigger a RST_STREAM from the | |
| // client due to the application violating the RPC's protocol. The RST_STREAM will prevent | |
| // the method handler from being called. |
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.
dfawley marked this conversation as resolved.
Show resolved
Hide resolved
Outdated
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.
Please add a string here and include the issue number (#7286)
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.
Please use the t.Skip string instead of adding a comment.
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.
Outdated
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.
Can you switch all these tests to use local credentials instead of insecure?
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.
arjan-bal marked this conversation as resolved.
Show resolved
Hide resolved
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.
It seems like we should have a test case that sets this true, also, and ensures that the server errors? This is theoretically catching a client-side check that errors if zero requests are sent before CloseSend is called when the client knows it is not client-streaming.
(And if we don't have such a check we may want to add one.)
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.
Just to clarify — you're suggesting me to add a test where the client behaves as client/bidi-streaming and sends zero request, while server behave as server-streaming, and then assert that it fails on the server side due to a cardinality violation. Is that correct?
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.
Correct. This exact test, pretty much, but set this field to true.
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.
I have modified the test to a table-driven test, where server will run against multiple streamdesc including client-streaming, server-streaming and bidi-streaming.
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.
The key difference between the two cases that I'd like to see is whether the client knows it's required to send a message.
In the case where the client knows (ClientStreams: false), we should detect the error locally and send a RST_STREAM to the server, but if the client doesn't know (ClientStreams: true), the server should detect the error and end the stream with an INTERNAL error. Can we confirm these things are happening? (And AFACT the test will fail since the client doesn't check whether it has sent a message in CloseSend, so it's fine to make that test case Skip until we fix it in another PR.)
Uh oh!
There was an error while loading. Please reload this page.