From a4587cd3f03fe017b6dcfd0f42dd550faae8d171 Mon Sep 17 00:00:00 2001 From: Menghan Li Date: Fri, 29 Jul 2016 17:17:36 -0700 Subject: [PATCH] Fix review comments --- call.go | 3 ++- clientconn.go | 23 ++++++++++++----------- transport/transport.go | 22 +++++++++++----------- 3 files changed, 25 insertions(+), 23 deletions(-) diff --git a/call.go b/call.go index 0f34b5e8..5fba11eb 100644 --- a/call.go +++ b/call.go @@ -84,7 +84,8 @@ func sendRequest(ctx context.Context, codec Codec, compressor Compressor, callHd } defer func() { if err != nil { - if e, ok := err.(transport.ConnectionError); !ok || !e.Temporary() { + // If err is connection error, t will be closed, no need to close stream here. + if _, ok := err.(transport.ConnectionError); !ok { t.CloseStream(stream, err) } } diff --git a/clientconn.go b/clientconn.go index 946cca9c..8d9d8d1c 100644 --- a/clientconn.go +++ b/clientconn.go @@ -283,9 +283,9 @@ func Dial(target string, opts ...DialOption) (*ClientConn, error) { cc.Close() return nil, ErrClientConnTimeout } + // If balancer is nil or balancer.Notify() is nil, ok will be false here. + // The lbWatcher goroutine will not be created. if ok { - // If balancer is nil or balancer.Notify() is nil, ok will false here. - // Then this goroutine will not be created. go cc.lbWatcher() } colonPos := strings.LastIndex(target, ":") @@ -434,13 +434,14 @@ func (cc *ClientConn) resetAddrConn(addr Address, skipWait bool, tearDownErr err if ac.dopts.block && !skipWait { if err := ac.resetTransport(false); err != nil { if err != errConnClosing { - ac.cc.mu.Lock() - delete(ac.cc.conns, ac.addr) - ac.cc.mu.Unlock() + // Tear down ac and delete it from cc.conns. + cc.mu.Lock() + delete(cc.conns, ac.addr) + cc.mu.Unlock() ac.tearDown(err) } if e, ok := err.(transport.ConnectionError); ok && !e.Temporary() { - return e.OriginalError() + return e.Origin() } return err } @@ -452,9 +453,7 @@ func (cc *ClientConn) resetAddrConn(addr Address, skipWait bool, tearDownErr err if err := ac.resetTransport(false); err != nil { grpclog.Printf("Failed to dial %s: %v; please retry.", ac.addr.Addr, err) if err != errConnClosing { - ac.cc.mu.Lock() - delete(ac.cc.conns, ac.addr) - ac.cc.mu.Unlock() + // Keep this ac in cc.conns, to get the reason it's torn down. ac.tearDown(err) } return @@ -475,7 +474,7 @@ func (cc *ClientConn) getTransport(ctx context.Context, opts BalancerGetOptions) // If balancer is nil, there should be only one addrConn available. cc.mu.RLock() for _, ac = range cc.conns { - // Break after the first loop to get the first addrConn. + // Break after the first iteration to get the first addrConn. ok = true break } @@ -747,6 +746,7 @@ func (ac *addrConn) transportMonitor() { ac.mu.Unlock() grpclog.Printf("grpc: addrConn.transportMonitor exits due to: %v", err) if err != errConnClosing { + // Keep this ac in cc.conns, to get the reason it's torn down. ac.tearDown(err) } return @@ -762,8 +762,9 @@ func (ac *addrConn) wait(ctx context.Context, failFast bool) (transport.ClientTr ac.mu.Lock() switch { case ac.state == Shutdown: + err := ac.tearDownErr ac.mu.Unlock() - return nil, ac.tearDownErr + return nil, err case ac.state == Ready: ct := ac.transport ac.mu.Unlock() diff --git a/transport/transport.go b/transport/transport.go index 311158f7..d59e5113 100644 --- a/transport/transport.go +++ b/transport/transport.go @@ -487,18 +487,18 @@ func StreamErrorf(c codes.Code, format string, a ...interface{}) StreamError { // ConnectionErrorf creates an ConnectionError with the specified error description. func ConnectionErrorf(temp bool, e error, format string, a ...interface{}) ConnectionError { return ConnectionError{ - Desc: fmt.Sprintf(format, a...), - temp: temp, - origErr: e, + Desc: fmt.Sprintf(format, a...), + temp: temp, + err: e, } } // ConnectionError is an error that results in the termination of the // entire connection and the retry of all the active streams. type ConnectionError struct { - Desc string - temp bool - origErr error + Desc string + temp bool + err error } func (e ConnectionError) Error() string { @@ -510,14 +510,14 @@ func (e ConnectionError) Temporary() bool { return e.temp } -// OriginalError returns the original error of this connection error. -func (e ConnectionError) OriginalError() error { +// Origin returns the original error of this connection error. +func (e ConnectionError) Origin() error { // Never return nil error here. - // If original error is nil, return itself. - if e.origErr == nil { + // If the original error is nil, return itself. + if e.err == nil { return e } - return e.origErr + return e.err } var (