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/httputil: request may be killed by mistake by former cancel request in ReverseProxy.ServeHTTP #16041

Closed
xuzhaokui opened this issue Jun 12, 2016 · 5 comments
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone

Comments

@xuzhaokui
Copy link

xuzhaokui commented Jun 12, 2016

Hi:

  1. What version of Go are you using (go version)?

    go version go1.6.2 darwin/amd64

  2. What operating system and processor architecture are you using (go env)?

    GOARCH="amd64"
    GOOS="darwin"

  3. What did you do?

    1. serving proxy requests by httputil.ReverseProxy
    2. about 10% of request are canceled by clients
  4. What did you expect to see?

    no requests should occur any unexpected error.

  5. What did you see instead?

    some normal(non-canceled) requests are affected by their former requests which are canceled by clients.

Finally I find this:

There is a goroutine here(in ServeHTTP): goroutine

this goroutine is used for receiving clientGone or reqDone signal, however, we assume this scene:

  1. ServeHTTP start handling Request A(using underlying net.Conn C)
  2. Request A was canceled by its client (sending sig to clientGone)
  3. RoundTrip return error(may be use of closed network connection in go1.6- or other error in go1.6+)
  4. ServeHTTP start to return and defer close(reqDone) is executed(close is non-block, so ServeHTTP may be return before the goroutine A receive close signal, but goroutine A is still alive)
  5. net/http put net.Conn C to IdleConn
  6. Request B arrived and net/http pick the same underlying net.Conn C
  7. ServeHTTP start handling Request B (using the same underlying net.Conn C)
  8. goroutine A got signal clientGone first, and then do CancelRequest which will close net.Conn C
  9. Request B's RoundTrip return error cause its underlying conn is closed
@ianlancetaylor ianlancetaylor added this to the Go1.7Maybe milestone Jun 12, 2016
@ianlancetaylor
Copy link
Contributor

CC @adg

@dsnet dsnet changed the title net/http/httputil: request may be killed by mistake by former cancel requet in ReverseProxy.ServeHTTP net/http/httputil: request may be killed by mistake by former cancel request in ReverseProxy.ServeHTTP Jun 13, 2016
@adg
Copy link
Contributor

adg commented Jun 14, 2016

Are you able to write a test that demonstrates the issue? I have a couple of ideas what might be wrong, but without a test I can't be sure.

@adg adg self-assigned this Jun 14, 2016
@danp
Copy link
Contributor

danp commented Jun 21, 2016

If it helps at all I did spend some time looking at this and trying to reproduce with various means, such as adding delay between the clientGone close being noticed and CancelRequest being called. And I tried with both 1.6.2 and tip post-1.7beta2. Wasn't able to come up with anything but happy to test further if there are any hunches.

Any more info on requests (specifically ones like "Request A") would be useful, such as what HTTP method they used, whether they had a body, etc.

@bradfitz bradfitz added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jun 28, 2016
@bradfitz
Copy link
Contributor

I don't think step 8 in your scenario (requestCanceler.CancelRequest(outreq)) should close the underlying TCP connection. If step 3 happens (the RoundTrip failed), the cancellation code to close the TCP connection should've already been unregistered.

In any case, more details as requested by @adg and @dpiddy would be nice. How can we reproduce this? What do the requests look like? How are the 10% of requests being canceled? I'd like to help if I can see it fail.

Thanks.

For now, kicking to Go 1.8, unless you can get back to us soon.

@bradfitz bradfitz modified the milestones: Go1.8Maybe, Go1.7Maybe Jun 28, 2016
@bradfitz
Copy link
Contributor

Closing due to timeout. Let us know and reopen if you have more details.

@golang golang locked and limited conversation to collaborators Aug 15, 2017
@rsc rsc unassigned bradfitz and adg Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

6 participants