FAQ
Hi All,

Ref: [golang-dev] Possible race caused by http persistent connections
https://groups.google.com/d/topic/golang-dev/0Nl6k5Sj6UU/discussion

In the above discussion, I mentioned incidentally that using
http.*Transport.CancelRequest is tricky.

The background is that you can't call http.CancelRequest until you
know the request is in flight. So if you do something along the lines:

   req := &http.Request{...}
   go func() {
      <-requestShouldBeCancled
      transport.CancelRequest(req)
   }()

   client.Do(req)

Then this is racy. The problem is that with the current
implementation, CancelRequest() may have nothing to cancel if it beats
the Do(). To avoid the race you have to do something a bit contorted,
along the lines of what httputil.ReverseProxy does:

https://github.com/golang/go/blob/ab08f79af3d41e28bf2ccf2f8738024a1404aeac/src/net/http/httputil/reverseproxy.go#L130-L156

So, I was idly wondering: is there a good reason that the cancelation
state of a request lives on the transport, and not on the request?

If I were thinking about implementing this now, I would make Cancel a
method on the *Request, which would close a member variable of chan
struct{}.

Then clients could avoid worrying about a race, since the Do method's
internals would be able to check the req's cancel state when it comes
to starting the request.

Further idle thinking: can this transformation be done and
simultaneously maintain the API promise? I guess it seems doubtful,
since it would amount to a behaviour change of some sort. It would
make the API much less error prone if it were possible. I'm unable to
spend long enough thinking about this to rule out that it would break
existing legitimate code; hence this post.

The existing API could be kept, of course.
*Transport.CancelRequest(req) would just call the Cancel() method on
the `req`. There is a curious detail here that you could use
CancelRequest on a *Transport to cancel requests going over other
*Transports, but it's not obvious to me if/how this could be harmful.

Regards,

- Peter

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Search Discussions

  • James Bardin at Jun 10, 2015 at 3:20 pm

    On Wednesday, June 10, 2015 at 7:00:28 AM UTC-4, Peter Waller wrote:

    So, I was idly wondering: is there a good reason that the cancelation
    state of a request lives on the transport, and not on the request?
    The Request doesn't *do* the request. It's basically just metadata to give
    to the RoundTripper. The Transport is what handles the connections, and
    where things needs to be canceled.

    --
    You received this message because you are subscribed to the Google Groups "golang-nuts" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Peter Waller at Jun 10, 2015 at 3:39 pm

    On 10 June 2015 at 16:20, James Bardin wrote:
    The Request doesn't *do* the request. It's basically just metadata to give
    to the RoundTripper. The Transport is what handles the connections, and
    where things needs to be canceled.
    I see! One might make a single &Request{} object and invoke it
    repeatedly, for example. That's an obvious reason my proposal
    would not work.

    It's interesting how something as simple as
    (*Transport).CancelRequest(*req) can be so deceptive.

    Perhaps the documentation could of CancelRequest could be improved to
    make the situation clearer?

    Currently, it's:

    http://golang.org/pkg/net/http/#Transport.CancelRequest
    CancelRequest cancels an in-flight request by closing its connection.
    Which is accurate and concise, but doesn't give the tiniest hint to
    a naive reader that there might be issues they have to deal with there.

    --
    You received this message because you are subscribed to the Google Groups "golang-nuts" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Daniel Morsing at Jun 11, 2015 at 2:46 pm

    On Wednesday, June 10, 2015 at 4:39:23 PM UTC+1, Peter Waller wrote:
    On 10 June 2015 at 16:20, James Bardin <j.ba...@gmail.com <javascript:>>
    wrote:
    The Request doesn't *do* the request. It's basically just metadata to give
    to the RoundTripper. The Transport is what handles the connections, and
    where things needs to be canceled.
    I see! One might make a single &Request{} object and invoke it
    repeatedly, for example. That's an obvious reason my proposal
    would not work.
    This is already the case. Using the same request multiple times
    concurrently means that only one of the requests will be cancelled if you
    call CancelRequest. I'm inclined to add a panic in transport.RoundTrip for
    this case since it breaks so many assumptions around cancellation

    I don't think we can store cancellation requests on the Transport. It has a
    lot of weird side effects. If we cancel a request then never issue the
    roundtrip, then we'll end up leaking requests structs since the transport
    will hold on to it because it might see the request soon and should
    immediately cancel it. If we cancel a request that's already done, we will
    also have to hold on to it, since we don't know if this is a request that
    has just finished or a request about to happen.

    It's interesting how something as simple as
    (*Transport).CancelRequest(*req) can be so deceptive.

    Perhaps the documentation could of CancelRequest could be improved to
    make the situation clearer?

    Currently, it's:

    http://golang.org/pkg/net/http/#Transport.CancelRequest
    CancelRequest cancels an in-flight request by closing its connection.
    Which is accurate and concise, but doesn't give the tiniest hint to
    a naive reader that there might be issues they have to deal with there.
      Annoyingly, I don't think the problem can be solved reasonably with the
    current API. Maybe provide a new method on Transport that takes a context,
    inspired by the pattern described in http://blog.golang.org/context

    --
    You received this message because you are subscribed to the Google Groups "golang-nuts" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
  • Peter Waller at Aug 7, 2015 at 2:41 pm
    I've just discovered that I happened to propose this around a similar time
    to someone else, and that someone else followed through and got a Cancel
    channel added to the request. Woohoo! That makes life a lot easier going
    forward.

    https://github.com/golang/go/issues/11013
    https://go-review.googlesource.com/#/c/11601/

    Along with this new package for cancelling outbound http requests from a
    net.Context:

    http://godoc.org/golang.org/x/net/context/ctxhttp

    On 11 June 2015 at 14:55, Daniel Morsing wrote:
    On Wednesday, June 10, 2015 at 4:39:23 PM UTC+1, Peter Waller wrote:
    On 10 June 2015 at 16:20, James Bardin wrote:
    The Request doesn't *do* the request. It's basically just metadata to give
    to the RoundTripper. The Transport is what handles the connections, and
    where things needs to be canceled.
    I see! One might make a single &Request{} object and invoke it
    repeatedly, for example. That's an obvious reason my proposal
    would not work.
    This is already the case. Using the same request multiple times
    concurrently means that only one of the requests will be cancelled if you
    call CancelRequest. I'm inclined to add a panic in transport.RoundTrip for
    this case since it breaks so many assumptions around cancellation

    I don't think we can store cancellation requests on the Transport. It has
    a lot of weird side effects. If we cancel a request then never issue the
    roundtrip, then we'll end up leaking requests structs since the transport
    will hold on to it because it might see the request soon and should
    immediately cancel it. If we cancel a request that's already done, we will
    also have to hold on to it, since we don't know if this is a request that
    has just finished or a request about to happen.

    It's interesting how something as simple as
    (*Transport).CancelRequest(*req) can be so deceptive.

    Perhaps the documentation could of CancelRequest could be improved to
    make the situation clearer?

    Currently, it's:

    http://golang.org/pkg/net/http/#Transport.CancelRequest
    CancelRequest cancels an in-flight request by closing its connection.
    Which is accurate and concise, but doesn't give the tiniest hint to
    a naive reader that there might be issues they have to deal with there.
    Annoyingly, I don't think the problem can be solved reasonably with the
    current API. Maybe provide a new method on Transport that takes a context,
    inspired by the pattern described in http://blog.golang.org/context

    --
    You received this message because you are subscribed to the Google Groups
    "golang-nuts" group.
    To unsubscribe from this group and stop receiving emails from it, send an
    email to golang-nuts+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.
    --
    You received this message because you are subscribed to the Google Groups "golang-nuts" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/d/optout.

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-nuts @
categoriesgo
postedJun 10, '15 at 11:00a
activeAug 7, '15 at 2:41p
posts5
users3
websitegolang.org

People

Translate

site design / logo © 2022 Grokbase