grpc: Fix cardinality violations in client 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 (f06ff56, bbd8366, 1546427, c3ca09b, 9436092, 643998d, f37ea78, a21fcc4, 6deac72, 4e5075b, ad4ce90, f85707b, 11da0dc, 7808f8a, ca4860a, 1a23bb7, 8107444)

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

Codecov Report

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

Project coverage is 82.20%. Comparing base (6dfe07c) to head (4e5075b).

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8278      +/-   ##
==========================================
- Coverage   82.33%   82.20%   -0.14%     
==========================================
  Files         419      419              
  Lines       42062    42068       +6     
==========================================
- Hits        34631    34580      -51     
- Misses       5979     6019      +40     
- Partials     1452     1469      +17

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

... and 24 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 io.EOF // indicates successful end of stream.
 		}

 		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

@@ -1134,11 +1136,17 @@ func (a *csAttempt) recvMsg(m any, payInfo *payloadInfo) (err error) {
 			if statusErr := a.transportStream.Status().Err(); statusErr != nil {
 				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 io.EOF // indicates successful end of stream.
 		}

 		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

@@ -1134,11 +1136,17 @@ func (a *csAttempt) recvMsg(m any, payInfo *payloadInfo) (err error) {
 			if statusErr := a.transportStream.Status().Err(); statusErr != nil {
 				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.

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

stream.go

@@ -1134,6 +1134,10 @@ func (a *csAttempt) recvMsg(m any, payInfo *payloadInfo) (err error) {
 			if statusErr := a.transportStream.Status().Err(); statusErr != nil {
 				return statusErr
 			}
+			// received no msg and status ok for non-server streaming rpcs.
+			if !cs.desc.ServerStreams {


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

I'll made the similar changes once the changes made in client.RecvMsg() gets finalized.

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

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

Pranjali-2501 added 2 commits May 26, 2025 17:05

modify error statement

9436092

changed test to not use low-level framer

11da0dc

Pranjali-2501 requested a review from arjan-bal May 26, 2025 20:15

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

arjan-bal reviewed May 27, 2025

View reviewed changes

added test

7808f8a

Pranjali-2501 requested a review from arjan-bal May 27, 2025 07:51

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

arjan-bal changed the title ~~Cardinality violations in Client streaming RPC.~~ grpc: Fix cardinality violations in client streaming RPCs May 28, 2025

Pranjali-2501 added 2 commits May 28, 2025 10:22

add test for unary RPC

ad4ce90

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

grpc: Fix cardinality violations in client streaming RPCs #8278

Are you sure you want to change the base?

grpc: Fix cardinality violations in client 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!

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