net/http: http.Client Timeout incompatible with writable bodies #31391

drigz · 2019-04-10T19:41:20Z

As part of #26937, Transport started returning writable bodies for 101 Switching Protocol responses. This is great, thank you!

However, if you have a non-zero Timeout on your http.Client, you can't use this, because of the following lines from client.go:

	if !deadline.IsZero() {
		resp.Body = &cancelTimerBody{
			stop:          stopTimer,
			rc:            resp.Body,
			reqDidTimeout: didTimeout,
		}
	}

This wraps the writable body in a non-writable body.

Go version: 1.12
Observed behavior: res.Body.(io.ReadWriteCloser) fails when client.Timeout > 0.
Expected behavior: timeouts and writeable bodies are compatible.

Although we have some streaming requests, we would like to set a long (eg one hour) timeout on our requests to prevent resource exhaustion. #30876 suggests that we can't even do this manually with contexts.

The text was updated successfully, but these errors were encountered:

agnivade · 2019-04-11T04:36:24Z

@bradfitz

nhooyr · 2019-05-31T22:59:02Z

How about we change the context cancellation to also apply to the request body. Then, when the body is first written to, both the context cancellation and Timeout are reset to give the application full control over the protocol being spoken now?

The hanging behavior described in #971 happens after this Golang change: golang/go@5416204 , released in 1.12. If a `101 Switching Protocols` response is received, `response.Body` will be replaced by an underlying `net.Conn`, and reading it blocks indefinitely without a protocol implementation. This is not a big problem for k6 in this case, since we don't support protocol upgrades with the current HTTP API as shown in the example, so the agreed solution was to detect it and return an error. This Golang change is currently incompatible with client timeouts[1], though this will hopefully be resolved by the time we add support for h2c (#970) or other protocols. [1]: golang/go#31391 Closes #971

The hanging behavior described in grafana#971 happens after this Golang change: golang/go@5416204 , released in 1.12. If a `101 Switching Protocols` response is received, `response.Body` will be replaced by an underlying `net.Conn`, and reading it blocks indefinitely without a protocol implementation. This is not a big problem for k6 in this case, since we don't support protocol upgrades with the current HTTP API as shown in the example, so the agreed solution was to detect it and return an error. This Golang change is currently incompatible with client timeouts[1], though this will hopefully be resolved by the time we add support for h2c (grafana#970) or other protocols. [1]: golang/go#31391 Closes grafana#971

jim-minter · 2020-10-10T02:30:36Z

I think I've been able to hack around this issue outside of the standard library as follows:

// cancelBody is a workaround for the fact that http timeouts are incompatible
// with hijacked connections (https://github.com/golang/go/issues/31391):
// net/http.cancelTimerBody does not implement Writer.
type cancelBody struct {
	io.ReadWriteCloser
	t *time.Timer
	c chan struct{}
}

func (b *cancelBody) wait() {
	select {
	case <-b.t.C:
		b.ReadWriteCloser.Close()
	case <-b.c:
		b.t.Stop()
	}
}

func (b *cancelBody) Close() error {
	select {
	case b.c <- struct{}{}:
	default:
	}

	return b.ReadWriteCloser.Close()
}

func newCancelBody(rwc io.ReadWriteCloser, d time.Duration) io.ReadWriteCloser {
	b := &cancelBody{
		ReadWriteCloser: rwc,
		t:               time.NewTimer(d),
		c:               make(chan struct{}),
	}

	go b.wait()

	return b
}

Where appropriate, add resp.Body = newCancelBody(resp.Body.(io.ReadWriteCloser), time.Hour) to wrap the old resp.Body in one that will close itself after an hour but which still allows 101 Switching Protocol responses.

bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 10, 2019

bcmills added this to the Go1.13 milestone Apr 10, 2019

nhooyr mentioned this issue Apr 22, 2019

Warn users against setting Timeout on the http.Client nhooyr/websocket#67

Closed

andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019

na-- mentioned this issue Sep 26, 2019

fix(httpext): return error on unsupported 101 response grafana/k6#1172

Merged

rsc modified the milestones: Go1.14, Backlog Oct 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net/http: http.Client Timeout incompatible with writable bodies #31391

net/http: http.Client Timeout incompatible with writable bodies #31391

drigz commented Apr 10, 2019

agnivade commented Apr 11, 2019

nhooyr commented May 31, 2019

jim-minter commented Oct 10, 2020 •

edited

net/http: http.Client Timeout incompatible with writable bodies #31391

net/http: http.Client Timeout incompatible with writable bodies #31391

Comments

drigz commented Apr 10, 2019

agnivade commented Apr 11, 2019

nhooyr commented May 31, 2019

jim-minter commented Oct 10, 2020 • edited

jim-minter commented Oct 10, 2020 •

edited