-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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
x/net/http2: http.Server.WriteTimeout does not fire if the http2 stream's window is out of space. #49741
Comments
interestingly, you can increase the tick until 15ms, it always block when it writes 64 blocks. If I divide by 2 the data It seems that something blocks when it has 64x65536 = 4MB, the bytes.Buffer? Line 16 in 2ebe77a
|
I believe that is the stream's window, as the client is not reading from the response stream it will not be triggering window updates to refill the window |
@davecheney you are right, I've found the problem, is the stream window as you say I'll send a patch so you can review |
Change https://golang.org/cl/367154 mentions this issue: |
@mknyszek I see you're landing a bunch of HTTP/2 fixes for 1.17.4. Could I ask you to consider this one for inclusion. Thank you. |
@davecheney It might be a little late, we just met today to go over the backport issues to cherry-pick, and generally we only include those whose changes have already landed at tip. I think this should probably wait until the next minor release (that is in a month), but I'll see if @neild has any opinions on this. If it's severe enough it may warrant trying to get it in this one. |
Nice catch for a subtle bug. CL looks good to me, but as this fix is just landing and the bug is long-standing I think it's too late for 1.17.4. |
Thanks for considering it. I can stick with x/net/http2 til the next minor ships |
(Moving to backlog as part of triage, just because this isn't a new issue in 1.18.) |
(Er, actually it seems like y'all plan to fix this relatively soon, I'm gonna guess in 1.19? Moving to 1.19 milestone.) |
@mknyszek i'd prefer to not have to wait til 1.19 for this to be backported to net/http. Is it possible this could be included in the next x/net/http2 backport into 1.17.6, or at least 1.18.1? I agree this this is an existing issue that has been broken for a long time, but equally, its been broken for a long time and has quite serious impacts for anyone attempting to expose a Go http/2 service on the open internet. We have pretty good control of the client side on copilot, and still see requests with a 10 second WriteTimeout hang for the better part of an hour because of this issue. |
@davecheney The backport for 1.17 will go in sooner, and we'll file a separate issue for that. It'll definitely go into 1.18.1 when the milestone is opened, I just mean that the fix on tip will land in the 1.19 cycle (so when the tree opens in February). @gopherbot Please open a backport issue for Go 1.17. |
Backport issue(s) opened: #49921 (for 1.17), #49922 (for 1.18). Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases. |
@neild @mknyszek @davecheney Because this issue was fixed before go1.18rc1, and we want it in the 1.18.1 release, we should actually just update the vendored x/net in the In that case, the steps would be:
|
@gopherbot Please open a backport issue for Go 1.16. |
Backport issue(s) opened: #50449 (for 1.16) Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases. |
Change https://golang.org/cl/375718 mentions this issue: |
Change https://golang.org/cl/375719 mentions this issue: |
…n random write scheduler The http2 random write scheduler should not queue RST_STREAM frames with the DATA frames, and instead treat them as control frames. There can be deadlock situations if data frames block the queue, because if the sender wants to close the stream it sends an RST frame, but if the client is not draining the queue, the RST frame is stuck and the sender is not able to finish. For golang/go#49741 Updates golang/go#50449 Change-Id: I0940a76d1aad95f1c4d3856e4d79cf5ce2a78ff2 Reviewed-on: https://go-review.googlesource.com/c/net/+/367154 Trust: Dave Cheney <[email protected]> Reviewed-by: Damien Neil <[email protected]> Trust: Damien Neil <[email protected]> Run-TryBot: Damien Neil <[email protected]> TryBot-Result: Gopher Robot <[email protected]> (cherry picked from commit 04296fa) Reviewed-on: https://go-review.googlesource.com/c/net/+/375718 Run-TryBot: Carlos Amedee <[email protected]> Trust: Dmitri Shuralyov <[email protected]>
…n random write scheduler The http2 random write scheduler should not queue RST_STREAM frames with the DATA frames, and instead treat them as control frames. There can be deadlock situations if data frames block the queue, because if the sender wants to close the stream it sends an RST frame, but if the client is not draining the queue, the RST frame is stuck and the sender is not able to finish. For golang/go#49741 Updates golang/go#49921 Change-Id: I0940a76d1aad95f1c4d3856e4d79cf5ce2a78ff2 Reviewed-on: https://go-review.googlesource.com/c/net/+/367154 Trust: Dave Cheney <[email protected]> Reviewed-by: Damien Neil <[email protected]> Trust: Damien Neil <[email protected]> Run-TryBot: Damien Neil <[email protected]> TryBot-Result: Gopher Robot <[email protected]> (cherry picked from commit 04296fa) Reviewed-on: https://go-review.googlesource.com/c/net/+/375719 Run-TryBot: Carlos Amedee <[email protected]> Trust: Dmitri Shuralyov <[email protected]>
The original test case given above still does not timeout after a second when building with go1.17.6, even though the commit that fixed golang.org/x/net was cherry picked. I've seen the failure with both Darwin and Linux builds. I can get the test case to timeout correctly if I use the latest golang.org/x/net package and modify main with the ConfigureServer call: package main
import (
"io"
"log"
"net/http"
"net/http/httptest"
"strings"
"time"
"golang.org/x/net/http2"
)
func handler(w http.ResponseWriter, r *http.Request) {
data := strings.Repeat("x", 1<<16)
tick := time.NewTicker(1 * time.Millisecond)
defer tick.Stop()
for {
select {
case <-tick.C:
n, err := io.WriteString(w, data)
log.Printf("wrote %d, err %v", n, err)
if err != nil {
return
}
case <-r.Context().Done():
log.Printf("context cancelled")
return
}
}
}
func main() {
sv := httptest.NewUnstartedServer(http.HandlerFunc(handler))
err := http2.ConfigureServer(sv.Config, &http2.Server{})
if err != nil {
log.Fatal(err)
}
sv.EnableHTTP2 = true
sv.Config.WriteTimeout = 1 * time.Second
sv.StartTLS()
resp, err := sv.Client().Get(sv.URL + "/")
if err != nil {
log.Fatal(err)
}
defer resp.Body.Close()
select {} // block forever
} The go.mod:
But what's interesting is I can also get the hung write to timeout correctly without the golang.org/x/net package, but I had |
Reopening. CC @neild |
I've found the problem, there is an inconsistency on the default writeScheduler chosen: The fix for the WriteTimeout issue was in the Lines 4114 to 4118 in a5c0b19
However, when http2 is configured automatically, it uses a different WriteScheduler, that may have the same issue than the other WriteScheduler Lines 3296 to 3311 in a5c0b19
So, I think that there are 2 things to fix here:
@neild @ianlancetaylor I can take on this if you want. |
Change https://golang.org/cl/382014 mentions this issue: |
Submitted a patch for fixing the scheduler http2NewPriorityWriteScheduler bug. What should be the default scheduler EDIT It seems it has to be the PriorityWriteScheduler based on #18318 |
The http2 random write scheduler should not queue RST_STREAM frames with the DATA frames, and instead treat them as control frames. There can be deadlock situations if data frames block the queue, because if the sender wants to close the stream it sends an RST frame, but if the client is not draining the queue, the RST frame is stuck and the sender is not able to finish. Fixes golang/go#49741 Change-Id: I0940a76d1aad95f1c4d3856e4d79cf5ce2a78ff2 Reviewed-on: https://go-review.googlesource.com/c/net/+/367154 Trust: Dave Cheney <[email protected]> Reviewed-by: Damien Neil <[email protected]> Trust: Damien Neil <[email protected]> Run-TryBot: Damien Neil <[email protected]> TryBot-Result: Gopher Robot <[email protected]>
The http2 priority write scheduler should not queue RST_STREAM frames with the DATA frames, and instead treat them as control frames. There can be deadlock situations if data frames block the queue, because if the sender wants to close the stream it sends an RST frame, but if the client is not draining the queue, the RST frame is stuck and the sender is not able to finish. Fixes golang/go#49741 Signed-off-by: Antonio Ojea <[email protected]> Change-Id: Ie4462603380039f7aba8f701fe810d1d84376efa Reviewed-on: https://go-review.googlesource.com/c/net/+/382014 Reviewed-by: Ian Lance Taylor <[email protected]> Reviewed-by: Damien Neil <[email protected]> Run-TryBot: Dave Cheney <[email protected]> TryBot-Result: Gopher Robot <[email protected]>
…n random write scheduler The http2 random write scheduler should not queue RST_STREAM frames with the DATA frames, and instead treat them as control frames. There can be deadlock situations if data frames block the queue, because if the sender wants to close the stream it sends an RST frame, but if the client is not draining the queue, the RST frame is stuck and the sender is not able to finish. For golang/go#49741 Updates golang/go#49921 Change-Id: I0940a76d1aad95f1c4d3856e4d79cf5ce2a78ff2 Reviewed-on: https://go-review.googlesource.com/c/net/+/367154 Trust: Dave Cheney <[email protected]> Reviewed-by: Damien Neil <[email protected]> Trust: Damien Neil <[email protected]> Run-TryBot: Damien Neil <[email protected]> TryBot-Result: Gopher Robot <[email protected]> (cherry picked from commit 04296fa82e83b85317bd93ad50dd00460d6d7940) Reviewed-on: https://go-review.googlesource.com/c/net/+/375719 Run-TryBot: Carlos Amedee <[email protected]> Trust: Dmitri Shuralyov <[email protected]>
…n random write scheduler The http2 random write scheduler should not queue RST_STREAM frames with the DATA frames, and instead treat them as control frames. There can be deadlock situations if data frames block the queue, because if the sender wants to close the stream it sends an RST frame, but if the client is not draining the queue, the RST frame is stuck and the sender is not able to finish. For golang/go#49741 Updates golang/go#50449 Change-Id: I0940a76d1aad95f1c4d3856e4d79cf5ce2a78ff2 Reviewed-on: https://go-review.googlesource.com/c/net/+/367154 Trust: Dave Cheney <[email protected]> Reviewed-by: Damien Neil <[email protected]> Trust: Damien Neil <[email protected]> Run-TryBot: Damien Neil <[email protected]> TryBot-Result: Gopher Robot <[email protected]> (cherry picked from commit 04296fa) Reviewed-on: https://go-review.googlesource.com/c/net/+/375718 Run-TryBot: Carlos Amedee <[email protected]> Trust: Dmitri Shuralyov <[email protected]>
…n random write scheduler The http2 random write scheduler should not queue RST_STREAM frames with the DATA frames, and instead treat them as control frames. There can be deadlock situations if data frames block the queue, because if the sender wants to close the stream it sends an RST frame, but if the client is not draining the queue, the RST frame is stuck and the sender is not able to finish. For golang/go#49741 Updates golang/go#50449 Change-Id: I0940a76d1aad95f1c4d3856e4d79cf5ce2a78ff2 Reviewed-on: https://go-review.googlesource.com/c/net/+/367154 Trust: Dave Cheney <[email protected]> Reviewed-by: Damien Neil <[email protected]> Trust: Damien Neil <[email protected]> Run-TryBot: Damien Neil <[email protected]> TryBot-Result: Gopher Robot <[email protected]> (cherry picked from commit 04296fa) Reviewed-on: https://go-review.googlesource.com/c/net/+/375718 Run-TryBot: Carlos Amedee <[email protected]> Trust: Dmitri Shuralyov <[email protected]>
The http2 priority write scheduler should not queue RST_STREAM frames with the DATA frames, and instead treat them as control frames. There can be deadlock situations if data frames block the queue, because if the sender wants to close the stream it sends an RST frame, but if the client is not draining the queue, the RST frame is stuck and the sender is not able to finish. Fixes golang/go#49741 Signed-off-by: Antonio Ojea <[email protected]> Change-Id: Ie4462603380039f7aba8f701fe810d1d84376efa Reviewed-on: https://go-review.googlesource.com/c/net/+/382014 Reviewed-by: Ian Lance Taylor <[email protected]> Reviewed-by: Damien Neil <[email protected]> Run-TryBot: Dave Cheney <[email protected]> TryBot-Result: Gopher Robot <[email protected]>
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
yes
What operating system and processor architecture are you using (
go env
)?What did you do?
This program creates a HTTP2 service which fills the HTTP/2 stream's response window by failing to consume any data from the response body. The write deadline should fire after 1 second and terminate the response. It does not.
What did you expect to see?
If the
tick
rate is increased to 100ms, the write timeout firesWhat did you see instead?
If
tick
is left at 1 ms, permitting it to fill the stream's response window,handler
blocks, no timeout is reported on either the client or the server side./cc @neild @bradfitz
The text was updated successfully, but these errors were encountered: