grpc: Fix cardinality violations in client streaming and unary RPCs. #8330

Pranjali-2501 · 2025-05-15T17:46:16Z

Client should ensure an INTERNAL error is returned for non-server streaming RPCs if the server attempts to send more than one response.
Currently it is returning UNKNOWN, which should not be the case for cardinality violations.

Filed in #7286
RELEASE NOTES:

grpc: unary and client-streaming RPCs fail with internal status when server sends multiple response messages.

codecov · 2025-05-15T17:51:17Z

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 82.20%. Comparing base (6821606) to head (0441021).
Report is 41 commits behind head on master.

Files with missing lines	Patch %	Lines
stream.go	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8330      +/-   ##
==========================================
- Coverage   82.25%   82.20%   -0.05%     
==========================================
  Files         417      419       +2     
  Lines       41356    42039     +683     
==========================================
+ Hits        34017    34560     +543     
- Misses       5923     6008      +85     
- Partials     1416     1471      +55

Files with missing lines	Coverage Δ
stream.go	`81.91% <50.00%> (+0.35%)`	⬆️

... and 57 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 · 2025-05-16T16:24:04Z

@Pranjali-2501 please use the Assignees field to indicate who needs to act on the PR next. I assume you have meant to assign this to @arjan-bal and me?

dfawley

Looks great, just a couple really small things. Thanks!

dfawley · 2025-05-20T22:11:14Z

test/end2end_test.go

@@ -3739,6 +3739,39 @@ func (s) TestClientStreaming_ReturnErrorAfterSendAndClose(t *testing.T) {
 	}
 }

+// Tests that a client receives a cardinality violation error for client-streaming
+// RPCs if the server call SendAndClose after calling SendAndClose.


Suggested change

// RPCs if the server call SendAndClose after calling SendAndClose.

// RPCs if the server calls SendAndClose multiple times.

dfawley · 2025-05-20T22:11:49Z

test/end2end_test.go

+		t.Fatalf("stream.Send(_) = %v, want <nil>", err)
+	}
+	if _, err := stream.CloseAndRecv(); status.Code(err) != codes.Internal {
+		t.Fatalf("stream.CloseAndRecv() = %v, want error %s", err, codes.Internal)


Suggested change

t.Fatalf("stream.CloseAndRecv() = %v, want error %s", err, codes.Internal)

t.Fatalf("stream.CloseAndRecv() = %v, want error with status code %s", err, codes.Internal)

arjan-bal · 2025-05-27T04:34:07Z

stream.go

@@ -1167,7 +1167,7 @@ func (a *csAttempt) recvMsg(m any, payInfo *payloadInfo) (err error) {
 	} else if err != nil {
 		return toRPCErr(err)
 	}
-	return toRPCErr(errors.New("grpc: client streaming protocol violation: get <nil>, want <EOF>"))
+	return status.Errorf(codes.Internal, "client streaming cardinality violation: get <nil>, want <EOF>")


There is a similar method on addrConnStream that also needs to be updated.

arjan-bal

LGTM with minor comments.

arjan-bal · 2025-05-27T04:43:03Z

test/end2end_test.go

+func (s) TestClientStreaming_ServerHandlerSendAndCloseAfterSendAndClose(t *testing.T) {
+	ss := stubserver.StubServer{
+		StreamingInputCallF: func(stream testgrpc.TestService_StreamingInputCallServer) error {
+			if err := stream.SendAndClose(&testpb.StreamingInputCallResponse{}); err != nil {


In my opinion, we should call Send instead of SendAndClose. SendAndClose doesn't actually close the stream, but it can confuse readers why the server didn't get an error when trying to write to a closed stream.

SendAndClose() have been used for all client-streaming test cases. To maintain the consistency, I have used SendAndClose() in new tests.

If all occurrences of SendAndClose() needs to be replaced by Send(), I'll do that in a separate PR.

Existing refferences use SendAndClose probably because the intention is to close the stream (even though closing doesn't happen until the handler returns). This is not done for consistency. In this test, we want to send multiple message and not close, so I would suggest just changing this test to use Close(). We don't need to change the remaining tests. Both Send and SendAndClose are part of the stable API.

Send() method is not defined in StreamingInputCall. So I have used SendMsg() instead of Send().

Let me know if you want me to use Send() only. I will then change the complete test.

arjan-bal · 2025-05-27T04:46:55Z

stream.go

@@ -1167,7 +1167,7 @@ func (a *csAttempt) recvMsg(m any, payInfo *payloadInfo) (err error) {
 	} else if err != nil {
 		return toRPCErr(err)
 	}
-	return toRPCErr(errors.New("grpc: client streaming protocol violation: get <nil>, want <EOF>"))
+	return status.Errorf(codes.Internal, "client streaming cardinality violation: get <nil>, want <EOF>")


I think we can improve this error message to say something like:
cardinality violation: expected EOF from non-streaming server, but received another message. I believe this can also happen in unary RPCs.

The message still says "" instead of "another message". When a user sees "got nil", they would need to lookup the implementation to understand what it means. If we say "another message", the failure condition is clearer.

Made the changes.

arjan-bal

Approving.

return error code Internal instead of EOF

5da4f32

Pranjali-2501 added this to the 1.73 Release milestone May 15, 2025

Pranjali-2501 added the Type: Bug label May 15, 2025

Pranjali-2501 self-assigned this May 15, 2025

Pranjali-2501 requested review from dfawley and arjan-bal May 15, 2025 17:58

dfawley assigned dfawley and arjan-bal and unassigned Pranjali-2501 May 16, 2025

dfawley approved these changes May 20, 2025

View reviewed changes

dfawley assigned Pranjali-2501 and unassigned dfawley May 20, 2025

minor changes

0810217

Pranjali-2501 mentioned this pull request May 26, 2025

grpc: Fix cardinality violations in non-server streaming RPCs #8278

Merged

arjan-bal reviewed May 27, 2025

View reviewed changes

arjan-bal approved these changes May 27, 2025

View reviewed changes

arjan-bal assigned Pranjali-2501 and unassigned Pranjali-2501 and arjan-bal May 27, 2025

Pranjali-2501 added 2 commits May 27, 2025 07:33

update error message

c3f1f04

Replace SendAndClose() with SendMsg() in the test

0441021

Pranjali-2501 assigned arjan-bal and unassigned Pranjali-2501 May 27, 2025

Pranjali-2501 changed the title ~~Cardinality violations in Client streaming and Unary RPC.~~ grpc: Fix cardinality violations in client streaming and unary RPCs. May 28, 2025

arjan-bal merged commit ec4810c into grpc:master May 28, 2025
15 checks passed

yashuatla mentioned this pull request Jun 25, 2025

Feature implementation from commits 6995ef2..f6bf86c yashuatla/grpc-go#2

Open

	// RPCs if the server call SendAndClose after calling SendAndClose.
	// RPCs if the server calls SendAndClose multiple times.

	t.Fatalf("stream.CloseAndRecv() = %v, want error %s", err, codes.Internal)
	t.Fatalf("stream.CloseAndRecv() = %v, want error with status code %s", err, codes.Internal)

grpc: Fix cardinality violations in client streaming and unary RPCs. #8330

grpc: Fix cardinality violations in client streaming and unary RPCs. #8330

Uh oh!

Conversation

Pranjali-2501 commented May 15, 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 May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dfawley commented May 16, 2025

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arjan-bal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pranjali-2501 May 27, 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!

Pranjali-2501 May 27, 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!

arjan-bal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Pranjali-2501 commented May 15, 2025 •

edited by arjan-bal

Loading

codecov bot commented May 15, 2025 •

edited

Loading

Pranjali-2501 May 27, 2025 •

edited

Loading

Pranjali-2501 May 27, 2025 •

edited

Loading