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

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

Open
drigz opened this issue Apr 10, 2019 · 2 comments
Milestone

Comments

@drigz
Copy link

@drigz drigz commented Apr 10, 2019

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.

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

This comment has been minimized.

Copy link
Contributor

@agnivade agnivade commented Apr 11, 2019

@nhooyr

This comment has been minimized.

Copy link
Contributor

@nhooyr nhooyr commented May 31, 2019

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?

@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
imiric added a commit to loadimpact/k6 that referenced this issue Sep 23, 2019
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
imiric added a commit to loadimpact/k6 that referenced this issue Sep 24, 2019
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
imiric added a commit to loadimpact/k6 that referenced this issue Sep 26, 2019
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
imiric added a commit to loadimpact/k6 that referenced this issue Sep 26, 2019
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
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
cuonglm added a commit to loadimpact/k6 that referenced this issue Oct 15, 2019
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
srguglielmo added a commit to srguglielmo/k6 that referenced this issue Nov 3, 2019
The hanging behavior described in loadimpact#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 (loadimpact#970) or other protocols.

[1]: golang/go#31391

Closes loadimpact#971
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.