New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net/http: add http1 and http2 Server IdleTimeout #14204

Closed
bradfitz opened this Issue Feb 2, 2016 · 18 comments

Comments

Projects
None yet
6 participants
@bradfitz
Member

bradfitz commented Feb 2, 2016

Add http.Server.IdleTimeout to limit the keep-alive idle time for http1 and http2 servers.

@bradfitz bradfitz self-assigned this Feb 2, 2016

@bradfitz bradfitz added this to the Go1.7 milestone Feb 2, 2016

@gopherbot

This comment has been minimized.

gopherbot commented Feb 3, 2016

CL https://golang.org/cl/19159 mentions this issue.

gopherbot pushed a commit to golang/net that referenced this issue Feb 3, 2016

http2/h2demo: update bug link, add idle conn timeouts
Updates golang/go#14204

Change-Id: Id2598c77e2677a50988c00adc8751a9b87751202
Reviewed-on: https://go-review.googlesource.com/19159
Reviewed-by: Andrew Gerrand <adg@golang.org>
@odeke-em

This comment has been minimized.

Member

odeke-em commented Apr 4, 2016

If I may ask, this is going to entail us adding MaxIdleTimeout and MaxActiveTimeout to Server, right? And also we'll add defaults as you in have in CL 19159, true?
Or should we just expose a new function IdleTimeoutHook so that it takes these durations which if <= 0 means we should use the default durations?

@gopherbot

This comment has been minimized.

gopherbot commented Apr 4, 2016

CL https://golang.org/cl/21491 mentions this issue.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Sep 1, 2016

@odeke-em, the naming here is much easier if we don't consider HTTP/2 and PING frames as activity. Then we'd just call it IdleTimeout and be done with it.

But with HTTP/2, do we care about clients sending a PING or other useless frame every N seconds vs. actually sending real requests every N seconds? If so, we need names for both types of idle (network idle vs no-HTTP-requests idle). Or we define IdleTimeout to mean one thing, and give ReadTimeout the other meaning (see #16958 (comment))?

Opinions welcome. Ideally with sample documentation you'd tell users.

I got as far as:

        // ReadTimeout optionally specifies a maximum duration before                                                                                        
        // timing out reads of request headers. It does not include                                                                                          
        // request bodies. For requests after the first on a                                                                                                 
        // connection, this is timer starts upon receiving the first                                                                                         
        // byte. For keep-alive duration, see IdleTimeout.                                                                                                   
        ReadTimeout  time.Duration

        // IdleTime, is, uh....                                                                                                                              
        IdleTimeout time.Duration

/cc @rhysh @broady @adg @bmizerany

@bradfitz

This comment has been minimized.

Member

bradfitz commented Sep 1, 2016

/cc @prabrisat1 too.

@freeformz

This comment has been minimized.

Contributor

freeformz commented Sep 3, 2016

// IdleTimeout is the maximum time between requests that a connection 
can stay idle. The timeout starts as soon as a connection is started and 
after each request is finished. It is canceled when a request starts.
@bradfitz

This comment has been minimized.

Member

bradfitz commented Sep 3, 2016

So PING frames don't count, then? That's a valid answer.

Now that I think about, we should disable the Transport's TCP keep-alive when HTTP/2 is negotiated, since PING frames and TCP keep-alive packets serve the same purpose of keeping the connection alive from NAT timeouts and whatnot.

@freeformz

This comment has been minimized.

Contributor

freeformz commented Sep 6, 2016

WRT request bodies vs request headers ... That does mean a malicious client could send headers fully and then bodies as slowly as possible. I guess though you would use a context with a timeout or the like to protect against that in the handler?

@freeformz

This comment has been minimized.

Contributor

freeformz commented Sep 6, 2016

That probably complicates code though? Should ReadTimeout be canceled after the request's Body is closed, not when the headers are finished being read?

@bradfitz

This comment has been minimized.

Member

bradfitz commented Sep 6, 2016

WRT request bodies vs request headers ... That does mean a malicious client could send headers fully and then bodies as slowly as possible. I guess though you would use a context with a timeout or the like to protect against that in the handler?

Yes, that's the job of the handler or caller of http.Transport.RoundTrip to guard against. We only give a timeout for headers, since the user isn't in control there.

Should ReadTimeout be canceled after the request's Body is closed, not when the headers are finished being read?

SetReadDeadline is currently only called before a TLS handshake, and before reading request headers, and is never disabled.

So yes, to work similarly to Go 1.0-1.7, we should probably not reset it after the headers and have it apply to the whole request, even if that's slightly odd.

@freeformz

This comment has been minimized.

Contributor

freeformz commented Sep 6, 2016

Can the deadlines (since this is what is currently used) even be reset in the middle of a Read?

@bradfitz

This comment has been minimized.

Member

bradfitz commented Sep 6, 2016

Why would we be in the middle of a read?

@freeformz

This comment has been minimized.

Contributor

freeformz commented Sep 7, 2016

@bradfitz Actually you wouldn't be AFAICT. Not sure why I thought that earlier (in an internal convo about this issue).

gopherbot pushed a commit to golang/net that referenced this issue Sep 30, 2016

http2: in Server, disarm connection's ReadTimeout after first request
Fix a regression from Go 1.5 to Go 1.6: when we introduced automatic
HTTP/2 support in Go 1.6, we never handled Server.ReadTimeout. If a
user set ReadTimeout, the net/http package would set the read deadline
on the connection during the TLS handshake, but then the http2 package
would never disarm it, killing the likely-still-in-use connection
after the timeout period.

This CL changes it to disarm the timeout after the first request
headers, similar to net/http.Server.

Unlike net/http.Server, we don't re-arm it on each idle period,
because the definition of idle is more complicated with HTTP/2.

No tests here for now. Tests will be in the go repo once all the knobs
are in place and this is re-bundled into std, testing both http1 and
http2.

Updates golang/go#16450 (minimal fix for now)
Updates golang/go#14204 (considering a new http1+http2 IdleTimeout knob)

Change-Id: Iaa1570c118efda7dc0a65ba84cd77885699ec8fc
Reviewed-on: https://go-review.googlesource.com/30077
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

@quentinmit quentinmit added the NeedsFix label Oct 7, 2016

@gopherbot

This comment has been minimized.

gopherbot commented Oct 22, 2016

CL https://golang.org/cl/31727 mentions this issue.

gopherbot pushed a commit to golang/net that referenced this issue Oct 23, 2016

http2: add Server.IdleTimeout
Also remove some stale TODOs and finish one of the TODOs, ignoring
more incoming frames when the connection is in GOAWAY mode.

Also, fix independently-broken transport test from a change in std:
https://golang.org/cl/31595

Updates golang/go#14204

Change-Id: Iae8b02248464d613979c30d8f86eacb329cd262f
Reviewed-on: https://go-review.googlesource.com/31727
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot

This comment has been minimized.

gopherbot commented Oct 25, 2016

CL https://golang.org/cl/32024 mentions this issue.

gopherbot pushed a commit that referenced this issue Oct 26, 2016

net/http: add Server.ReadHeaderTimeout, IdleTimeout, document WriteTi…
…meout

Updates #14204
Updates #16450
Updates #16100

Change-Id: Ic283bcec008a8e0bfbcfd8531d30fffe71052531
Reviewed-on: https://go-review.googlesource.com/32024
Reviewed-by: Tom Bergan <tombergan@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot

This comment has been minimized.

gopherbot commented Oct 27, 2016

CL https://golang.org/cl/32230 mentions this issue.

gopherbot pushed a commit to golang/net that referenced this issue Oct 27, 2016

http2: initialize Server.IdleTimeout from http.Server as http1 does
This makes ConfigureServer initialize the http2 Server's IdleTimeout
from the http1 Server configuration, using the same rules as
IdleTimeout in Go 1.8: first use IdleTimeout if specified, else fall
back to the old ReadTimeout.

Updates golang/go#14204

Change-Id: I4dee971f8416ef0cbf99335a199c46355f9ab09d
Reviewed-on: https://go-review.googlesource.com/32230
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Tom Bergan <tombergan@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot

This comment has been minimized.

gopherbot commented Oct 29, 2016

CL https://golang.org/cl/32322 mentions this issue.

@gopherbot

This comment has been minimized.

gopherbot commented Oct 29, 2016

CL https://golang.org/cl/32323 mentions this issue.

gopherbot pushed a commit to golang/net that referenced this issue Oct 29, 2016

http2: change how Server.IdleTimeout is initialized from http.Server
This is an alternate implementation of https://golang.org/cl/32230
which is nicer to the dead code elimination in Go's linker.

The old implementation causes a test in the net/http package to fail:
https://storage.googleapis.com/go-build-log/2c24cf88/linux-amd64_39728ac9.log
... since it caused the cmd/go binary to link in the whole http1 & http2 Server
code, due to the init func and slice which referenced those symbols.

Instead, use an explicit func.

This aso includes the tests meant to be in CL 32230 but which I'd
failed to git add earlier.

Updates golang/go#14204

Change-Id: I13dc138bf0c4df65bc282133ee94036b7f84ab70
Reviewed-on: https://go-review.googlesource.com/32323
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Martin Möhrmann <martisch@uos.de>

@gopherbot gopherbot closed this in 4b130f9 Oct 29, 2016

@golang golang locked and limited conversation to collaborators Oct 29, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.