Client connection phase should optionally wait for SETTINGS frame and set deadlines

[gyuho]

What version of gRPC are you using?

Master branch as of today (bfaf042).

What version of Go are you using (go version)?

go version go1.9rc1 darwin/amd64

What operating system (Linux, Windows, …) and version?

MacOS

What did you do?

c.f. etcd-io/etcd#8258

What did you expect to see?

We want to use keepalive for HTTP/2 ping health checking. We expect endpoint switch when one endpoint times out on keepalive.

What did you see instead?

keepalive time-out triggers address connection state update to transient failure, and resetTransport retries this endpoint: balancer keeps calling Up on the timed-out endpoint. If the endpoint never comes back, balancer gets stuck with retrying.

Is there any other way to stop those retries on timed-out endpoint, and try others? We have our own balancer interface implementation, but the keepalive time-out error is not distinguishable in client side, so not much we can do.

Here's the code path for reference:

1. Configure grpc.Balancer(ep1,ep2) with keepalive 1-second
2. Blackhole(ep1)
3. keepalive(ep1) times out in 1-second, which is expected
4. grpc-go/transport/http2_client.go/*http2Client calls (*http2Client).Close on ep1
   • ep1 has transportState reachable at the moment
   • close(t.errorChan)
5. Signal <-t.Error() on grpc-go/clientconn.go/(*addrConn).transportMonitor()
   • ep1 *addrConn.(connectivity.State) is connectivity.Ready
   • ep1 *addrConn.(connectivity.State) is set to connectivity.TransientFailure
6. resetTransport(drain=false) on ep1
   • Calls ep1's down with grpc: failed with network I/O error
7. resetTransport(drain=false) retries on ep1 unless *addrConn.(connectivity.State) != connectivity.Shutdown
   • for retries := 0; ; retries++ {
8. Still ep1's *addrConn.(connectivity.State) == connectivity.TransientFailure
9. Thus, the retrial loop will keep calling ac.cc.dopts.balancer.Up(ep1)
10. Now, be stuck with blackholed ep1

Thanks.

[gyuho]

We will see if we can temporarily mask timed-out endpoint in application layer.

[dfawley]

It looks like clients are only waiting for a connection to be made and for the client preface and a settings frame to be sent to the server -- never waiting for the server to send a valid settings frame back -- before attempting to use the connection. It may make sense to wait for that settings frame before using the connection. We'd need to do this through a DialOption, because this will break cmux users that don't have the "workaround for java" in place.

Further, we noticed there are no deadlines on the reads/writes happening during connection initialization, which is problematic -- we should set these to the deadline of the context during this phase.

[dfawley]

cc @vtubati

The changes to implement this are not significant, but we have higher priority things in flight right now. I expect this to be done within a month.

[tsuna]

Any update on this bug? We regularly run into crazy busy loop situations and I have to manually patch transportMonitor() in our vendored code to add a time.Sleep(1 * time.Second) at the end of the endless for loop in there as a poor man's solution to get gRPC to chill out instead of going crazy.

[dfawley]

Thanks for the ping. We should hopefully be able to have this done by the end of next week.

[dfawley]

We haven't made much progress on this, but it's at the top of our priority list. Also, we have a slightly different plan:

1. As before, add a DialOption to block in the client until a settings frame is received from the server before sending RPCs on the connection.

2. The new part: even if the option is not set, do not consider the connection "good" (and consequently reset the backoff timer) until a settings frame is received from the server. We would still proceed to send RPCs on this connection immediately, as we do today. But if a failure occurs before a settings frame is received, we will resume connecting to alternate backends and backoff with the same deadline as if the initial server never connected at all.

[anshupitlia]

Any update on this?

[tsuna]

Any update please? This is a problem with the client busy-looping when connecting to a TCP reverse-proxy like haproxy that accepts the connection and has no other choice than closing it if no backend is healthy.

[MakMukhi]

This should be in this week. Sorry for the delay I got distracted by something else.

[dfawley]

Note: this is PR #1648 if you are curious.

[tsuna]

Just to be clear (and for the casual pedestrian stumbling on this issue and seeing it closed) the issue isn't actually fixed unless we use the new DialOption called WithWaitForHandshake(), right?

[dfawley]

If I understand your concerns correctly, then I believe it should be fixed for everyone. We will not consider a connection "successful" (from a backoff perspective) if the server never sent the HTTP2 preface to the client.

The option is there to prevent RPCs from being assigned to the channel until after the handshake has been received. This can be set if you want extra-stable behavior so RPCs don't fail due to a connection that fails in this way.