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

Attention: Patch coverage is 78.57143% with 3 lines in your changes missing coverage. Please review.

Project coverage is 82.43%. Comparing base (20bd1e7) to head (dff08b3).
Report is 6 commits behind head on master.

Files with missing lines	Patch %	Lines
stream.go	76.92%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8385      +/-   ##
==========================================
- Coverage   82.44%   82.43%   -0.01%     
==========================================
  Files         413      414       +1     
  Lines       40424    40437      +13     
==========================================
+ Hits        33328    33336       +8     
- Misses       5742     5743       +1     
- Partials     1354     1358       +4

Files with missing lines	Coverage Δ
server.go	`82.08% <100.00%> (+0.01%)`	⬆️
stream.go	`82.09% <76.92%> (+0.43%)`	⬆️

... and 22 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.

arjan-bal · 2025-06-17T06:33:20Z

stream.go

@@ -1774,6 +1775,9 @@ func (ss *serverStream) RecvMsg(m any) (err error) {
 					binlog.Log(ss.ctx, chc)
 				}
 			}
+			if !ss.desc.ClientStreams {


It's not immediately clear to me how this conditions indicates RecvMsg is called twice. From my understanding, the handling of cardinality violations in the server stream should be similar to the one in client stream. The check here should be for 0 requests being received for a non-client-streaming request.

grpc-go/stream.go

Lines 1142 to 1144 in e5de1e2

if !cs.desc.ServerStreams {

return status.Error(codes.Internal, "cardinality violation: received no response message from non-streaming RPC")

}

For > 1 requests, there should be a block near the end of this method similar to the following:

grpc-go/stream.go

Lines 1167 to 1178 in e5de1e2

if cs.desc.ServerStreams {

// Subsequent messages should be received by subsequent RecvMsg calls.

return nil

}

// Special handling for non-server-stream rpcs.

// This recv expects EOF or errors, so we don't collect inPayload.

if err := recv(a.parser, cs.codec, a.transportStream, a.decompressorV0, m, *cs.callInfo.maxReceiveMessageSize, nil, a.decompressorV1, false); err == io.EOF {

return a.transportStream.Status().Err() // non-server streaming Recv returns nil on success

} else if err != nil {

return toRPCErr(err)

}

return status.Error(codes.Internal, "cardinality violation: expected <EOF> for non server-streaming RPCs, but received another message")

Is there any issue if we try to follow the same pattern as client streams here?

In server-streaming, client can't send more than one message. So the following code will be useless for server.recvmsg().

grpc-go/stream.go

Lines 1167 to 1178 in e5de1e2

if cs.desc.ServerStreams {

// Subsequent messages should be received by subsequent RecvMsg calls.

return nil

}

// Special handling for non-server-stream rpcs.

// This recv expects EOF or errors, so we don't collect inPayload.

if err := recv(a.parser, cs.codec, a.transportStream, a.decompressorV0, m, *cs.callInfo.maxReceiveMessageSize, nil, a.decompressorV1, false); err == io.EOF {

return a.transportStream.Status().Err() // non-server streaming Recv returns nil on success

} else if err != nil {

return toRPCErr(err)

}

return status.Error(codes.Internal, "cardinality violation: expected <EOF> for non server-streaming RPCs, but received another message")

The check here should be for 0 requests being received for a non-client-streaming request.

grpc-go/stream.go

Lines 1142 to 1144 in e5de1e2

if !cs.desc.ServerStreams {

return status.Error(codes.Internal, "cardinality violation: received no response message from non-streaming RPC")

}

Right, in client.recvMsg() this check is triggered in case of 0 response from server.
But in server-stream, it is not possible to have 0 request. So the following case will only be triggered when server.recvmsg() is called more than once and receives an io.EOF.

In server-streaming, client can't send more than one message.

What is stopping a client from doing this?

Clientstream maintains a variable named sentlast which will be set to true when client sends a msg in case of non client-streaming RPCs.

grpc-go/stream.go

Lines 905 to 910 in e5de1e2

if cs.sentLast {

return status.Errorf(codes.Internal, "SendMsg called after CloseSend")

}

if !cs.desc.ClientStreams {

cs.sentLast = true

}

As we discussed earlier, can we have the client (incorrectly) create a bidi stream by manipulating the strem desc?

Ideally, a well behaving client should never cause cardinality violations. The reason for adding these checks is to fail the RPCs with good error codes and messages when a client misbehaves.

Added the logic in server.recvmsg() to call recv() twice in case of non-client-stream RPCs. Also added a test with misbehaving client.

stream.go

test/end2end_test.go

arjan-bal · 2025-06-24T18:54:34Z

test/end2end_test.go

+// Tests the behavior for unary RPC when server call
+// RecvMsg twice. Second call to RecvMsg should fail with Internal
+// error.
+func (s) TestUnaryRPC_ServerCallRecvMsgTwice(t *testing.T) {


Can this be combined with TestServerStreaming_ServerCallRecvMsgTwice into a single table drive test? They seem to be exactly the same except for a couple of lines. Similarly for the ClientCallSendMsgTwice tests.

Bump on this.

arjan-bal · 2025-06-24T19:03:31Z

test/end2end_test.go

+	}
+}
+
+func (s) TestServerStreaming_ClientBehaveAsBidiStreaming(t *testing.T) {


I think a better suffix is ClientSendsMultipleMessages because we want to test the case when a client sends multiple messages. Making the client bi-di is just a way to bypass the client side checks that stop a non-streaming client from sending multiple messages.

Can you please add a doc comment describing the test?

Already have a test with suffix ClientCallSendMsgTwice() to test what happen when a correctly configured client for server-side streaming attempts to call SendMsg() twice.

I'll add a comment to describe what this test is doing.

ClientSendsMultipleMessages is not the same as ClientCallSendMsgTwice. The former implies that the client is actually sending two messages, whereas the latter indicates that the client is calling the SendMsg method twice.

arjan-bal · 2025-06-24T19:04:48Z

test/end2end_test.go

+	}
+	defer cc.Close()
+
+	desc := &grpc.StreamDesc{


Please add a comment describing the need to use a bi-di streaming client for this test.

I don't see the comment here.

test/end2end_test.go

arjan-bal · 2025-07-04T11:02:28Z

test/end2end_test.go

+	ss := grpc.UnknownServiceHandler(func(any, grpc.ServerStream) error {
+		return nil
+	})


We should use real implementations in tests as much as possible so that the entire RPC stack is used and there are no unintended interactions. By mocking the server, there is a chance that the client side check is removed and the test continues to pass because the fake server doesn't have the required functionality to handle the call.

arjan-bal · 2025-07-04T11:09:21Z

test/end2end_test.go

+	}
+}
+
+func (s) TestServerStreaming_ClientBehaveAsBidiStreaming(t *testing.T) {


ClientSendsMultipleMessages is not the same as ClientCallSendMsgTwice. The former implies that the client is actually sending two messages, whereas the latter indicates that the client is calling the SendMsg method twice.

arjan-bal · 2025-07-04T11:09:51Z

test/end2end_test.go

+	}
+	defer cc.Close()
+
+	desc := &grpc.StreamDesc{


I don't see the comment here.

arjan-bal · 2025-07-04T11:28:59Z

test/end2end_test.go

+	go s.Serve(lis)
+	defer s.Stop()
+
+	// s := grpc.NewServer(ss)


Please remove this commented code.

arjan-bal · 2025-07-04T11:31:04Z

test/end2end_test.go

+	defer lis.Close()
+
+	s := grpc.NewServer()
+	serviceDesc := grpc.ServiceDesc{


I believe we don't need to change the stream descriptors on the server side for this test. If so, please use a stubserver and override one of the handlers.

arjan-bal · 2025-07-04T11:38:06Z

test/end2end_test.go

+	defer lis.Close()
+
+	s := grpc.NewServer()
+	serviceDesc := grpc.ServiceDesc{


Do we need to fake the stream descriptors in this test? If not, we should use the test service client and server.

arjan-bal · 2025-07-04T11:39:17Z

test/end2end_test.go

+// Tests the behavior for unary RPC when server call
+// RecvMsg twice. Second call to RecvMsg should fail with Internal
+// error.
+func (s) TestUnaryRPC_ServerCallRecvMsgTwice(t *testing.T) {


Bump on this.

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

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

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

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

Pranjali-2501 and others added 4 commits June 19, 2025 08:50

Merge branch 'master' into server_streaming

9fbf6b7

change server.recvmsg() to catch cardinality violation

9b9cddf

Merge branch 'master' into server_streaming

63565f2

replace srv with _

67549e7

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

arjan-bal reviewed Jun 24, 2025

View reviewed changes

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

eshitachandwani modified the milestones: 1.74 Release, 1.75 Release Jun 25, 2025

Pranjali-2501 added 2 commits July 1, 2025 07:23

resolving comments

6030b90

resolving vets

dff08b3

Pranjali-2501 requested review from arjan-bal and removed request for easwars July 1, 2025 07:43

Pranjali-2501 assigned dfawley and arjan-bal and unassigned easwars and Pranjali-2501 Jul 1, 2025

arjan-bal reviewed Jul 4, 2025

View reviewed changes

arjan-bal assigned Pranjali-2501 and unassigned arjan-bal Jul 4, 2025

	if !cs.desc.ServerStreams {
	return status.Error(codes.Internal, "cardinality violation: received no response message from non-streaming RPC")
	}

	if cs.desc.ServerStreams {
	// Subsequent messages should be received by subsequent RecvMsg calls.
	return nil
	}
	// Special handling for non-server-stream rpcs.
	// This recv expects EOF or errors, so we don't collect inPayload.
	if err := recv(a.parser, cs.codec, a.transportStream, a.decompressorV0, m, *cs.callInfo.maxReceiveMessageSize, nil, a.decompressorV1, false); err == io.EOF {
	return a.transportStream.Status().Err() // non-server streaming Recv returns nil on success
	} else if err != nil {
	return toRPCErr(err)
	}
	return status.Error(codes.Internal, "cardinality violation: expected <EOF> for non server-streaming RPCs, but received another message")

	if cs.sentLast {
	return status.Errorf(codes.Internal, "SendMsg called after CloseSend")
	}
	if !cs.desc.ClientStreams {
	cs.sentLast = true
	}

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

Are you sure you want to change the base?

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

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!

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!

Uh oh!

Uh oh!

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!

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!

Pranjali-2501 commented Jun 6, 2025 •

edited by arjan-bal

Loading

codecov bot commented Jun 6, 2025 •

edited

Loading