-
Notifications
You must be signed in to change notification settings - Fork 4.5k
grpc: Fix cardinality violations in non-client streaming RPCs. #8385
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
298edba
4c3a18a
fd5d614
7e4206c
324566b
9fbf6b7
9b9cddf
63565f2
67549e7
6030b90
dff08b3
f4f1b61
83e8664
5acd9ab
8f4f39b
6de922d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -1580,6 +1580,7 @@ | |||||
s *transport.ServerStream | ||||||
p *parser | ||||||
codec baseCodec | ||||||
desc *StreamDesc | ||||||
|
||||||
compressorV0 Compressor | ||||||
compressorV1 encoding.Compressor | ||||||
|
@@ -1774,6 +1775,9 @@ | |||||
binlog.Log(ss.ctx, chc) | ||||||
} | ||||||
} | ||||||
if !ss.desc.ClientStreams { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a behavior change? Users that call |
||||||
return status.Error(codes.Internal, "cardinality violation: received no request message from non-client-stream RPC") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit:
Suggested change
|
||||||
} | ||||||
return err | ||||||
} | ||||||
if err == io.ErrUnexpectedEOF { | ||||||
|
@@ -1800,7 +1804,19 @@ | |||||
binlog.Log(ss.ctx, cm) | ||||||
} | ||||||
} | ||||||
return nil | ||||||
|
||||||
if ss.desc.ClientStreams { | ||||||
// Subsequent messages should be received by subsequent RecvMsg calls. | ||||||
return nil | ||||||
} | ||||||
// Special handling for non-client-stream rpcs. | ||||||
// This recv expects EOF or errors, so we don't collect inPayload. | ||||||
if err := recv(ss.p, ss.codec, ss.s, ss.decompressorV0, m, ss.maxReceiveMessageSize, nil, ss.decompressorV1, true); err == io.EOF { | ||||||
return nil | ||||||
} else if err != nil { | ||||||
return err | ||||||
} | ||||||
return status.Error(codes.Internal, "cardinality violation: expected <EOF> for non client-streaming RPCs, but received another message") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about something more like the first message:
Suggested change
|
||||||
} | ||||||
|
||||||
// MethodFromServerStream returns the method string for the input stream. | ||||||
|
arjan-bal marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3740,6 +3740,345 @@ func (s) TestClientStreaming_ReturnErrorAfterSendAndClose(t *testing.T) { | |
} | ||
} | ||
|
||
// Tests the behavior for server-side streaming when server calls RecvMsg twice. | ||
// Second call to RecvMsg should fail with Internal error. | ||
func (s) TestServerStreaming_ServerCallRecvMsgTwice(t *testing.T) { | ||
lis, err := testutils.LocalTCPListener() | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
defer lis.Close() | ||
|
||
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 { | ||
t.Errorf("stream.RecvMsg() = %v, want error %v", status.Code(err), codes.Internal) | ||
} | ||
return nil | ||
}, | ||
} | ||
if err := ss.Start(nil); err != nil { | ||
t.Fatal("Error starting server:", err) | ||
} | ||
defer ss.Stop() | ||
|
||
ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) | ||
defer cancel() | ||
|
||
stream, err := ss.Client.StreamingOutputCall(ctx, &testpb.StreamingOutputCallRequest{}) | ||
if err != nil { | ||
t.Fatalf(".StreamingOutputCall(_) = _, %v, want <nil>", err) | ||
} | ||
|
||
if err := stream.RecvMsg(&testpb.Empty{}); status.Code(err) != codes.Internal { | ||
t.Errorf("stream.RecvMsg() = %v, want error %v", status.Code(err), codes.Internal) | ||
} | ||
} | ||
|
||
// Tests the behavior for server-side streaming when client calls SendMsg twice. | ||
// Second call to SendMsg should fail with Internal error. | ||
func (s) TestServerStreaming_ClientCallSendMsgTwice(t *testing.T) { | ||
lis, err := testutils.LocalTCPListener() | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
defer lis.Close() | ||
|
||
s := grpc.NewServer() | ||
serviceDesc := grpc.ServiceDesc{ | ||
ServiceName: "grpc.testing.TestService", | ||
HandlerType: (*any)(nil), | ||
Methods: []grpc.MethodDesc{}, | ||
Streams: []grpc.StreamDesc{ | ||
{ | ||
StreamName: "ServerStreaming", | ||
Handler: func(_ any, _ grpc.ServerStream) error { | ||
return nil | ||
}, | ||
ClientStreams: false, | ||
ServerStreams: true, | ||
}, | ||
}, | ||
} | ||
s.RegisterService(&serviceDesc, &testServer{}) | ||
go s.Serve(lis) | ||
defer s.Stop() | ||
|
||
ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) | ||
defer cancel() | ||
cc, err := grpc.NewClient(lis.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials())) | ||
if err != nil { | ||
t.Fatalf("grpc.NewClient(%q) failed unexpectedly: %v", lis.Addr(), err) | ||
} | ||
defer cc.Close() | ||
|
||
desc := &grpc.StreamDesc{ | ||
StreamName: "ServerStreaming", | ||
ServerStreams: true, | ||
ClientStreams: false, | ||
} | ||
|
||
stream, err := cc.NewStream(ctx, desc, "/grpc.testing.TestService/ServerStreaming") | ||
if err != nil { | ||
t.Fatalf("cc.NewStream() failed unexpectedly: %v", err) | ||
} | ||
|
||
if err := stream.SendMsg(&testpb.Empty{}); err != nil { | ||
t.Errorf("stream.SendMsg() = %v, want <nil>", err) | ||
} | ||
|
||
if err := stream.SendMsg(&testpb.Empty{}); status.Code(err) != codes.Internal { | ||
t.Errorf("stream.SendMsg() = %v, want error %v", status.Code(err), codes.Internal) | ||
} | ||
Comment on lines
+3831
to
+3833
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
|
||
// Tests the behavior for unary RPC when server calls RecvMsg twice. Second call | ||
// to RecvMsg should fail with Internal error. | ||
func (s) TestUnaryRPC_ServerCallRecvMsgTwice(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this be combined with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bump on this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test are different now. |
||
lis, err := testutils.LocalTCPListener() | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
defer lis.Close() | ||
|
||
s := grpc.NewServer() | ||
serviceDesc := grpc.ServiceDesc{ | ||
ServiceName: "grpc.testing.TestService", | ||
HandlerType: (*any)(nil), | ||
Methods: []grpc.MethodDesc{}, | ||
Streams: []grpc.StreamDesc{ | ||
{ | ||
StreamName: "UnaryCall", | ||
Handler: func(_ any, stream grpc.ServerStream) error { | ||
err := stream.RecvMsg(&testpb.Empty{}) | ||
if err != nil { | ||
t.Errorf("stream.RecvMsg() = %v, want <nil>", err) | ||
} | ||
|
||
if err = stream.RecvMsg(&testpb.Empty{}); status.Code(err) != codes.Internal { | ||
t.Errorf("stream.RecvMsg() = %v, want error %v", status.Code(err), codes.Internal) | ||
} | ||
return nil | ||
}, | ||
ClientStreams: false, | ||
ServerStreams: false, | ||
}, | ||
}, | ||
} | ||
s.RegisterService(&serviceDesc, &testServer{}) | ||
go s.Serve(lis) | ||
defer s.Stop() | ||
|
||
ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) | ||
defer cancel() | ||
cc, err := grpc.NewClient(lis.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials())) | ||
if err != nil { | ||
t.Fatalf("grpc.NewClient(%q) failed unexpectedly: %v", lis.Addr(), err) | ||
} | ||
defer cc.Close() | ||
|
||
desc := &grpc.StreamDesc{ | ||
StreamName: "UnaryCall", | ||
ServerStreams: false, | ||
ClientStreams: false, | ||
} | ||
|
||
stream, err := cc.NewStream(ctx, desc, "/grpc.testing.TestService/UnaryCall") | ||
if err != nil { | ||
t.Fatalf("cc.NewStream() failed unexpectedly: %v", err) | ||
} | ||
|
||
if err := stream.SendMsg(&testpb.Empty{}); err != nil { | ||
t.Errorf("stream.SendMsg() = %v, want <nil>", err) | ||
} | ||
|
||
if err := stream.RecvMsg(&testpb.Empty{}); status.Code(err) != codes.Internal { | ||
t.Errorf("stream.RecvMsg() = %v, want error %v", status.Code(err), codes.Internal) | ||
} | ||
} | ||
|
||
// 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) { | ||
lis, err := testutils.LocalTCPListener() | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
defer lis.Close() | ||
|
||
s := grpc.NewServer() | ||
serviceDesc := grpc.ServiceDesc{ | ||
ServiceName: "grpc.testing.TestService", | ||
HandlerType: (*any)(nil), | ||
Methods: []grpc.MethodDesc{}, | ||
Streams: []grpc.StreamDesc{ | ||
{ | ||
StreamName: "UnaryCall", | ||
Handler: func(_ any, _ grpc.ServerStream) error { | ||
return nil | ||
}, | ||
ClientStreams: false, | ||
ServerStreams: false, | ||
}, | ||
}, | ||
} | ||
s.RegisterService(&serviceDesc, &testServer{}) | ||
go s.Serve(lis) | ||
defer s.Stop() | ||
|
||
ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) | ||
defer cancel() | ||
cc, err := grpc.NewClient(lis.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials())) | ||
if err != nil { | ||
t.Fatalf("grpc.NewClient(%q) failed unexpectedly: %v", lis.Addr(), err) | ||
} | ||
defer cc.Close() | ||
|
||
desc := &grpc.StreamDesc{ | ||
StreamName: "UnaryCall", | ||
ServerStreams: false, | ||
ClientStreams: false, | ||
} | ||
|
||
stream, err := cc.NewStream(ctx, desc, "/grpc.testing.TestService/UnaryCall") | ||
if err != nil { | ||
t.Fatalf("cc.NewStream() failed unexpectedly: %v", err) | ||
} | ||
|
||
if err := stream.SendMsg(&testpb.Empty{}); err != nil { | ||
t.Errorf("stream.SendMsg() = %v, want <nil>", err) | ||
} | ||
|
||
if err := stream.SendMsg(&testpb.Empty{}); status.Code(err) != codes.Internal { | ||
t.Errorf("stream.SendMsg() = %v, want error %v", status.Code(err), codes.Internal) | ||
} | ||
} | ||
|
||
// Tests the behavior for server-side streaming RPC when client misbehaves as Bidi-streaming | ||
// and sends multiple nessages. | ||
func (s) TestServerStreaming_ClientSendsMultipleMessages(t *testing.T) { | ||
lis, err := testutils.LocalTCPListener() | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
defer lis.Close() | ||
|
||
s := grpc.NewServer() | ||
serviceDesc := grpc.ServiceDesc{ | ||
ServiceName: "grpc.testing.TestService", | ||
HandlerType: (*any)(nil), | ||
Methods: []grpc.MethodDesc{}, | ||
Streams: []grpc.StreamDesc{ | ||
{ | ||
StreamName: "ServerStreaming", | ||
Handler: func(_ any, stream grpc.ServerStream) error { | ||
if err = stream.RecvMsg(&testpb.Empty{}); status.Code(err) != codes.Internal { | ||
t.Errorf("stream.RecvMsg() = %v, want error %v", status.Code(err), codes.Internal) | ||
} | ||
return nil | ||
}, | ||
ClientStreams: false, | ||
ServerStreams: true, | ||
}, | ||
}, | ||
} | ||
s.RegisterService(&serviceDesc, &testServer{}) | ||
go s.Serve(lis) | ||
defer s.Stop() | ||
|
||
ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) | ||
defer cancel() | ||
cc, err := grpc.NewClient(lis.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials())) | ||
if err != nil { | ||
t.Fatalf("grpc.NewClient(%q) failed unexpectedly: %v", lis.Addr(), err) | ||
} | ||
defer cc.Close() | ||
|
||
// Making the client bi-di to bypass the client side checks that stop a non-streaming client | ||
// from sending multiple messages. | ||
desc := &grpc.StreamDesc{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a comment describing the need to use a bi-di streaming client for this test. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see the comment here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had added a comment before test. |
||
StreamName: "ServerStreaming", | ||
ServerStreams: true, | ||
ClientStreams: true, | ||
} | ||
|
||
stream, err := cc.NewStream(ctx, desc, "/grpc.testing.TestService/ServerStreaming") | ||
if err != nil { | ||
t.Fatalf("cc.NewStream() failed unexpectedly: %v", err) | ||
} | ||
|
||
if err := stream.SendMsg(&testpb.Empty{}); err != nil { | ||
t.Errorf("stream.SendMsg() = %v, want <nil>", err) | ||
} | ||
|
||
if err := stream.SendMsg(&testpb.Empty{}); err != nil { | ||
t.Errorf("stream.SendMsg() = %v, want <nil>", err) | ||
} | ||
|
||
if err := stream.RecvMsg(&testpb.Empty{}); status.Code(err) != codes.Internal { | ||
t.Errorf("stream.RecvMsg() = %v, want error %v", status.Code(err), codes.Internal) | ||
} | ||
} | ||
|
||
// Tests the behavior for server-side streaming RPC when client sends zero request message. | ||
func (s) TestServerStreaming_ClientSendsZeroRequest(t *testing.T) { | ||
lis, err := testutils.LocalTCPListener() | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
defer lis.Close() | ||
|
||
s := grpc.NewServer() | ||
serviceDesc := grpc.ServiceDesc{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
ServiceName: "grpc.testing.TestService", | ||
HandlerType: (*any)(nil), | ||
Methods: []grpc.MethodDesc{}, | ||
Streams: []grpc.StreamDesc{ | ||
{ | ||
StreamName: "ServerStreaming", | ||
Handler: func(_ any, stream grpc.ServerStream) error { | ||
if err = stream.RecvMsg(&testpb.Empty{}); status.Code(err) != codes.Internal { | ||
t.Errorf("stream.RecvMsg() = %v, want error %v", status.Code(err), codes.Internal) | ||
} | ||
return nil | ||
}, | ||
ClientStreams: false, | ||
ServerStreams: true, | ||
}, | ||
}, | ||
} | ||
s.RegisterService(&serviceDesc, &testServer{}) | ||
go s.Serve(lis) | ||
defer s.Stop() | ||
|
||
ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) | ||
defer cancel() | ||
cc, err := grpc.NewClient(lis.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials())) | ||
if err != nil { | ||
t.Fatalf("grpc.NewClient(%q) failed unexpectedly: %v", lis.Addr(), err) | ||
} | ||
defer cc.Close() | ||
|
||
desc := &grpc.StreamDesc{ | ||
StreamName: "ServerStreaming", | ||
ServerStreams: true, | ||
ClientStreams: false, | ||
} | ||
|
||
stream, err := cc.NewStream(ctx, desc, "/grpc.testing.TestService/ServerStreaming") | ||
if err != nil { | ||
t.Fatalf("cc.NewStream() failed unexpectedly: %v", err) | ||
} | ||
|
||
if err := stream.CloseSend(); err != nil { | ||
t.Errorf("stream.CloseSend() = %v, want <nil>", err) | ||
} | ||
if err := stream.RecvMsg(&testpb.Empty{}); status.Code(err) != codes.Internal { | ||
t.Errorf("stream.RecvMsg() = %v, want error %v", status.Code(err), codes.Internal) | ||
} | ||
} | ||
|
||
// Tests that a client receives a cardinality violation error for client-streaming | ||
// RPCs if the server call SendMsg multiple times. | ||
func (s) TestClientStreaming_ServerHandlerSendMsgAfterSendMsg(t *testing.T) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not immediately clear to me how this conditions indicates RecvMsg is called twice. From my understanding, the handling of cardinality violations in the server stream should be similar to the one in client stream. The check here should be for 0 requests being received for a non-client-streaming request.
grpc-go/stream.go
Lines 1142 to 1144 in e5de1e2
For > 1 requests, there should be a block near the end of this method similar to the following:
grpc-go/stream.go
Lines 1167 to 1178 in e5de1e2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any issue if we try to follow the same pattern as client streams here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In server-streaming, client can't send more than one message. So the following code will be useless for server.recvmsg().
grpc-go/stream.go
Lines 1167 to 1178 in e5de1e2
Right, in client.recvMsg() this check is triggered in case of 0 response from server.
But in server-stream, it is not possible to have 0 request. So the following case will only be triggered when server.recvmsg() is called more than once and receives an
io.EOF
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is stopping a client from doing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clientstream maintains a variable named
sentlast
which will be set totrue
when client sends a msg in case of non client-streaming RPCs.grpc-go/stream.go
Lines 905 to 910 in e5de1e2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we discussed earlier, can we have the client (incorrectly) create a bidi stream by manipulating the strem desc?
Ideally, a well behaving client should never cause cardinality violations. The reason for adding these checks is to fail the RPCs with good error codes and messages when a client misbehaves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the logic in server.recvmsg() to call recv() twice in case of non-client-stream RPCs. Also added a test with misbehaving client.