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.35%. Comparing base (20bd1e7) to head (0f5248a).
Report is 34 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8385      +/-   ##
==========================================
- Coverage   82.44%   82.35%   -0.09%     
==========================================
  Files         413      413              
  Lines       40424    40527     +103     
==========================================
+ Hits        33328    33378      +50     
- Misses       5742     5778      +36     
- Partials     1354     1371      +17

Files with missing lines	Coverage Δ
server.go	`81.54% <100.00%> (-0.53%)`	⬇️
stream.go	`81.93% <100.00%> (+0.27%)`	⬆️

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

test/end2end_test.go

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.

test/end2end_test.go

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.

StreamingOutputCall have predefined calls to sendmsg() and recvmsg().
This test is to check the case when client sends no msg.

The server behaviour shouldn't matter for this test, right? When the client sends 0 request messages, the server should send an internal error immediately. Are you seeing that this is not happening with stubserver?

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.

test/end2end_test.go

Pranjali-2501 · 2025-07-08T11:27:44Z

test/end2end_test.go

+	ss := stubserver.StubServer{
+		StreamingOutputCallF: func(_ *testpb.StreamingOutputCallRequest, stream testgrpc.TestService_StreamingOutputCallServer) error {
+			// This is second call to RecvMsg(), the initial call having been performed by the server handler.
+			if err = stream.RecvMsg(&testpb.Empty{}); status.Code(err) != codes.Internal {


@arjan-bal , as mentioned in the comment I'm using stubserver here.
But StreamingOutputCall handler inherently calls RecvMsg() and second call to RecvMsg() in StreamingOutputCallF cause a race to happen.
https://github.com/grpc/grpc-go/actions/runs/16136673126/job/45534355675?pr=8385

Should I revert it back to previous implementation and register a new service on the server to test?

The race is in the writes performed on the err variable declared at line 3746. We should declare a new err variable inside the goroutine.

dfawley · 2025-07-18T20:52:30Z

stream.go

+	} else if err != nil {
+		return err
+	}
+	return status.Error(codes.Internal, "cardinality violation: expected <EOF> for non client-streaming RPCs, but received another message")


How about something more like the first message:

Suggested change

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

return status.Error(codes.Internal, "cardinality violation: received multiple request messages for non-client-streaming RPC")

I'm trying to keep it consistent with client.RecvMsg().

grpc-go/stream.go

Lines 1180 to 1181 in a5e7cd6

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

}

Let me know if multiple request messages looks better, I'll change it.

These error messages go out on the wire. To clients written in any language. So they should generically explain the problem and not use Go-specific terminology, or internal implementation details like "expected EOF".

That's probably not appropriate on the client side, either, but slightly less bad. But since you're changing things, that one should probably look like:

return status.Errorf(codes.Internal, "cardinality violation: received multiple response messages for non-server-streaming RPC")

dfawley · 2025-07-18T20:52:45Z

stream.go

@@ -1774,6 +1775,9 @@ func (ss *serverStream) RecvMsg(m any) (err error) {
 					binlog.Log(ss.ctx, chc)
 				}
 			}
+			if !ss.desc.ClientStreams {
+				return status.Error(codes.Internal, "cardinality violation: received no request message from non-client-stream RPC")


Nit:

Suggested change

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

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

dfawley · 2025-07-18T20:56:21Z

stream.go

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


Is this a behavior change? Users that call RecvMsg would previously get io.EOF and now they'll get a cardinality violation?

Yes, there is a behaviour change.

For non-client-streaming RPCs, it will return internal error for the following 2 cases.

When there is 0 request message from Client.

When Server call RecvMsg() twice.

I guess the second behavior change is only possible if you are using the generic API, so wouldn't affect 99% of our users. But I still don't think this is a change we want to make. I would probably be in favor of it if we were not 1.0, but we need to keep backward compatibility unless there's enough justification, which doesn't seem to be the case here.

As discussed offline, I have added an additional state in serverstream to track the first call to server.recvMsg().

Based on that server will only return Internal error when there is 0 request message from Client.

dfawley · 2025-07-18T20:58:43Z

test/end2end_test.go

+	if err := stream.SendMsg(&testpb.Empty{}); status.Code(err) != codes.Internal {
+		t.Errorf("stream.SendMsg() = %v, want error %v", status.Code(err), codes.Internal)
+	}


Can we confirm that this cancels the RPC, too, as observed from the server? If the stream ends on the client side it must perform a RST_STREAM with the server.

No, it did not do a RST_STREAM.
Following code causes this.

grpc-go/stream.go

Lines 911 to 917 in a5e7cd6

if cs.sentLast {

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

}

if !cs.desc.ClientStreams {

cs.sentLast = true

}

Did you confirm a RST_STREAM doesn't happen?

Because tracing through the code...that error results in a call to cs.finish which will call csAttempt.finish which calls transportStream.Close. That should hopefully result in a RST_STREAM.

You’re right — I traced it again and confirmed that the RST_STREAM does happen. The flow is:
cs.finish → csAttempt.finish → transportStream.Close → http2Client.closeStream → controlBuf.executeAndPut → cleanupStream.isTransportResponseFrame().

Great. Can we update the test to confirm the stream is getting cancelled on the server side? I think you could just make it do send operations and eventually see it error?

arjan-bal · 2025-07-22T10:12:29Z

test/end2end_test.go

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


The server behaviour shouldn't matter for this test, right? When the client sends 0 request messages, the server should send an internal error immediately. Are you seeing that this is not happening with stubserver?

arjan-bal · 2025-07-22T10:16:12Z

test/end2end_test.go

+
+	ss := stubserver.StubServer{
+		StreamingOutputCallF: func(_ *testpb.StreamingOutputCallRequest, stream testgrpc.TestService_StreamingOutputCallServer) error {
+			// This is second call to RecvMsg(), the initial call having been performed by the server handler.


the initial call having been performed by the server handler.

nit: I think a clearer way to mention the reason is that the the initial call to receive the request is made by the generated code.

Test is no longer required.

arjan-bal · 2025-07-22T10:21:50Z

test/end2end_test.go

+	desc := &grpc.StreamDesc{
+		StreamName:    "UnaryCall",
+		ServerStreams: false,
+		ClientStreams: false,
+	}


Since the test is for a misbehaving server, can we use a real test service client instead of manipulating StreamDescs?

Test is no longer required.

arjan-bal · 2025-07-22T10:25:43Z

test/end2end_test.go

+	}
+	defer lis.Close()
+
+	s := grpc.NewServer()


Similar to previous comments, can we use a stubserver if we don't need the server to misbehave? Also applicable to TestUnaryRPC_ClientCallSendMsgTwice.

dfawley · 2025-07-25T20:17:09Z

stream.go

@@ -1588,6 +1589,8 @@ type serverStream struct {

 	sendCompressorName string

+	recvFirstMsg bool // recv frist msg from client


This comment doesn't really say anything. How about:

Suggested change

recvFirstMsg bool // recv frist msg from client

recvFirstMsg bool // set after the first message is received

dfawley · 2025-07-25T20:21:31Z

stream.go

+	} else if err != nil {
+		return err
+	}
+	return status.Error(codes.Internal, "cardinality violation: expected <EOF> for non client-streaming RPCs, but received another message")


These error messages go out on the wire. To clients written in any language. So they should generically explain the problem and not use Go-specific terminology, or internal implementation details like "expected EOF".

That's probably not appropriate on the client side, either, but slightly less bad. But since you're changing things, that one should probably look like:

return status.Errorf(codes.Internal, "cardinality violation: received multiple response messages for non-server-streaming RPC")

dfawley · 2025-07-25T20:25:50Z

test/end2end_test.go

+	if err := stream.SendMsg(&testpb.Empty{}); status.Code(err) != codes.Internal {
+		t.Errorf("stream.SendMsg() = %v, want error %v", status.Code(err), codes.Internal)
+	}


Great. Can we update the test to confirm the stream is getting cancelled on the server side? I think you could just make it do send operations and eventually see it error?

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

Pranjali-2501 assigned arjan-bal and unassigned 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

Pranjali-2501 added 3 commits July 8, 2025 07:04

addressed comments

f4f1b61

resolving vets

83e8664

resolving vets

5acd9ab

Pranjali-2501 requested a review from arjan-bal July 8, 2025 08:30

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

Pranjali-2501 commented Jul 8, 2025

View reviewed changes

Pranjali-2501 added 2 commits July 15, 2025 09:16

minor change

8f4f39b

minor change

6de922d

dfawley reviewed Jul 18, 2025

View reviewed changes

minor change

5f6c715

arjan-bal reviewed Jul 22, 2025

View reviewed changes

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

Pranjali-2501 added 4 commits July 24, 2025 08:34

resolving comments

990efe1

resolving nits

7f6d31f

vet changes

41d8328

added comment

59ad122

Pranjali-2501 removed their assignment Jul 25, 2025

Pranjali-2501 requested review from dfawley and arjan-bal July 25, 2025 10:54

added comment

0f5248a

dfawley reviewed Jul 25, 2025

View reviewed changes

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

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

	return status.Errorf(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
	}

		@@ -1588,6 +1589,8 @@ type serverStream struct {

		sendCompressorName string

		recvFirstMsg bool // recv frist msg from client

	recvFirstMsg bool // recv frist msg from client
	recvFirstMsg bool // set after the first message is received

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

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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Pranjali-2501 Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Pranjali-2501 Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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

Pranjali-2501 commented Jun 6, 2025 •

edited by arjan-bal

Loading

codecov bot commented Jun 6, 2025 •

edited

Loading

Pranjali-2501 Jul 8, 2025 •

edited

Loading

Pranjali-2501 Jul 22, 2025 •

edited

Loading