code review 68150045: net/http: add Transport.TLSHandshakeTimeout; set it by ...

net/http: add Transport.TLSHandshakeTimeout; set it by default

Update Issue 3362

[bradfitz, 2014-02-24 23:28:40]

Hello agl@chromium.org (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[agl1, 2014-02-25 00:05:59]

LGTM

https://codereview.appspot.com/68150045/diff/40001/src/pkg/net/http/transport.go
File src/pkg/net/http/transport.go (right):

https://codereview.appspot.com/68150045/diff/40001/src/pkg/net/http/transport...
src/pkg/net/http/transport.go:563: timeoutFired := timer != nil && !timer.Stop()
It's unclear why you worry about the return value of timer.Stop(). Why not just
call it and send to errc? If the timeout has already fired then it'll have
enqueued its error.

[bradfitz, 2014-02-25 00:12:27]

Isn't there a race window where handshake succeeds first and then timer
also fires before being canceled , closing the conn already successfully
returned?
 On Feb 24, 2014 4:06 PM, <agl@golang.org> wrote:

> LGTM
>
>
> https://codereview.appspot.com/68150045/diff/40001/src/
> pkg/net/http/transport.go
> File src/pkg/net/http/transport.go (right):
>
> https://codereview.appspot.com/68150045/diff/40001/src/
> pkg/net/http/transport.go#newcode563
> src/pkg/net/http/transport.go:563: timeoutFired := timer != nil &&
> !timer.Stop()
> It's unclear why you worry about the return value of timer.Stop(). Why
> not just call it and send to errc? If the timeout has already fired then
> it'll have enqueued its error.
>
> https://codereview.appspot.com/68150045/
>

[agl1, 2014-02-25 00:16:17]

https://codereview.appspot.com/68150045/diff/40001/src/pkg/net/http/transport.go
File src/pkg/net/http/transport.go (right):

https://codereview.appspot.com/68150045/diff/40001/src/pkg/net/http/transport...
src/pkg/net/http/transport.go:558: plainConn.Close()
Ah, I see. You're right, in which case I think the plainConn.Close() is in the
wrong place. It should be in the original goroutine, where <-errc is checked for
being non-nil.

[bradfitz, 2014-02-25 00:56:02]

Hello agl@chromium.org, agl@golang.org (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[bradfitz, 2014-02-25 00:56:18]

PTAL. Simplified.



On Mon, Feb 24, 2014 at 4:56 PM, <bradfitz@golang.org> wrote:

> Hello agl@chromium.org, agl@golang.org (cc:
> golang-codereviews@googlegroups.com),
>
> Please take another look.
>
>
> https://codereview.appspot.com/68150045/
>

[agl1, 2014-02-25 15:20:43]

LGTM, thanks!

[bradfitz, 2014-02-25 16:08:16]

*** Submitted as https://code.google.com/p/go/source/detail?r=c8edfe4ddd5a ***

net/http: add Transport.TLSHandshakeTimeout; set it by default

Update Issue 3362

LGTM=agl
R=agl
CC=golang-codereviews
https://codereview.appspot.com/68150045

[gobot, 2014-02-25 16:30:57]

This CL appears to have broken the windows-amd64 builder.