net/http: WriteTimeout not reset in http2 #18437

Closed
pnelson opened this Issue Dec 27, 2016 · 9 comments

Comments

Projects
None yet
5 participants
@pnelson

pnelson commented Dec 27, 2016

What version of Go are you using (go version)?

$ go version
go version go1.8beta2 linux/amd64

What operating system and processor architecture are you using (go env)?

Ubuntu 16.04 amd64

$ go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/pnelson"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build506631447=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
PKG_CONFIG="pkg-config"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"

What did you do?

I wrote a simple HTTP/2 server with a WriteTimeout set to reproduce.

https://github.com/pnelson/go-timeouts-test

Using a load testing tool for 5 seconds yields latencies to the point of client timeout. Also reproducible in browser by refreshing the page a few times.

What did you expect to see?

Request latency to be well under 2s.

What did you see instead?

Request latency greater than 30s (client timeout) after a few requests from the same client, presumably due to connection reuse and timeout not being reset.

This sounds a bit similar to an older issue with ReadTimeout, #16450.

@vcabbage

This comment has been minimized.

Show comment
Hide comment
@vcabbage

vcabbage Dec 27, 2016

Member

@pnelson and I chatted about this issue on Slack. As far as I can tell, HTTP1 write deadlines are reset here, but since HTTP2 takes a different path the deadline is never reset.

The patch below resolved the issue for me. (It may well not be the correct fix, but I think it narrows down the issue.)

index 2e0b3c905a..c9aa12948f 100644
--- a/src/net/http/h2_bundle.go
+++ b/src/net/http/h2_bundle.go
@@ -4323,6 +4323,9 @@ func (sc *http2serverConn) processHeaders(f *http2MetaHeadersFrame) error {
        if sc.hs.ReadTimeout != 0 {
                sc.conn.SetReadDeadline(time.Time{})
        }
+       if sc.hs.WriteTimeout != 0 {
+               sc.conn.SetWriteDeadline(time.Now().Add(sc.hs.WriteTimeout))
+       }

        go sc.runHandler(rw, req, handler)
        return nil
Member

vcabbage commented Dec 27, 2016

@pnelson and I chatted about this issue on Slack. As far as I can tell, HTTP1 write deadlines are reset here, but since HTTP2 takes a different path the deadline is never reset.

The patch below resolved the issue for me. (It may well not be the correct fix, but I think it narrows down the issue.)

index 2e0b3c905a..c9aa12948f 100644
--- a/src/net/http/h2_bundle.go
+++ b/src/net/http/h2_bundle.go
@@ -4323,6 +4323,9 @@ func (sc *http2serverConn) processHeaders(f *http2MetaHeadersFrame) error {
        if sc.hs.ReadTimeout != 0 {
                sc.conn.SetReadDeadline(time.Time{})
        }
+       if sc.hs.WriteTimeout != 0 {
+               sc.conn.SetWriteDeadline(time.Now().Add(sc.hs.WriteTimeout))
+       }

        go sc.runHandler(rw, req, handler)
        return nil
@gopherbot

This comment has been minimized.

Show comment
Hide comment

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

@gopherbot

This comment has been minimized.

Show comment
Hide comment

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

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

http2: clear WriteTimeout in Server
Current handling of WriteTimeout for http2 does not
extend the timeout on new streams. Disable the WriteTimeout
in http2 for 1.8 release.

Fixes test added in https://golang.org/cl/34723

Updates golang/go#18437

Change-Id: I366899fb4ff2e740610cad71e004141d092699a2
Reviewed-on: https://go-review.googlesource.com/34724
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

gopherbot pushed a commit that referenced this issue Dec 30, 2016

net/http: add test for http2 Server WriteTimeout
Current handling of WriteTimeout for http2 does not
extend the timeout on new streams. Disable the WriteTimeout
in http2 for 1.8 release.

Updates #18437

Change-Id: I20480432ab176f115464434645defb56ebeb6ece
Reviewed-on: https://go-review.googlesource.com/34723
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.

Show comment
Hide comment

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

@gopherbot

This comment has been minimized.

Show comment
Hide comment

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

@gopherbot

This comment has been minimized.

Show comment
Hide comment

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

gopherbot pushed a commit that referenced this issue Dec 31, 2016

net/http: update bundled http2 for Server WriteTimeout change
Updates http2 to x/net/http2 git rev 8fd7f25 for:

    http2: clear WriteTimeout in Server
    https://golang.org/cl/34724

And un-skip the new test. (The new test is a slow test, anyway, so
won't affect builders or all.bash, but I verified it now passes.)

Updates #18437

Change-Id: Ia91ae702edfd23747a9d6b61da284a5a957bfed3
Reviewed-on: https://go-review.googlesource.com/34729
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
Reviewed-by: Kale B <kale@lemnisys.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Jan 4, 2017

Contributor

@bradfitz, should this be closed?

Contributor

rsc commented Jan 4, 2017

@bradfitz, should this be closed?

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Jan 4, 2017

Member

No, it should be fixed properly later. There's a pending CL for 1.9 to respect the field. Currently the submitted workaround just makes the field ignored for 1.8.

Member

bradfitz commented Jan 4, 2017

No, it should be fixed properly later. There's a pending CL for 1.9 to respect the field. Currently the submitted workaround just makes the field ignored for 1.8.

@rsc rsc added this to the Go1.9 milestone Jan 4, 2017

@rsc rsc added the NeedsFix label Jan 4, 2017

gopherbot pushed a commit that referenced this issue Apr 6, 2017

net/http: add tests for http2 Server WriteTimeout enforcement per stream
Updates #18437

Change-Id: Iaa8a35d18eca8be24763dd151ad9e324ecbf7f7b
Reviewed-on: https://go-review.googlesource.com/34726
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

gopherbot pushed a commit to golang/net that referenced this issue Apr 6, 2017

http2: enforce write deadline per stream
If the writeDeadline elapses RST_STREAM is sent with ErrCodeInternal.

Fixes tests added in https://golang.org/cl/34726

Updates golang/go#18437 (fixes once it's bundled into net/http)

Change-Id: I84e4f76753963c29cd3c06730d6bfbb7f1ee6051
Reviewed-on: https://go-review.googlesource.com/34727
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

lparth added a commit to lparth/go that referenced this issue Apr 13, 2017

net/http: add tests for http2 Server WriteTimeout enforcement per stream
Updates #18437

Change-Id: Iaa8a35d18eca8be24763dd151ad9e324ecbf7f7b
Reviewed-on: https://go-review.googlesource.com/34726
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot

This comment has been minimized.

Show comment
Hide comment

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

@gopherbot gopherbot closed this in 0d3143e Apr 25, 2017

c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018

http2: clear WriteTimeout in Server
Current handling of WriteTimeout for http2 does not
extend the timeout on new streams. Disable the WriteTimeout
in http2 for 1.8 release.

Fixes test added in https://golang.org/cl/34723

Updates golang/go#18437

Change-Id: I366899fb4ff2e740610cad71e004141d092699a2
Reviewed-on: https://go-review.googlesource.com/34724
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018

http2: enforce write deadline per stream
If the writeDeadline elapses RST_STREAM is sent with ErrCodeInternal.

Fixes tests added in https://golang.org/cl/34726

Updates golang/go#18437 (fixes once it's bundled into net/http)

Change-Id: I84e4f76753963c29cd3c06730d6bfbb7f1ee6051
Reviewed-on: https://go-review.googlesource.com/34727
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

@golang golang locked and limited conversation to collaborators Apr 25, 2018

jasonwbarnett pushed a commit to jasonwbarnett/fileserver that referenced this issue Jul 11, 2018

net/http: update bundled x/net/http2
This updates the bundled http2 package from git rev
5602c733f70afc6dcec6766be0d5034d4c4f14de of the x/net repo for:

  http2: Use NO_ERROR instead of CANCEL when responding before the request is finished
  https://golang.org/cl/40630

  http2: enforce write deadline per stream
  https://golang.org/cl/34727

Updates golang/go#19948
Fixes golang/go#18437

Change-Id: I14500476e91551fa8f27a1aeb8ae3cac9600b74c
Reviewed-on: https://go-review.googlesource.com/41753
Reviewed-by: Kale Blankenship <kale@lemnisys.com>
Reviewed-by: Tom Bergan <tombergan@google.com>
Run-TryBot: Kale Blankenship <kale@lemnisys.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.