net/http: Default{Client,Transport} need better timeout support when used with connection pooling/redirects

[balasanjay]

With the new SetDeadline style methods on net.Conns, there is no clean way to set
timeouts for http requests, while taking into account redirects and connection pooling.

See https://groups.google.com/forum/?fromgroups#!topic/golang-nuts/-DE6-aLttM4 for
discussion.

[ianlancetaylor]

Comment 1:

Labels changed: added priority-later, packagechange, removed priority-triage.

[rsc]

Comment 2:

At least that was true of the old way too.

[mreiferson]

Comment 3:

heres one way to do it (although admittedly it is not zestfully *clean*)
https://github.com/mreiferson/go-httpclient

[rsc]

Comment 4:

Status changed to Accepted.

[rsc]

Comment 5:

Labels changed: added go1.1maybe.

[stevvooe]

Comment 6:

Is there any activity on this issue? I'd like to volunteer my time if it's needed. I
would consider this a major stability bug for long running services that make http
requests. A solid http client with timeout support should be considered a critical part
of a "batteries included" standard library.

[remyoudompheng]

Comment 7:

You are of course welcome if you propose a fix that suitably addresses the issue.

[bradfitz]

Comment 8:

There are at least three timeouts that make sense.  We may even want all three:
// HeaderTimeout is the amount of time to wait to get a response header
// after writing the request.  It does not include the response body.
// Zero means no timeout.
HeaderTimeout time.Duration
// ResponseTimeout is like HeaderTimeout, but includes the time to transmit
// the entire body, regardless of size.  If the server is still sending
// the body as ResponseTimeout is hit, the body is interrupted and a Read
// will result in ErrTimeout. 
ResponseTimeout time.Duration
// InactivityTimeout is the amount of time to get any data from the server
// before considering it timed out. Unlike HeaderTimeout and ResponseTimeout,
// this timer resets every time a byte of data is received from the server,
// so a slow trickle of data could take a large amount of time. 
// Zero means no timeout.
InactivityTimeout time.Duration

Comment 9:

How about allowing the application to set the read deadline on the connection?  If an
application can set the read deadline, then the application can easily implement the
three timeouts suggested by Brad.

[bradfitz]

Comment 10:

You can already do that.
I thought this bug was about making the commonly-requested timeout use cases easy.

[stevvooe]

Comment 11:

The common use case is probably centered around what bradfitz has labeled as the
ResponseTimeout. HeaderTimeout and InactivityTimeout seem to be relatively uncommon, and
would require that a particular request has claimed a connection as its own out of a
pool until the corresponding timeout has passed or the request has completed. Its not
unrealistic to expect applications that rely on this level of detail to implement their
own RoundTripper, as connection pooling may not be as critical for APIs that have
disparate requirements around the timing of header responses and byte level responses.
The way I envisioned this was to simply have the timeout based on the total running time
of a call to Tranport.RoundTrip, including any redirects. This is relatively
straightforward and should meet the needs of most applications.

Comment 12:

HeaderTimeout and InactivityTimeout are bounded by the lifetime of a round trip. There's
no need for a request to make a claim on the connection beyond the claim made by the
round trip.
Streaming APIs like Twitter's are a use case for HeaderTimeout and InactivityTimeout.
With these APIs, the client receives a stream of data and keep-alive messages in the
response. The header timeout detects problems connecting to the server. The inactivity
timeout detects the case where the client is not receiving data or keep-alive messages.
The read deadline can be set via the Transport Dial function, but not in ways that the
application might want.  For example, it's not possible to implement ResponseTimeout
when a connection is reused.

[rsc]

Comment 13:

The three values Brad suggested seem fine to me. I would s/Inactivity/Idle/.

[bradfitz]

Comment 14:

I actually don't like the name ResponseTimeout anymore, because we name the result
parameter from Do/Get/Post (*Response, error), and I don't want people assuming that
ResponseTimeout means that Response.
BodyTimeout doesn't work well as an alternative to the name ResponseTimeout, because it
implies that it's only for a body, not the headers + an optional body.
So I'm leaning towards shortening "ResponseTimeout" to just Timeout.
So we'll have:
1) Timeout (the whole thing)
2) HeaderTimeout (just the headers)
3) IdleTimeout (what I called InactivityTImeout above)
That should be sufficient.
If we really need more control later, we could provide a method on *Response (assuming
you get a *Response after the HeaderTimeout) to plumb through a deadline to the
underlying connection, but I don't think it will be necessary and I don't want to do it
now.  I recognize that 1) and 3) would be unnecessary if we had 2) and a *Response
SetDeadline method, but I'd prefer to provide solutiosn to the common use cases, rather
than provide a bunch of mechanisms.
Any final thoughts or objections before I start writing this?

[tgulacsi]

Comment 15:

LGTM