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.

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.ReadTimeout does not fire for HTTP/2 requests #49837

Open
davecheney opened this issue Nov 29, 2021 · 3 comments
Open

x/net/http2: Server.ReadTimeout does not fire for HTTP/2 requests #49837

davecheney opened this issue Nov 29, 2021 · 3 comments

Comments

@davecheney
Copy link
Contributor

@davecheney davecheney commented Nov 29, 2021

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

$ go version 1.17.3

Does this issue reproduce with the latest release?

yes

What did you do?

package main

import (
        "io"
        "log"
        "net/http"
        "net/http/httptest"
        "strings"
        "time"
)

func handler(w http.ResponseWriter, r *http.Request) {
        log.Println("handler started")
        start := time.Now()
        buf, err := io.ReadAll(r.Body)
        log.Printf("read %d in %v, err: %v", len(buf), time.Since(start), err)
}

type slowReader struct {
        delay time.Duration
        io.Reader
}

func (sr *slowReader) Read(buf []byte) (int, error) {
        time.Sleep(sr.delay)
        return sr.Reader.Read(buf)
}

func main() {
        sv := httptest.NewUnstartedServer(http.HandlerFunc(handler))
        sv.EnableHTTP2 = true
        sv.Config.ReadTimeout = 1 * time.Second
        sv.StartTLS()

        resp, err := sv.Client().Post(sv.URL+"/", "text/plain", &slowReader{
                delay:  5 * time.Second,
                Reader: strings.NewReader("hello, HTTP/2"),
        })
        if err != nil {
                log.Fatal(err)
        }
        defer resp.Body.Close()
}

What did you expect to see?

After 1 second, some error from io.ReadAll in handler indicating the request had timed out during reading.

What did you see instead?

(~/devel/http2bug) % go run main.go
2021/11/29 14:57:45 handler started
2021/11/29 14:57:55 read 13 in 10.00982654s, err: <nil>

The call to io.ReadAll completed in 10 seconds with no error.

@davecheney davecheney added this to the Go1.18 milestone Nov 29, 2021
@davecheney
Copy link
Contributor Author

@davecheney davecheney commented Nov 29, 2021

/cc @aojea

@aojea
Copy link

@aojea aojea commented Nov 29, 2021

This seems to be the expected behavior, I can see that for ReadTimeout is the same as IdleTimeout for http2,

https://github.com/golang/net/blob/d455829e376dcf0ca6aca82a86d093afbcc6a8ee/http2/server.go#L229-L234

and doesn't take the body into consideration

	// ReadTimeout is the maximum duration for reading the entire
	// request, including the body. A zero or negative value means
	// there will be no timeout.
	//
	// Because ReadTimeout does not let Handlers make per-request
	// decisions on each request body's acceptable deadline or
	// upload rate, most users will prefer to use
	// ReadHeaderTimeout. It is valid to use them both.
	ReadTimeout time.Duration

there is also a test that checks that the handler is not affected by the timeout https://github.com/golang/net/blob/d455829e376dcf0ca6aca82a86d093afbcc6a8ee/http2/server_test.go#L3891-L3917

It seems this comment from @bradfitz explains the decision
#14204 (comment)

... anyway, if my understanding is wrong I'm happy to help if I have some guidance 😅

@davecheney
Copy link
Contributor Author

@davecheney davecheney commented Nov 29, 2021

Thanks for finding the link to the previous discussion. IMO if this is the intended behaviour then the comment on ReadTimeout probably needs to be updated.

	// ReadTimeout is the maximum duration for reading the entire
	// request, including the body. A zero or negative value means
	// there will be no timeout.

This isn't correct in the HTTP/2 context. At best, if the user has set WriteTimeout then it will fire at some point and tear down the stream, but ReadTimeout is ineffective in HTTP/2 contexts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants