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: document that context cancellation does not apply to writable bodies #30876

Open
nhooyr opened this issue Mar 16, 2019 · 5 comments
Open
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@nhooyr
Copy link
Contributor

nhooyr commented Mar 16, 2019

#26937

To enable http.ReverseProxy to cleanly support websocket connections, http.Transport began returning writable response bodies.

This is documented. But what's undocumented is how this response body deals with the request context's cancellation.

See #26101

Right now, the documentation says the cancellation of the request context will cause the response body to close, even if it is writable. However, this is not the case. If a writable response body is returned, cancellation of the request context does not affect it. This is certainly the right behaviour but should be documented.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/167682 mentions this issue: net/http: document behaviour of cancellation on writable response bodies

@katiehockman
Copy link
Contributor

/cc @bradfitz

Is this intended behavior, and if so, do you think the additional comment in their PR is helping clarify?

@bradfitz
Copy link
Contributor

If a writable response body is returned, cancellation of the request context does not affect it. This is certainly the right behaviour but should be documented.

Why is that certainly the right behavior?

I don't recall that being discussed or designed.

@bradfitz bradfitz added this to the Go1.13 milestone Mar 20, 2019
@nhooyr
Copy link
Contributor Author

nhooyr commented Mar 22, 2019

Why is that certainly the right behavior?

If you use this feature to do a WebSocket handshake, you will want to have a timeout on the handshake but not on the connection afterwards as WebSocket's can be long lived. Its best to let the caller handle timeouts via the Close method on the returned body.

If the context applied on the returned body for upgraded requests, it would become impossible to cleanly set a timeout on the handshake but not the body.

@nhooyr
Copy link
Contributor Author

nhooyr commented Mar 24, 2019

I definitely do see how this could be unexpected as a server could make client code hang if it returns an unexpected upgrade response..

I think the solution here is to only return a writable body if the request is a upgrade request as well, meaning the client understands the body will be writable and not affected by the request context.

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 12, 2019
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants