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.48%. Comparing base (20bd1e7) to head (29f6657).
⚠️ Report is 38 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8385      +/-   ##
==========================================
+ Coverage   82.44%   82.48%   +0.03%     
==========================================
  Files         413      413              
  Lines       40424    40532     +108     
==========================================
+ Hits        33328    33431     +103     
  Misses       5742     5742              
- Partials     1354     1359       +5

Files with missing lines	Coverage Δ
server.go	`82.23% <100.00%> (+0.16%)`	⬆️
stream.go	`81.68% <100.00%> (+0.02%)`	⬆️

... and 39 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-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")

test/end2end_test.go

stream.go

dfawley · 2025-07-28T19:47:01Z

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")


I don't see it.

dfawley · 2025-07-28T19:52:06Z

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.


Is this actually needed? Why? If the client were to attempt to send its second message immediately, and that caused a RST_STREAM, that would all happen after the server processed the headers, and the handler should get invoked regardless, right?

The first call to server.RecvMsg is made by the generated handler before the test's server handler is invoked. If this call fails, the server handler in the test will not be executed at all.

To handle this, I synchronise the calls in the following way:

Client.SendMsg()

Server.RecvMsg()

Client.SendMsg() – Expected to fail with an Internal error and close connection with RST_STREAM.

Server.SendMsg() – Expected to fail with a Canceled error

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().

How does that help? The first Server.RecvMsg should be hung waiting for the client to send an END_STREAM, shouldn't it?

No, it will not hung.
For non-client-streaming RPCs, initial call to client.SendMsg will close the client stream with EOF.

grpc-go/stream.go

Line 1102 in ac13172

if err := a.transportStream.Write(hdr, payld, &transport.WriteOptions{Last: !cs.desc.ClientStreams}); err != nil {

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?

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().

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.

test/end2end_test.go

dfawley · 2025-07-28T19:56:40Z

test/end2end_test.go

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


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

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?

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

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.

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

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!

dfawley · 2025-07-31T22:45:19Z

test/end2end_test.go

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


This is testing client-side checks. We should also have an equivalent test for server-side checks of the same condition (which does not exist). We do have tests for logic on the server for server-streaming RPCs, but not for unary. Unary handling goes through a different code path.

We can add that as a follow-on PR.

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)

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 requested a review from 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

resolving comments

1ff8868

Pranjali-2501 requested a review from dfawley July 28, 2025 18:44

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

dfawley reviewed Jul 28, 2025

View reviewed changes

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

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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!

dfawley Jul 29, 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!

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 31, 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!

dfawley left a comment

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

dfawley Jul 29, 2025 •

edited

Loading

Pranjali-2501 Jul 31, 2025 •

edited

Loading