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

Pranjali-2501 · 2025-04-29T13:57:21Z

Partially addresses: #7286

Client should ensure an "Internal" error is returned for client-streaming RPCs if the server doesn't send a message before returning status OK.
Currently it is returning "EOF", which should not be the case for cardinality violations.

RELEASE NOTES:

grpc: return status code INTERNAL when servers send 0 response messages in unary and client streaming RPCs.

linux-foundation-easycla · 2025-04-29T13:57:26Z

The committers listed above are authorized under a signed CLA.

✅ login: Pranjali-2501 (a21fcc4, 643998d, c3ca09b, f85707b, 11da0dc, ad4ce90, 85ab6ed, 1a23bb7, ca4860a, 1546427, b2ab207, 9436092, 4e5075b, 7808f8a, f06ff56, f37ea78, 8107444, 6deac72, bbd8366)

codecov · 2025-04-29T14:03:22Z

Codecov Report

Attention: Patch coverage is 50.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 82.10%. Comparing base (6dfe07c) to head (85ab6ed).
Report is 7 commits behind head on master.

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8278      +/-   ##
==========================================
- Coverage   82.33%   82.10%   -0.23%     
==========================================
  Files         419      419              
  Lines       42062    42071       +9     
==========================================
- Hits        34631    34543      -88     
- Misses       5979     6049      +70     
- Partials     1452     1479      +27

Files with missing lines	Coverage Δ
stream.go	`81.74% <50.00%> (-0.17%)`	⬇️

... and 36 files with indirect coverage changes

🚀 New features to boost your workflow:

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

stream.go

arjan-bal · 2025-05-05T09:33:01Z

stream.go

 		return toRPCErr(err)
 	}
+	if cs.desc.ClientStreams {
+		cs.recvMsg = true


Can we set this boolean without the check? Otherwise we would need to document the variable in a way to make it obvious that this is set only for client streaming RPCs. Even then users may accidentally use it for other types of RPCs.

Changed it to !cs.desc.ServerStreams, so that it will mark cs.recvFirstMsg=true for both unary and clientStreaming RPCs.

stream.go

arjan-bal · 2025-05-05T09:49:23Z

Do we need a similar check on the server side also, i.e. change the status to internal if the server attempts to close without writing a message?

dfawley · 2025-05-12T20:06:19Z

stream.go

 				return statusErr
 			}
+			if !cs.desc.ServerStreams && !cs.recvFirstMsg {
+				return status.Errorf(codes.Internal, "client streaming cardinality violation")


Let's be more specific about what happened here so that a user could understand what's happening when they see the error.

"received no response message from non-streaming RPC"?

I don't see a change here.

@Pranjali-2501 bump.

dfawley · 2025-05-12T20:06:21Z

stream.go

 		return toRPCErr(err)
 	}
+	if !cs.desc.ServerStreams {
+		cs.recvFirstMsg = true


Should there also be something here that fails if recvFirstMsg is already set? (That would indicate we received multiple messages in a unary RPC.)

For both unary and client-streaming RPCs, client.recvmsg() will wait for the trailers after receiving its first msg over here .

I'll change the status code here from UNKNOWN to INTERNAL in upcoming PR with tests. It will handle the case when trailers are not send from server side for unary and client-streaming RPCs.

I see. So why do you need this new field, then?

If the above recv ever returns io.EOF, then we already know there was never a message read from the stream. Either that, or the user called Recv after receiving an error from a previous call to Recv, which is illegal and undefined.

Yeah, make sense.
So for non-server streaming rpcs, we can directly return INTERNAL error if the first recv() call gets EOF.

if !cs.desc.ServerStreams { return status.Errorf(codes.Internal, "client streaming cardinality violation") }

Yes, I think that should work and avoids adding new state to track.

Ping - I think we can remove recvFirstMsg now?

I had reverted that in this commit bbd8366.
But than @arjan-bal pointed out that in case of successful RPC, if user call RecvMsg() twice, the second RecvMsg() call should return with io.EOF instead of cardinality violation.
That's why I reverted back to the older code to fulfill that condition.

As I mentioned before, it's not defined what happens if RecvMsg is called after returning an error.

There are other scenarios where calling recvMsg repeatedly will not cause the previously returned error to occur. E.g. if a received message is too large, recv will return an error. But I believe a subsequent call to RecvMsg would actually return the next message from the stream, or io.EOF at the end of the stream.

Acknowledged, if we're fine with changing the existing behaviour of returning io.EOF on subsequent calls after an RPC ends successfully, we can get rid of the extra variable to track state.

I have removed the extra variable and the tests that were added to check the scenario when recvMsg() called twice.

dfawley · 2025-05-20T21:55:25Z

stream.go

 				return statusErr
 			}
+			if !cs.desc.ServerStreams && !cs.recvFirstMsg {
+				return status.Errorf(codes.Internal, "client streaming cardinality violation")


I don't see a change here.

test/end2end_test.go

dfawley · 2025-05-20T22:08:54Z

test/end2end_test.go

+			return
+		}
+		defer conn.Close()
+		framer := http2.NewFramer(conn, conn)


Realistically this test shouldn't need low level framer access. You should be able to implement it with a grpc.Server that has its EmptyCall handler configured as a streaming handling, and with an implementation that just returns nil immediately without sending a message.

I do think that would be preferable, because the framing stuff is tedious and error prone, and it makes the test much more complex. If you're having too much trouble with that approach, let me know and I can try to help, or if it somehow turns out to be a lot more work than I was expecting, feel free to push back.

Initially, I attempted to resolve this by returning nil from EmptyCall. However, this approach resulted in an empty, successful response rather than a failure, which wasn't the desired outcome.

The scenario that sends no response cannot be replicated within grpc-go itself, as the framework requires a response to be sent.

This test aims to verify behavior in a cross-language context, where a unary client might interact with a unary server implemented in a different language, potentially allowing the server to send no response.

Please let me know if that is not the case or I overlook something.

I think @dfawley is saying that we can register a streaming handler for a unary method on the server. The client will call EmptyCall, but the server will call a streaming handler.

srv := grpc.NewServer() serviceDesc := grpc.ServiceDesc{ ServiceName: "grpc.testing.TestService", HandlerType: (*any)(nil), Methods: []grpc.MethodDesc{}, Streams: []grpc.StreamDesc{ { StreamName: "EmptyCall", Handler: func(any, grpc.ServerStream) error { return nil }, ClientStreams: true, }, }, Metadata: "grpc/testing/test.proto", } srv.RegisterService(&serviceDesc, &testServer{}) go srv.Serve(lis) defer srv.Stop()

Thanks for clarifying. I had made the changes.

Another option (maybe easier, but no need to change now) is to use the unknown service handler, and don't register anything on the server: https://pkg.go.dev/google.golang.org/grpc#UnknownServiceHandler

UnknownServiceHandler uses bidirectional streaming[desc.clientstreams:true, desc.serverstreams:true].
Will it work with the changes I made to addresses cardinality violations?

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

The idea is to make the server think it's a bidi stream, but the client thinks it's not. The client's setting is what the code snippet you are showing is checking. The client has no knowledge of how the server is treating the stream.

@Pranjali-2501 did you try to use UnknownServiceHandler? I think it should reduce the amount of test code.

Made the changes.

stream.go

test/end2end_test.go

dfawley · 2025-06-06T21:08:30Z

test/end2end_test.go

+}
+
+// Tests that client will receive cardinality violation in the subsequent calls to RecvMsg().
+func (s) TestClientStreaming_ClientCallRecvMsgTwice(t *testing.T) {


In what way is this different from the previous test case? The server side is different, but actually does the same thing.

Both tests does the same thing, only difference is StreamDesc.ClientStreams is set to false in this test(Unary RPC) and set to true in previous test(Client Streaming RPC)

This PR is actually a fix for for "non-server streaming" RPCs instead of "client streaming RPCs". I've updated the PR title to reflect the same. So the value of StreamDesc.ClientStreams should not matter, but it's okay to have test coverage for this.

I noticed that none of the tests send 2 response messages from the server. This shows up as missing coverage reported by Codecov around line 1486. Can you please have the server send back two response messages here? You should use the UnknownServiceHandler for this to ensure the server doesn't skip sending the message after noticing it's a non-server streaming RPC.

Discussed offline: line 1486 is in addrConnStream, not csAttempt. I had initially thought the missing coverage was in csAttempt. addrConnStream is largely similar to csAttempt but is only used for establishing health streams. There is a plan to unify it with csAttempt in the future. Adding test coverage for addrConnStream may be difficult and is not necessary at this point.

test/end2end_test.go

)

…grpc#8278)" This reverts commit a64d933.

…g RPCs (#8404) This reverts commit a64d933.

…avior to return io.EOF on repeated RecvMsg() calls for client-streaming RPCs (#8523) Partially addresses: #7286 This reverts commit 20bd1e7 Changes: - Modifies client.RecvMsg() so that successive calls after stream ends return io.EOF. - Adds extra state to track calls to client.recvmsg(required to return Cardinality Violation only in case zero response) RELEASE NOTES: * client: Return status code INTERNAL when a server sends 0 response messages for a unary or client streaming RPC.

… behavior to return io.EOF on repeated RecvMsg() calls for client-streaming RPCs (grpc#8523) Partially addresses: grpc#7286 This reverts commit grpc@20bd1e7 Changes: - Modifies client.RecvMsg() so that successive calls after stream ends return io.EOF. - Adds extra state to track calls to client.recvmsg(required to return Cardinality Violation only in case zero response) RELEASE NOTES: * client: Return status code INTERNAL when a server sends 0 response messages for a unary or client streaming RPC.

return error code Internal instead of EOF

f06ff56

arjan-bal reviewed May 5, 2025

View reviewed changes

arjan-bal assigned Pranjali-2501 May 5, 2025

arjan-bal added the Type: Bug label May 5, 2025

arjan-bal added this to the 1.73 Release milestone May 5, 2025

Pranjali-2501 added 2 commits May 12, 2025 04:47

change variable name from recvMsg to recvFirstMsg

1546427

changes for unary rpc

ca4860a

Pranjali-2501 requested review from arjan-bal and dfawley May 12, 2025 19:46

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

dfawley reviewed May 12, 2025

View reviewed changes

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

Pranjali-2501 added 2 commits May 13, 2025 09:39

added comment

c3ca09b

added test

bbd8366

Pranjali-2501 requested a review from dfawley May 15, 2025 17:14

add comment before test

1a23bb7

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

dfawley reviewed May 20, 2025

View reviewed changes

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

formatting changes

f37ea78

Pranjali-2501 requested a review from dfawley May 26, 2025 08:27

Pranjali-2501 assigned dfawley and arjan-bal May 26, 2025

add test for unary RPC

a21fcc4

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

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

Pranjali-2501 and others added 3 commits June 3, 2025 06:39

remove extra variable

8107444

Merge branch 'master' into cardinality_violation

6deac72

vet errors

643998d

Pranjali-2501 assigned dfawley and unassigned Pranjali-2501 Jun 3, 2025

Pranjali-2501 added 2 commits June 3, 2025 10:14

added tests back

f85707b

refactor comments

4e5075b

arjan-bal approved these changes Jun 5, 2025

View reviewed changes

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

eshitachandwani modified the milestones: 1.73 Release, 1.74 Release Jun 5, 2025

use unknownServiceHandler

b2ab207

dfawley reviewed Jun 6, 2025

View reviewed changes

dfawley removed their assignment Jun 6, 2025

arjan-bal reviewed Jun 9, 2025

View reviewed changes

test/end2end_test.go Outdated Show resolved Hide resolved

resolved nits

85ab6ed

arjan-bal changed the title ~~grpc: Fix cardinality violations in client streaming RPCs~~ grpc: Fix cardinality violations in non-server streaming RPCs Jun 10, 2025

Pranjali-2501 requested a review from dfawley June 10, 2025 09:56

arjan-bal merged commit a64d933 into grpc:master Jun 11, 2025
23 of 24 checks passed

vinothkumarr227 pushed a commit to vinothkumarr227/grpc-go that referenced this pull request Jun 19, 2025

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

bf7d62a

)

arjan-bal added a commit to arjan-bal/grpc-go that referenced this pull request Jun 23, 2025

Revert "grpc: Fix cardinality violations in non-server streaming RPCs (…

2db9aa5

…grpc#8278)" This reverts commit a64d933.

arjan-bal added a commit that referenced this pull request Jun 23, 2025

grpc: revert #8278: Fix cardinality violations in non-server streamin…

20bd1e7

…g RPCs (#8404) This reverts commit a64d933.

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

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

Uh oh!

Conversation

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

Uh oh!

Uh oh!

linux-foundation-easycla bot commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arjan-bal commented May 5, 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!

Choose a reason for hiding this comment

Uh oh!

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

Pranjali-2501 Jun 3, 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!

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!

Choose a reason for hiding this comment

Pranjali-2501 commented Apr 29, 2025 •

edited by arjan-bal

Loading

linux-foundation-easycla bot commented Apr 29, 2025 •

edited

Loading

codecov bot commented Apr 29, 2025 •

edited

Loading

Pranjali-2501 May 13, 2025 •

edited

Loading

Pranjali-2501 Jun 3, 2025 •

edited

Loading

arjan-bal Jun 10, 2025 •

edited

Loading