Skip to content
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

client: propagate connection error causes to RPC statuses #4311

Merged
merged 12 commits into from Apr 13, 2021
18 changes: 12 additions & 6 deletions test/end2end_test.go
Expand Up @@ -1334,28 +1334,30 @@ func testConcurrentServerStopAndGoAway(t *testing.T, e env) {
}

func (s) TestDetailedConnectionCloseErrorPropagatesToRpcError(t *testing.T) {
rpcStartedOnServer := make(chan struct{})
rpcDoneOnClient := make(chan struct{})
ss := &stubserver.StubServer{
FullDuplexCallF: func(stream testpb.TestService_FullDuplexCallServer) error {
close(rpcStartedOnServer)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, another option here would be to send a message, and have the client receive it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This behavior of this test is IMO easier to reason about as is, just since there's slightly fewer moving parts and less happening on the wire. So for the purposes of this test, I feel the existing approach is simpler so I'll keep as is.

<-rpcDoneOnClient
return status.Error(codes.Internal, "arbitrary status")
},
}
// Use WithBlock() to guarantee that the RPC will be able to pick a healthy connection to
// go out on before we call Stop on the server.
if err := ss.Start(nil, grpc.WithBlock()); err != nil {
if err := ss.Start(nil); err != nil {
t.Fatalf("Error starting endpoint server: %v", err)
}
defer ss.Stop()

ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer cancel()
// The precise behavior of this test is subject to raceyness around the timing of when TCP packets
// or sent from client to server, and when we tell the server to stop, so we need to account for both
// are sent from client to server, and when we tell the server to stop, so we need to account for both
// of these possible error messages:
// 1) If the call to ss.S.Stop() causes the server's sockets to close while there's still in-fight
// data from the client on the TCP connection, then the kernel can send an RST back to the client (also
// see https://stackoverflow.com/questions/33053507/econnreset-in-send-linux-c).
// see https://stackoverflow.com/questions/33053507/econnreset-in-send-linux-c). Note that while this
// condition is expected to be rare due to the rpcStartedOnServer synchronization, in theory it should
// be possible, e.g. if the client sends a BDP ping at the right time.
// 2) If, for example, the call to ss.S.Stop() happens after the RPC headers have been received at the
// server, then the TCP connection can shutdown gracefully when the server's socket closes.
const possibleConnResetMsg = "connection reset by peer"
Expand All @@ -1367,9 +1369,13 @@ func (s) TestDetailedConnectionCloseErrorPropagatesToRpcError(t *testing.T) {
if err != nil {
t.Fatalf("%v.FullDuplexCall = _, %v, want _, <nil>", ss.Client, err)
}
// Block until the RPC has been started on the server. This ensures that the ClientConn will find a healthy
// connection for the RPC to go out on initially, and that the TCP connection will shut down strictly after
// the RPC has been started on it.
<-rpcStartedOnServer
ss.S.Stop()
if _, err := stream.Recv(); err == nil || (!strings.Contains(err.Error(), possibleConnResetMsg) && !strings.Contains(err.Error(), possibleEOFMsg)) {
t.Fatalf("%v.Recv() = _, %v, want _, rpc error containing substring: |%v| OR |%v|", stream, err, possibleConnResetMsg, possibleEOFMsg)
t.Fatalf("%v.Recv() = _, %v, want _, rpc error containing substring: %q OR %q", stream, err, possibleConnResetMsg, possibleEOFMsg)
}
close(rpcDoneOnClient)
}
Expand Down