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: Server doesn't respect client's flow control #16498

Open
bradfitz opened this Issue Jul 26, 2016 · 3 comments

Comments

Projects
None yet
3 participants
@bradfitz
Member

bradfitz commented Jul 26, 2016

Go's http2.Server doesn't respect the client's flow control.

It accounts for it partly, and enforces the other direction (that the client can't send too much to the server), but does not ever deduct its own credit when sending DATA frames, and doesn't wait for flow control when doing so.

In the process of fixing #16481 I had expected a test like this to hang:

        body := strings.Repeat("a", 16<<10)
        st := newServerTester(t, func(w http.ResponseWriter, r *http.Request) {
                println(r.RemoteAddr)
                // Write out a 16KB body which the client won't ever read.                                                                                                   
                io.WriteString(w, body)
        }, optOnlyServer)
        defer st.Close()

        tr := &Transport{TLSClientConfig: tlsConfigInsecure}
        defer tr.CloseIdleConnections()

        // Verify we can do this over 4 times without hanging:                                                                                                               
        for i := 0; i < 10; i++ {
                println(i)
                req, _ := http.NewRequest("GET", st.ts.URL, nil)
                res, err := tr.RoundTrip(req)
                if err != nil {
                        t.Fatal(err)
                }
                res.Body.Close()
                println(i, "done")
        }

But it didn't.

Add a test like that back once the http2.Server respects flow control.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Oct 22, 2016

I'm not going to get this done in time. Punting to Go 1.9. It hasn't mattered so far, and it doesn't matter in practice because client's typically just give servers effectively-infinite flow control. Clients don't typically throttle servers. Servers would probably tire of that and hang up on them anyway if they did.

@tombergan

This comment has been minimized.

Contributor

tombergan commented Nov 23, 2016

Go's http2.Server doesn't respect the client's flow control.

Probably a dumb question ... doesn't the code below respect the client's flow control?
https://github.com/golang/net/blob/1c5acb2c2d1df0409de556978dd628d90aee5cb3/http2/writesched.go#L108
https://github.com/golang/net/blob/1c5acb2c2d1df0409de556978dd628d90aee5cb3/http2/writesched.go#L119
https://github.com/golang/net/blob/1c5acb2c2d1df0409de556978dd628d90aee5cb3/http2/writesched.go#L148

There's also this test.

Assuming no bugs, I'm not sure why the test from the Jul 25 comment is expected to hang. The client (correctly) sends a RST_STREAM and WINDOW_UPDATE when it closes the response body without reading the full response. The client also sends a WINDOW_UPDATE when it receives DATA for a previously-closed stream. This means the server will have a full flow-control window at the start of each request.

Also note that the test uses very large window sizes relative to the tiny 16KB responses in the test (see log snippet below). And, even if the response bodies were larger, the client will usually send RST_STREAM (due to resp.Body.Close()) before the server has a chance to send more than a few KB on the wire.

http2: server processing setting [INITIAL_WINDOW_SIZE = 4194304]
http2: server read frame WINDOW_UPDATE len=4 (conn) incr=1073741824

@bradfitz bradfitz modified the milestones: Go1.10, Go1.9 Jun 14, 2017

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jun 14, 2017

Maybe it does? I forget. Too late for Go 1.9 in any case.

I do recall that one of the 4 flow control directions had some omission.

@bradfitz bradfitz modified the milestones: Go1.10, Go1.11 Dec 4, 2017

@bradfitz bradfitz modified the milestones: Go1.11, Go1.12 Jul 11, 2018

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