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: decide what to do about Transport.CancelRequest #13540

Closed
bradfitz opened this issue Dec 8, 2015 · 5 comments
Closed

x/net/http2: decide what to do about Transport.CancelRequest #13540

bradfitz opened this issue Dec 8, 2015 · 5 comments
Assignees
Milestone

Comments

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Dec 8, 2015

The net/http.Transport.CancelRequest method is an old feature & wart.

There is no interface which defines it, but it's sniffed for by many RoundTrippers and users of RoundTrippers (including net/http.Client for its Client.Timeout feature), and it's a thorn in the side of people trying to write composable RoundTripper implementations.

The modern replacement is the net/http.Request.Cancel receive-only channel to do cancelations. (closed by caller on cancel)

Unfortunately, net/http.Client doesn't use the Cancel channel. Perhaps it could. But that would require mutating the caller's *Request, at least for Do. But for net/http.Client methods like Get, Head, Post, and PostForm, we create the *http.Request, so we could set the Cancel channel appropriately.

That leaves net/http.Client.Do, which takes a raw *Request. Is it allowed to set the Cancel channel if it's nil? Probably not? Or maybe there is precedent in mutating the Request: we read from the Request.Body, so it's not safe to use concurrently already. So maybe we can just save/restore the Request.Cancel field.

Related to that debate is whether x/net/http2.Transport should have an old-style CancelRequest method. It would really be nice to stop letting that mistake infect things, though, spreading the idea that everybody needs to implement CancelRequest.

But unfortunately as-is, Client Timeouts are failing with http2: https://go-review.googlesource.com/#/c/17528/1 From the second comment:

$ go test -cover
--- FAIL: TestClientTimeout_h2 (2.01s)
    client_test.go:983: timeout after 1s waiting for timeout of 500ms

Thoughts welcome.

/cc @rsc @dsymonds @okdave @mcgreevy @bmizerany

@bradfitz bradfitz self-assigned this Dec 8, 2015
@bradfitz bradfitz added this to the Go1.6 milestone Dec 8, 2015
@dsymonds

This comment has been minimized.

Copy link
Member

@dsymonds dsymonds commented Dec 9, 2015

Could we make CancelRequest do nothing? It is already racy, so it's effectively best effort and we could just drop the effort. That would mean that net/http.Client doesn't have to change except to use the channel. I'm not sure I see why that would imply it has to mutate the request.

Or make CancelRequest close the Cancel channel?

@cespare

This comment has been minimized.

Copy link
Contributor

@cespare cespare commented Dec 9, 2015

@dsymonds There's nothing stopping people today from writing code that uses both CancelRequest and a Cancel channel. (Not that it makes much sense.) So making CancelRequest close the Cancel channel could cause a double-close, yes?

@dsymonds

This comment has been minimized.

Copy link
Member

@dsymonds dsymonds commented Dec 9, 2015

Yeah, but we could just tell people not to do that.

@bradfitz

This comment has been minimized.

Copy link
Contributor Author

@bradfitz bradfitz commented Dec 9, 2015

Could we make CancelRequest do nothing?

No, that's not really a great option. It does work mostly work today. It's only in some edge cases that it doesn't work.

That would mean that net/http.Client doesn't have to change except to use the channel.

So you're okay with Client using the channel? That means temporarily mutating & restoring the field on the Request because a Client uses a RoundTripper (e.g. Transport) (or potentially N RoundTripper composed deep) and passes down a Request (which has a Cancel). The Cancel (read: "context") is not a separate argument.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 5, 2016

CL https://golang.org/cl/18260 mentions this issue.

@bradfitz bradfitz closed this in 7de71c8 Jan 5, 2016
@golang golang locked and limited conversation to collaborators Jan 4, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.