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 it's illegal to reuse a Request concurrently from multiple goroutines #21780

Closed
twmb opened this issue Sep 6, 2017 · 10 comments
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@twmb
Copy link
Contributor

twmb commented Sep 6, 2017

Incorrect errRequestCanceled errors occur when reusing http.Request structs.

There is a small race in reusing Transport's reqCanceler 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 to getConn, no idle conns exist, b sets the request's canceler (L940, important), and a large select happens: L948-L986
a's persistConn running gets notified of the bodyEOF, sets its transport's request canceler for this request to nil (L1656) overriding the canceler that was just set by B, and goes to tryPutIdleConn (assuming other conditions are successful): L1655-L1661
a''s tryPutIdleConn sees a waitingDialer: L704-L706
b's dial select receives the persistConn that just ran a: L976-L986
b's RoundTrip getConn returns successfully and goes into pconn.roundTrip (L400-L413, the main block)
roundTrip tries to replace the transports request canceler for b with the persistConn's cancelRequest: L1889-L1891
replaceReqCancel fails because the prior replacement deleted the request from the transport's reqCanceler map, and if the request does not exist in the map, the canceler for it cannot be replaced: L856-L865
This causes roundTrip to return errRequestCanceled: L1893

Also 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)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/twmb/go"
GORACE=""
GOROOT="/home/twmb/go/go"
GOTOOLDIR="/home/twmb/go/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build465265467=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

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.

@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 26, 2018
@andybons andybons added this to the Go1.11 milestone Mar 26, 2018
@andybons
Copy link
Member

@bradfitz

@odeke-em
Copy link
Member

@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
screen shot 2018-04-24 at 4 52 23 pm

/cc @tombergan @bradfitz

@twmb
Copy link
Contributor Author

twmb commented Apr 25, 2018

$ go version && go test -bench .
go version devel +be012e1e2e Wed Apr 25 00:36:09 2018 +0000 linux/amd64
my thinger 0xc0000b0000
goos: linux
goarch: amd64
BenchmarkDeadreq-4   	my thinger 0xc0000b0000
panic: Get http://www.example.com: net/http: request canceled

goroutine 10 [running]:
_/home/twmb/testing.chk(0x701720, 0xc00016a390)
	/home/twmb/testing/junk_test.go:14 +0x4a
_/home/twmb/testing.BenchmarkDeadreq.func1(0xc0000f6300)
	/home/twmb/testing/junk_test.go:27 +0xd1
testing.(*B).RunParallel.func1(0xc000134030, 0xc000134028, 0xc000134020, 0xc000001e00, 0xc00000c060)
	/home/twmb/go/go/src/testing/benchmark.go:623 +0x97
created by testing.(*B).RunParallel
	/home/twmb/go/go/src/testing/benchmark.go:616 +0x192
exit status 2
FAIL	_/home/twmb/testing	0.034s

@odeke-em
Copy link
Member

Thanks for the check-in @twmb and run on Linux, apparently I am living in a bubble with my Darwin AMD64 environment.

@twmb
Copy link
Contributor Author

twmb commented Apr 25, 2018

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.

@bradfitz
Copy link
Contributor

bradfitz commented Jul 9, 2018

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.

@bradfitz bradfitz changed the title net/http: incorrect errRequestCanceled returns when reusing requests net/http: document that it's illegal to reuse a Request concurrently from multiple goroutines Jul 9, 2018
@bradfitz bradfitz added the NeedsFix The path to resolution is known, but the work has not been done. label Jul 9, 2018
@bradfitz bradfitz self-assigned this Jul 9, 2018
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 9, 2018
@twmb
Copy link
Contributor Author

twmb commented Jul 9, 2018

Changing the benchmark to be serial reproduces this for me as well:

func BenchmarkDeadreq(b *testing.B) {
	req, err := http.NewRequest("GET", "http://www.example.com", nil)
	chk(err)
	client := new(http.Client)
	for i := 0; i < b.N; i++ {
		resp, err := client.Do(req)
		chk(err)
		go func() {
			_, err = io.Copy(ioutil.Discard, resp.Body)
			chk(resp.Body.Close())
			chk(err)
		}()
	}
}

As far as I can tell, this is more in line with reusing a request serially back-to-back.

@bradfitz
Copy link
Contributor

bradfitz commented Jul 9, 2018

That still looks concurrent to me. You're not done with the request+response pair by the time of your next Do call.

@twmb
Copy link
Contributor Author

twmb commented Jul 9, 2018

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.

@gopherbot
Copy link

Change https://golang.org/cl/122817 mentions this issue: net/http: clarify when it's allowed to reuse a Request

@golang golang locked and limited conversation to collaborators Jul 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants