Skip to content

Commit

Permalink
clientconn: use separate defer blocks
Browse files Browse the repository at this point in the history
Commit 955eb8a ("channelz: cleanup channel registration if Dial
fails (grpc#2733)") moved a defer block earlier in DialContext() to
ensure that cc.Close() was always called.  This defer block also
checks whether the ctx.Done() is true, and if so ensures the context
error is returned.

If the dial options include a timeout, the original context gets
replaced with a new context that has the timeout, and this gets
a catchall `defer cancel()` to go with it.

However, this cancel() now gets called before the cleanup defer
block, so when the latter runs the context is always already cancelled.

Fix by splitting the larger defer block into two parts:
 - The part that does cc.Close() stays near the beginning of the
   method.
 - The part that checks ctx.Done() returns to below the `defer cancel()`
   call, and so gets invoked before it.
  • Loading branch information
daviddrysdale committed Apr 3, 2019
1 parent abdf27e commit 028714f
Showing 1 changed file with 7 additions and 6 deletions.
13 changes: 7 additions & 6 deletions clientconn.go
Original file line number Diff line number Diff line change
Expand Up @@ -135,12 +135,6 @@ func DialContext(ctx context.Context, target string, opts ...DialOption) (conn *
}

defer func() {
select {
case <-ctx.Done():
conn, err = nil, ctx.Err()
default:
}

if err != nil {
cc.Close()
}
Expand Down Expand Up @@ -207,6 +201,13 @@ func DialContext(ctx context.Context, target string, opts ...DialOption) (conn *
ctx, cancel = context.WithTimeout(ctx, cc.dopts.timeout)
defer cancel()
}
defer func() {
select {
case <-ctx.Done():
conn, err = nil, ctx.Err()
default:
}
}()

scSet := false
if cc.dopts.scChan != nil {
Expand Down

0 comments on commit 028714f

Please sign in to comment.