Issue 70120045: code review 70120045: net/http: add Client.Timeout for end-to-end timeouts (Closed)
Descriptionnet/http: add Client.Timeout for end-to-end timeouts
Fixes Issue 3362
Patch Set 1 #Patch Set 2 : diff -r c9f054b74d7b https://go.googlecode.com/hg/ #Patch Set 3 : diff -r c9f054b74d7b https://go.googlecode.com/hg/ #Patch Set 4 : diff -r c9f054b74d7b https://go.googlecode.com/hg/ #
Total comments: 15
Patch Set 5 : diff -r c9f054b74d7b https://go.googlecode.com/hg/ #
Total comments: 2
Patch Set 6 : diff -r d8b411485c48 https://go.googlecode.com/hg/ #
Total comments: 6
Patch Set 7 : diff -r d8b411485c48 https://go.googlecode.com/hg/ #MessagesTotal messages: 13
Hello golang-codereviews@googlegroups.com (cc: adg@golang.org, dsymonds@golang.org, josharian@gmail.com, n13m3y3r@gmail.com), I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
https://codereview.appspot.com/70120045/diff/60001/src/pkg/net/http/client.go File src/pkg/net/http/client.go (right): https://codereview.appspot.com/70120045/diff/60001/src/pkg/net/http/client.go... src/pkg/net/http/client.go:58: // Timeout optionally specifies the end-to-end timeout for s/optionally// https://codereview.appspot.com/70120045/diff/60001/src/pkg/net/http/client.go... src/pkg/net/http/client.go:68: // method or using Timeout will result in an error. The default "will result in an error" -- document: where will the error pop up? when? https://codereview.appspot.com/70120045/diff/60001/src/pkg/net/http/client.go... src/pkg/net/http/client.go:69: // Transport supports CancelRequest. s/The default Transport/DefaultTransport/ https://codereview.appspot.com/70120045/diff/60001/src/pkg/net/http/client.go... src/pkg/net/http/client.go:154: if c.Transport != nil { Do you need concurrency protection here, now that you're reading c.Transport twice? Or maybe if t := c.Transport; t != nil { return t } https://codereview.appspot.com/70120045/diff/60001/src/pkg/net/http/client.go... src/pkg/net/http/client.go:291: return nil, fmt.Errorf("net/http: Client Transport of type %T doesn't support CancelRequest; Timeout not supported", c.transport()) The error format above uses just "http:" as the prefix instead of "net/http:", and uses "Client.Transport" instead of "Client Transport". I don't care which in each case, but may as well be consistent. https://codereview.appspot.com/70120045/diff/60001/src/pkg/net/http/client.go... src/pkg/net/http/client.go:293: timer = time.AfterFunc(c.Timeout, func() { tr.CancelRequest(req) }) Thinking about DefaultTransport, if req1 completes in time, and req1's conn gets reused for req2, and then the timer fires during req2, will this close req2's conn out from under it? If so, does this also mean that CancelRequest needs extra documentation / constraints to ensure that it behaves well with Timeout? https://codereview.appspot.com/70120045/diff/60001/src/pkg/net/http/client.go... src/pkg/net/http/client.go:331: io.CopyN(ioutil.Discard, resp.Body, 1<<10) // if a short body (common), we'll reuse the conn Is this part of the Timeout change, or something else riding its coattails? (Genuine question.)
Sign in to reply to this message.
PTAL https://codereview.appspot.com/70120045/diff/60001/src/pkg/net/http/client.go File src/pkg/net/http/client.go (right): https://codereview.appspot.com/70120045/diff/60001/src/pkg/net/http/client.go... src/pkg/net/http/client.go:58: // Timeout optionally specifies the end-to-end timeout for On 2014/02/28 23:46:05, josharian wrote: > s/optionally// Done. https://codereview.appspot.com/70120045/diff/60001/src/pkg/net/http/client.go... src/pkg/net/http/client.go:68: // method or using Timeout will result in an error. The default On 2014/02/28 23:46:05, josharian wrote: > "will result in an error" -- document: where will the error pop up? when? Done. https://codereview.appspot.com/70120045/diff/60001/src/pkg/net/http/client.go... src/pkg/net/http/client.go:69: // Transport supports CancelRequest. On 2014/02/28 23:46:05, josharian wrote: > s/The default Transport/DefaultTransport/ Done. In another way. https://codereview.appspot.com/70120045/diff/60001/src/pkg/net/http/client.go... src/pkg/net/http/client.go:154: if c.Transport != nil { On 2014/02/28 23:46:05, josharian wrote: > Do you need concurrency protection here, now that you're reading c.Transport > twice? Or maybe > > if t := c.Transport; t != nil { > return t > } Transport should not be changed concurrently with requests in-flight. That was already the rule. https://codereview.appspot.com/70120045/diff/60001/src/pkg/net/http/client.go... src/pkg/net/http/client.go:291: return nil, fmt.Errorf("net/http: Client Transport of type %T doesn't support CancelRequest; Timeout not supported", c.transport()) On 2014/02/28 23:46:05, josharian wrote: > The error format above uses just "http:" as the prefix instead of "net/http:", > and uses "Client.Transport" instead of "Client Transport". I don't care which in > each case, but may as well be consistent. The net/http package used to be the http package, prior to the pre-Go1 rename. I'm using the proper names now, but I'd rather not fix up all the old ones in this CL. https://codereview.appspot.com/70120045/diff/60001/src/pkg/net/http/client.go... src/pkg/net/http/client.go:293: timer = time.AfterFunc(c.Timeout, func() { tr.CancelRequest(req) }) On 2014/02/28 23:46:05, josharian wrote: > Thinking about DefaultTransport, if req1 completes in time, and req1's conn gets > reused for req2, and then the timer fires during req2, will this close req2's > conn out from under it? > > If so, does this also mean that CancelRequest needs extra documentation / > constraints to ensure that it behaves well with Timeout? No, because the connection can't be re-used unless we hit EOF. And if we hit EOF, we stop the timer. I suppose there's a tiny window where it's possible at the edge, but those sorts of edges are all over HTTP in general--- like during a keep-alive request, you can send at any point or the peer can close at any point. So HTTP code has to be robust anyway. https://codereview.appspot.com/70120045/diff/60001/src/pkg/net/http/client.go... src/pkg/net/http/client.go:331: io.CopyN(ioutil.Discard, resp.Body, 1<<10) // if a short body (common), we'll reuse the conn On 2014/02/28 23:46:05, josharian wrote: > Is this part of the Timeout change, or something else riding its coattails? > (Genuine question.) Unrelated. Just noticed it was bad when I was reading this code again. I could remove it if you want it in a separate CL.
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, josharian@gmail.com (cc: adg@golang.org, dsymonds@golang.org, golang-codereviews@googlegroups.com, n13m3y3r@gmail.com), Please take another look.
Sign in to reply to this message.
A few more nits below, and the race detector is unhappy: `go test -race -run=ClientTimeout net/http`. https://codereview.appspot.com/70120045/diff/60001/src/pkg/net/http/client.go File src/pkg/net/http/client.go (right): https://codereview.appspot.com/70120045/diff/60001/src/pkg/net/http/client.go... src/pkg/net/http/client.go:331: io.CopyN(ioutil.Discard, resp.Body, 1<<10) // if a short body (common), we'll reuse the conn > > Is this part of the Timeout change, or something else riding its coattails? > > (Genuine question.) > > Unrelated. Just noticed it was bad when I was reading this code again. > > I could remove it if you want it in a separate CL. I'd prefer that (with apologies for the overhead). Helps when bisecting, etc. https://codereview.appspot.com/70120045/diff/80001/src/pkg/net/http/client.go File src/pkg/net/http/client.go (right): https://codereview.appspot.com/70120045/diff/80001/src/pkg/net/http/client.go... src/pkg/net/http/client.go:62: // will interrupt the read of the Response.Body if the end Maybe s/the end/EOF/ https://codereview.appspot.com/70120045/diff/80001/src/pkg/net/http/client.go... src/pkg/net/http/client.go:68: // method or Client return errors when attempting to make a s/Client return/Client will return/ or equivalent
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, josharian@gmail.com (cc: adg@golang.org, dsymonds@golang.org, golang-codereviews@googlegroups.com, n13m3y3r@gmail.com), Please take another look.
Sign in to reply to this message.
PTAL. All done, and race fixed. On Fri, Feb 28, 2014 at 4:32 PM, <josharian@gmail.com> wrote: > A few more nits below, and the race detector is unhappy: `go test -race > -run=ClientTimeout net/http`. > > > > https://codereview.appspot.com/70120045/diff/60001/src/ > pkg/net/http/client.go > File src/pkg/net/http/client.go (right): > > https://codereview.appspot.com/70120045/diff/60001/src/ > pkg/net/http/client.go#newcode331 > src/pkg/net/http/client.go:331: io.CopyN(ioutil.Discard, resp.Body, > 1<<10) // if a short body (common), we'll reuse the conn > >> > Is this part of the Timeout change, or something else riding its >> > coattails? > >> > (Genuine question.) >> > > Unrelated. Just noticed it was bad when I was reading this code again. >> > > I could remove it if you want it in a separate CL. >> > > I'd prefer that (with apologies for the overhead). Helps when bisecting, > etc. > > https://codereview.appspot.com/70120045/diff/80001/src/ > pkg/net/http/client.go > File src/pkg/net/http/client.go (right): > > https://codereview.appspot.com/70120045/diff/80001/src/ > pkg/net/http/client.go#newcode62 > src/pkg/net/http/client.go:62: // will interrupt the read of the > Response.Body if the end > Maybe s/the end/EOF/ > > https://codereview.appspot.com/70120045/diff/80001/src/ > pkg/net/http/client.go#newcode68 > src/pkg/net/http/client.go:68: // method or Client return errors when > attempting to make a > s/Client return/Client will return/ or equivalent > > https://codereview.appspot.com/70120045/ >
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=ada6f2d5f99f *** net/http: add Client.Timeout for end-to-end timeouts Fixes Issue 3362 LGTM=josharian R=golang-codereviews, josharian CC=adg, dsymonds, golang-codereviews, n13m3y3r https://codereview.appspot.com/70120045
Sign in to reply to this message.
Message was sent while issue was closed.
This CL appears to have broken the solaris-amd64-solaris11 builder.
Sign in to reply to this message.
Message was sent while issue was closed.
https://codereview.appspot.com/70120045/diff/100001/src/pkg/net/http/client.go File src/pkg/net/http/client.go (right): https://codereview.appspot.com/70120045/diff/100001/src/pkg/net/http/client.g... src/pkg/net/http/client.go:58: // Timeout specifies the end-to-end timeout for requests made Timeout specifies a time limit for requests made by this Client. https://codereview.appspot.com/70120045/diff/100001/src/pkg/net/http/client.g... src/pkg/net/http/client.go:60: // redirects, and reading the response body. The timeout The timer remains running after Get, Head, Post, or Do return and will interrupt reading of the Response.Body.
Sign in to reply to this message.
Message was sent while issue was closed.
Done. Sent https://codereview.appspot.com/70930043 https://codereview.appspot.com/70120045/diff/100001/src/pkg/net/http/client.go File src/pkg/net/http/client.go (right): https://codereview.appspot.com/70120045/diff/100001/src/pkg/net/http/client.g... src/pkg/net/http/client.go:58: // Timeout specifies the end-to-end timeout for requests made On 2014/03/03 04:47:15, adg wrote: > Timeout specifies a time limit for requests made by this Client. Done. https://codereview.appspot.com/70120045/diff/100001/src/pkg/net/http/client.g... src/pkg/net/http/client.go:60: // redirects, and reading the response body. The timeout On 2014/03/03 04:47:15, adg wrote: > The timer remains running after Get, Head, Post, or Do return and will interrupt > reading of the Response.Body. Done.
Sign in to reply to this message.
Message was sent while issue was closed.
Sorry to bring up an old draft; this patch brings up a couple questions about CancelRequest that might be good to answer in the documentation for CancelRequest and Timeout. Thanks! https://codereview.appspot.com/70120045/diff/100001/src/pkg/net/http/client.go File src/pkg/net/http/client.go (right): https://codereview.appspot.com/70120045/diff/100001/src/pkg/net/http/client.g... src/pkg/net/http/client.go:298: tr.CancelRequest(req) Won't CancelRequest's error be "use of closed network connection" and not have Timeout() set correctly? https://codereview.appspot.com/70120045/diff/100001/src/pkg/net/http/client.g... src/pkg/net/http/client.go:331: reqmu.Unlock() Is this lock and unlock necessary because CancelRequest relies on req not changing to be goroutine safe? Should that be in the documentation for CancelRequest or Transport? Right now it says "Clients and Transports are safe for concurrent use by multiple goroutines and for efficiency should only be created once and re-used."
Sign in to reply to this message.
|
