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 it's illegal to reuse a Request concurrently from multiple goroutines #21780
Comments
@twmb thank you for filling this issue! I am currently at tip d4e936c $ go version
go version devel +d4e936c Tue Apr 24 20:21:08 2018 +0000 darwin/amd64 and I can't reproduce this issue /cc @tombergan @bradfitz |
|
Thanks for the check-in @twmb and run on Linux, apparently I am living in a bubble with my Darwin AMD64 environment. |
I'm curious how this could be different on Darwin because none of the race is Darwin specific, but I'd have to dig back in. |
This bug's title ("when reusing requests") made me think this was about back-to-back serial re-use of Requests, but the description and repro above (https://play.golang.org/p/BP7YCP2D4J) show that this is really about about concurrent use of the same request. I suppose this could be made to work at least for GETs, but I'm not sure it's worth it. If it doesn't work for POSTs or things with bodies, I think I'd rather just document one consistent rule: that you can't use a Request concurrently by multiple goroutines. |
errRequestCanceled
returns when reusing requests
Changing the benchmark to be serial reproduces this for me as well:
As far as I can tell, this is more in line with reusing a request serially back-to-back. |
That still looks concurrent to me. You're not done with the request+response pair by the time of your next |
Agreed, I was drafting a response along those lines. I would be fine with documentation that a request cannot be request until after the response body is closed. |
Change https://golang.org/cl/122817 mentions this issue: |
Incorrect
errRequestCanceled
errors occur when reusinghttp.Request
structs.There is a small race in reusing
Transport
'sreqCanceler
map, where a finishing request can override an active request dial's canceler. I suspect this is what #19653 was running into, and Brad hinted at this.The problem:
The code centers around transport.go's
RoundTrip
block of code here, L400-L413.For two requests a and b that execute back to back:
a sees a body's EOF: L1622-L1624,
b begins, enters
RoundTrip
, tries togetConn
, no idle conns exist, b sets the request's canceler (L940, important), and a large select happens: L948-L986a's
persistConn
running gets notified of thebodyEOF
, sets its transport's request canceler for this request to nil (L1656) overriding the canceler that was just set by B, and goes totryPutIdleConn
(assuming other conditions are successful): L1655-L1661a''s
tryPutIdleConn
sees awaitingDialer
: L704-L706b's dial select receives the
persistConn
that just ran a: L976-L986b's
RoundTrip
getConn
returns successfully and goes intopconn.roundTrip
(L400-L413, the main block)roundTrip
tries to replace the transports request canceler for b with thepersistConn
'scancelRequest
: L1889-L1891replaceReqCancel
fails because the prior replacement deleted the request from the transport'sreqCanceler
map, and if the request does not exist in the map, the canceler for it cannot be replaced: L856-L865This causes
roundTrip
to returnerrRequestCanceled
: L1893Also note that this is only the http1 stack and is harder to notice against URLs that have redirects.
What version of Go are you using (
go version
)?1.8.3, 1.9, tip
Does this issue reproduce with the latest release?
yes
What operating system and processor architecture are you using (
go env
)?What did you do?
https://play.golang.org/p/BP7YCP2D4J
go test -bench . -benchtime 10s
(should happen quickly)What did you expect to see?
No panic.
What did you see instead?
A panic.
The text was updated successfully, but these errors were encountered: