transport.ErrConnDrain to client is confusing

[gyuho]

What version of gRPC are you using?

v1.7.2

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

Go 1.9.2

What did you do?

Run etcd client integration tests on gRPC dial path

(TestSetEndpointAndPut , https://github.com/coreos/etcd/blob/6f077bd74c5714dfa9221d8f3cb84f856e36ea42/clientv3/integration/dial_test.go#L193-L205)

Here's the code path that's being tested

1. notify addresses A to gRPC balancer
2. gRPC connects to A
3. etcd client switches endpoints to B
   • notify B to gRPC balancer
   • tearDown(errConnDrain) on A

     grpc-go/clientconn.go

     Lines 1119 to 1130 in cda8a00

     	func (ac *addrConn) tearDown(err error) {
     	ac.cancel()
     	ac.mu.Lock()
     	ac.curAddr = resolver.Address{}
     	defer ac.mu.Unlock()
     	if err == errConnDrain && ac.transport != nil {
     	// GracefulClose(...) may be executed multiple times when
     	// i) receiving multiple GoAway frames from the server; or
     	// ii) there are concurrent name resolver/Balancer triggered
     	// address removal and GoAway.
     	ac.transport.GracefulClose()
     	}

   • *http2Client returns transport.ErrStreamDrain in

     grpc-go/transport/http2_client.go

     Lines 403 to 406 in cda8a00

     	if t.state == draining {
     	t.mu.Unlock()
     	return nil, ErrStreamDrain
     	}

What did you expect to see?

transport.ErrConnClosing

What did you see instead?

transport.ErrConnDrain (most of the time, we get transport.ErrConnClosing but this still returns to our client intermittently)

This is confusing, because I thought, with latest balancer changes (in v1.7.x), gRPC does not return connection drain error to clients (see #1649). Now our clients are getting transport.ErrConnDrain.

And the error message the server stops accepting new RPCs is confusing, since this is an error returned from *http2Client, not from server.

What's more confusing is now we have 8ff8683#diff-3081f1aaaf958d7d5e00ccec7a62ea6fR413 that might have fixed this issue.

Are transport.ErrConnDrain errors to clients expected?

c.f. etcd-io/etcd#8859 and etcd-io/etcd#8869

Thanks.

/cc @xiang90

[xiang90]

@dfawley @menghanl

any update on this one?

[menghanl]

The draining message was added to support server graceful close. The error message was valid at that time. It was reused when an address is removed by a balancer, ant the message became invalid and confusing in this.
I filed #1844 to fix the error message. But it would be a behavior change, and since you are doing string matching in tests, you might also need to update your tests accordingly. Let me know what you think on the PR.

The fix (8ff8683#diff-3081f1aaaf958d7d5e00ccec7a62ea6fR413) you mentioned was to implement transparent retry.

With transparent retry, when an RPC fails because the picked connection is draining, the RPC will be retried one more time. Since ClientConn always tries to pick a healthy connection to send the RPC, in this retry, another connection would be picked and the RPC will succeed if the second picked connection is working. So even though you may see that the problem is "fixed", it's not. It's just made to be less likely to happen.