Cardinality violations in Client streaming RPC. #8278
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8278 +/- ##
==========================================
- Coverage 82.29% 82.25% -0.05%
==========================================
Files 417 417
Lines 41356 41362 +6
==========================================
- Hits 34035 34022 -13
- Misses 5907 5922 +15
- Partials 1414 1418 +4
🚀 New features to boost your workflow:
|
| @@ -542,6 +542,8 @@ type clientStream struct { | |||
|
|
|||
| sentLast bool // sent an end stream | |||
|
|
|||
| recvMsg bool // received msg from server | |||
There was a problem hiding this comment.
nit: Boolean field names should be like questions with yes/no answers, example "didReceiveFirstMessage".
| @@ -1134,11 +1136,17 @@ func (a *csAttempt) recvMsg(m any, payInfo *payloadInfo) (err error) { | |||
| if statusErr := a.transportStream.Status().Err(); statusErr != nil { | |||
| return statusErr | |||
| } | |||
| if cs.desc.ClientStreams && !cs.desc.ServerStreams && !cs.recvMsg{ | |||
There was a problem hiding this comment.
Can we leave out cs.desc.ClientStreams from this check? I would assume that the server needs to send a message for unary RPCs also. Are unary RPCs handled by this code path? If yes, we should add a test for unary RPCs also.
| return io.EOF // indicates successful end of stream. | ||
| } | ||
|
|
||
| return toRPCErr(err) | ||
| } | ||
| if cs.desc.ClientStreams { | ||
| cs.recvMsg = true |
There was a problem hiding this comment.
Can we set this boolean without the check? Otherwise we would need to document the variable in a way to make it obvious that this is set only for client streaming RPCs. Even then users may accidentally use it for other types of RPCs.
| @@ -1134,11 +1136,17 @@ func (a *csAttempt) recvMsg(m any, payInfo *payloadInfo) (err error) { | |||
| if statusErr := a.transportStream.Status().Err(); statusErr != nil { | |||
| return statusErr | |||
| } | |||
| if cs.desc.ClientStreams && !cs.desc.ServerStreams && !cs.recvMsg{ | |||
There was a problem hiding this comment.
The vet checks is failing, probably because of the missing space before {. Can you please fix the vet issues?
|
Do we need a similar check on the server side also, i.e. change the status to internal if the server attempts to close without writing a message? |
Client should ensure an "Internal" error is returned for client-streaming RPCs if the server doesn't send a message before returning status OK.
Currently it is returning "EOF", which should not be the case for cardinality violations.