Skip to content
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.

Sign up for GitHub

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

Jump to bottom

net/http: http.Server.ReadTimeout never reset in http2 #16450

Closed
grahamking opened this issue Jul 20, 2016 · 6 comments
Closed

net/http: http.Server.ReadTimeout never reset in http2 #16450

grahamking opened this issue Jul 20, 2016 · 6 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Go1.8

Comments

@grahamking
Copy link

  1. What version of Go are you using (go version)?
    go1.7rc2
    go1.6.3
    go1.6 if you use http2 (custom Transport without ExpectContinueTimeout)
  2. What operating system and processor architecture are you using (go env)?
    linux_amd64
  3. What did you do?

https://play.golang.org/p/7nC5EIVRCq (change my_cert, my_key and my_host)
Set a ReadTimeout on http.Server. Make several requests to the server using the same client (so that the TCP conn is re-used). With http2 program exits with 'unexpected EOF' after ReadTimeout.

  1. What did you expect to see? 5. What did you see instead?

Program should run forever.

This works:

GODEBUG=http2client=0 go run the_file.go

This fails:

go run the_file.go

Program exits after ReadTimeout because the TCP connection hits it's deadline, is closed. This means that in http2 ReadTimeout applies not to a single request but to the keep-alive connection.

The deadline is set here during TLS handshake: https://github.com/golang/go/blob/master/src/net/http/server.go#L1501-L1503

In http1 case the deadline is reset every request: https://github.com/golang/go/blob/master/src/net/http/server.go#L743-L751

In http2 it is never reset. I suspect the solution is to reset the deadline here: https://github.com/golang/net/blob/master/http2/server.go#L579-L602

There is a similar problem with WriteTimeout. The example program will hang after WriteTimeout.

This was fixed for http1 in #4676.
@bradfitz bradfitz self-assigned this Jul 20, 2016
@quentinmit quentinmit changed the title http.Server.ReadTimeout never reset in http2 net/http2: http.Server.ReadTimeout never reset in http2 Jul 20, 2016
@quentinmit quentinmit added this to the Go1.8 milestone Jul 20, 2016
@bradfitz bradfitz changed the title net/http2: http.Server.ReadTimeout never reset in http2 net/http: http.Server.ReadTimeout never reset in http2 Jul 21, 2016
@bradfitz
Copy link
Contributor

I haven't had time to investigate this yet, but if this isn't a regression from Go 1.6, it's hard to justify fixing it this late in the Go 1.7 cycle.

@bradfitz
Copy link
Contributor

(I do admit that it's a regression from Go 1.5, but I'm surprised nobody has mentioned it in the last year)

@grahamking
Copy link
Author

I agree it's strange that no-one else has mentioned it, and I did do a fair amount of searching. If you reproduce it, maybe 1.7 could add a documentation note that those timeouts are not supported on http/2 connections?

@bradfitz bradfitz mentioned this issue Aug 18, 2016
Closed
@bradfitz bradfitz mentioned this issue Sep 1, 2016
Closed
gopherbot pushed a commit to golang/net that referenced this issue Sep 30, 2016
@bradfitz
http2: in Server, disarm connection's ReadTimeout after first request
6972930 
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>
@bradfitz bradfitz added the Testing An issue that has been verified to require only test changes, not just a test failure. label Sep 30, 2016
@bradfitz
Copy link
Contributor

Bug is fixed in x/net/http2 now, but keeping this open until we figure out #14204 (Server.IdleTimeout) and add tests.

@quentinmit quentinmit added NeedsFix The path to resolution is known, but the work has not been done. Blocked labels Oct 7, 2016
@gopherbot
Copy link
Contributor

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

gopherbot pushed a commit that referenced this issue Oct 26, 2016
@bradfitz
net/http: add Server.ReadHeaderTimeout, IdleTimeout, document WriteTi…
c7e0dda 
…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>
@bradfitz
Copy link
Contributor

#14204 is sufficiently complete. Closing this one.

@pnelson pnelson mentioned this issue Dec 27, 2016
Closed
@pwaller pwaller mentioned this issue Feb 10, 2017
Closed
@mholt mholt mentioned this issue Feb 21, 2017
Closed
@oliverpool oliverpool mentioned this issue Feb 28, 2017
Closed
@golang golang locked and limited conversation to collaborators Oct 27, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Go1.8
Development

No branches or pull requests
4 participants
@bradfitz @quentinmit @grahamking @gopherbot