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

Comments

Projects
None yet
5 participants
@twmb
Copy link
Contributor

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 this to the Go1.11 milestone Mar 26, 2018

@andybons

This comment has been minimized.

Copy link
Member

commented Mar 26, 2018

@odeke-em

This comment has been minimized.

Copy link
Member

commented Apr 24, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

commented Apr 25, 2018

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

@twmb

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

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 label Jul 9, 2018

@bradfitz bradfitz self-assigned this Jul 9, 2018

@twmb

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link

commented Jul 9, 2018

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

@gopherbot gopherbot closed this in 9776d02 Jul 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.