grpc: Fix cardinality violations in non-client streaming RPCs. #8385

Pranjali-2501 · 2025-06-06T18:03:28Z

Partially addresses: #7286

In non-client streaming RPCs, the client's SendMsg() method is designed to automatically close the send operation after its initial call. If someone attempts to call Client.SendMsg() twice for non-client streaming RPCs, if will return with error Internal desc = SendMsg called after CloseSend.
To mirror this behavior, the server-side logic has been updated so that calling RecvMsg() more than once for non-client streaming RPCs will now similarly return an Internal error.

RELEASE NOTES:

grpc: return status code INTERNAL when client send more than one request in unary and server streaming RPC.

codecov · 2025-06-06T18:30:25Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.34%. Comparing base (20bd1e7) to head (1b1800d).
⚠️ Report is 40 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8385      +/-   ##
==========================================
- Coverage   82.44%   82.34%   -0.10%     
==========================================
  Files         413      413              
  Lines       40424    40532     +108     
==========================================
+ Hits        33328    33377      +49     
- Misses       5742     5787      +45     
- Partials     1354     1368      +14

Files with missing lines	Coverage Δ
server.go	`82.00% <100.00%> (-0.07%)`	⬇️
stream.go	`81.68% <100.00%> (+0.02%)`	⬆️

... and 48 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

dfawley

Is it possible to implement all of this in server.go to avoid adding state to the serverStream?

Pranjali-2501 · 2025-06-11T06:38:13Z

Is it possible to implement all of this in server.go to avoid adding state to the serverStream?

As discussed this with @arjan-bal offline, Cardinality violations can only be detected when messages are being read from the stream. This reading process occurs specifically within the server.RecvMsg() function. Since RecvMsg() is invoked from the user-implemented handler, it's not possible to detect cardinality violations during the initial stream setup phase.

stream.go

dfawley · 2025-07-29T15:25:03Z

stream.go

@@ -1820,7 +1820,7 @@ func (ss *serverStream) RecvMsg(m any) (err error) {
 	} else if err != nil {
 		return err
 	}
-	return status.Error(codes.Internal, "cardinality violation: expected <EOF> for non client-streaming RPCs, but received another message")


Non-client-streaming was right before, wasn't it?

Right, didn't realise while copying it.
Made the changes.

dfawley · 2025-07-29T15:26:26Z

test/end2end_test.go

+	desc := &grpc.StreamDesc{
+		StreamName:    "StreamingOutputCall",
+		ServerStreams: true,
+		ClientStreams: false,


Correct. This exact test, pretty much, but set this field to true.

dfawley

LGTM except the few tiny things. Thanks!

test/end2end_test.go

dfawley · 2025-07-31T22:47:52Z

test/end2end_test.go

+// 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.


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.

dfawley · 2025-07-31T22:50:30Z

test/end2end_test.go

+
+	ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
+	defer cancel()
+	cc, err := grpc.NewClient(ss.Address, grpc.WithTransportCredentials(insecure.NewCredentials()))


Can you switch all these tests to use local credentials instead of insecure?

dfawley · 2025-07-31T22:51:48Z

test/end2end_test.go

+
+// Tests the behavior of client for server-side streaming RPC when client sends zero request messages.
+func (s) TestServerStreaming_ClientSendsZeroRequests(t *testing.T) {
+	t.Skip()


Please add a string here and include the issue number (#7286)

Please use the t.Skip string instead of adding a comment.

dfawley · 2025-08-04T15:36:46Z

test/end2end_test.go

+// 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.


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.

dfawley · 2025-08-04T15:37:11Z

test/end2end_test.go

+
+// Tests the behavior of client for server-side streaming RPC when client sends zero request messages.
+func (s) TestServerStreaming_ClientSendsZeroRequests(t *testing.T) {
+	t.Skip()


Please use the t.Skip string instead of adding a comment.

dfawley · 2025-08-04T18:19:15Z

test/end2end_test.go

-// and validating expected error codes.
+// 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.


What I was hoping for was more like this:

Suggested change

// 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.

Pranjali-2501 and others added 2 commits June 6, 2025 15:24

handled cardinality violation and added tests

298edba

Merge branch 'master' into server_streaming

4c3a18a

Pranjali-2501 added the Type: Bug label Jun 6, 2025

Pranjali-2501 added this to the 1.74 Release milestone Jun 6, 2025

remove vet errors

fd5d614

modified tests

7e4206c

Pranjali-2501 requested review from dfawley and arjan-bal June 6, 2025 18:58

Pranjali-2501 assigned Pranjali-2501, dfawley and arjan-bal and unassigned Pranjali-2501 Jun 6, 2025

modified tests

324566b

dfawley removed their assignment Jun 6, 2025

dfawley removed their request for review June 6, 2025 21:16

dfawley reviewed Jun 6, 2025

View reviewed changes

Pranjali-2501 changed the title ~~grpc: Fix cardinality violations in server streaming RPC.~~ grpc: Fix cardinality violations in non-client streaming RPCs. Jun 11, 2025

Pranjali-2501 requested a review from easwars June 11, 2025 06:38

Pranjali-2501 assigned easwars Jun 11, 2025

Pranjali-2501 requested a review from dfawley June 12, 2025 04:37

arjan-bal reviewed Jun 17, 2025

View reviewed changes

stream.go Outdated Show resolved Hide resolved

arjan-bal assigned Pranjali-2501 and unassigned arjan-bal Jun 17, 2025

Pranjali-2501 requested a review from arjan-bal June 17, 2025 15:52

Pranjali-2501 assigned arjan-bal and unassigned Pranjali-2501 Jun 17, 2025

arjan-bal assigned Pranjali-2501 and unassigned arjan-bal Jun 18, 2025

dfawley assigned Pranjali-2501 and unassigned dfawley Jul 28, 2025

Pranjali-2501 added 2 commits July 28, 2025 20:16

update comment

68fd0f8

nits

19e9e71

Pranjali-2501 requested a review from dfawley July 29, 2025 11:24

Pranjali-2501 assigned dfawley and unassigned Pranjali-2501 Jul 29, 2025

dfawley reviewed Jul 29, 2025

View reviewed changes

dfawley assigned Pranjali-2501 and unassigned dfawley Jul 30, 2025

Pranjali-2501 added 2 commits July 30, 2025 19:28

modifying test

0d30b18

resolving vet

83fd598

Pranjali-2501 requested a review from dfawley July 30, 2025 19:39

Pranjali-2501 assigned dfawley and unassigned Pranjali-2501 Jul 30, 2025

remove comment

efa8e5a

dfawley assigned Pranjali-2501 and unassigned dfawley Jul 31, 2025

modifying tests

29f6657

dfawley approved these changes Jul 31, 2025

View reviewed changes

resolving comments

183b1da

Pranjali-2501 requested a review from dfawley August 4, 2025 09:34

dfawley approved these changes Aug 4, 2025

View reviewed changes

resolving comments

749a52c

Pranjali-2501 requested a review from dfawley August 4, 2025 18:10

dfawley reviewed Aug 4, 2025

View reviewed changes

resolving comments

1b1800d

Pranjali-2501 requested a review from dfawley August 4, 2025 18:49

dfawley approved these changes Aug 4, 2025

View reviewed changes

Pranjali-2501 merged commit 62ec29f into grpc:master Aug 5, 2025
15 checks passed

-	// 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.

grpc: Fix cardinality violations in non-client streaming RPCs. #8385

grpc: Fix cardinality violations in non-client streaming RPCs. #8385

Uh oh!

Conversation

Pranjali-2501 commented Jun 6, 2025 • edited by arjan-bal Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dfawley left a comment

Choose a reason for hiding this comment

Uh oh!

Pranjali-2501 commented Jun 11, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfawley left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Pranjali-2501 commented Jun 6, 2025 •

edited by arjan-bal

Loading

codecov bot commented Jun 6, 2025 •

edited

Loading