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/httptrace: add retry hook #18305

Open
benburkert opened this issue Dec 14, 2016 · 8 comments
Open

net/http/httptrace: add retry hook #18305

benburkert opened this issue Dec 14, 2016 · 8 comments

Comments

@benburkert
Copy link
Contributor

benburkert commented Dec 14, 2016

Please answer these questions before submitting your issue. Thanks!

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

go version go1.7.4 darwin/amd64

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/benburkert"
GORACE=""
GOROOT="/usr/local/Cellar/go/1.7.4/libexec"
GOTOOLDIR="/usr/local/Cellar/go/1.7.4/libexec/pkg/tool/darwin_amd64"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/qs/qkt9twmx4qg379d6f8kxl1vm0000gn/T/go-build714318584=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"

What did you do?

https://play.golang.org/p/odjH-4qcwa

What did you expect to see?

The WroteRequest hook is called only once for the second request, as it is with the first request, and the program exits normally.

What did you see instead?

The WroteRequest hook is called twice because the transport attempts to write the request to a dead connection then retries with a new connection. Both attempts call the hook. This causes the program to panic because of the send on the already closed channel during the second call.

panic: send on closed channel

goroutine 12 [running]:
panic(0x3167c0, 0x1070a648)
	/usr/local/go/src/runtime/panic.go:500 +0x720
main.request.func1(0x0, 0x0)
	/tmp/sandbox194689056/main.go:41 +0x40
net/http.(*Request).write.func1(0x10736780, 0x10716f18)
	/usr/local/go/src/net/http/request.go:452 +0x80
net/http.(*Request).write(0x10740880, 0x440890, 0x10733100, 0x0, 0x10733120, 0x0, 0x0, 0x0)
	/usr/local/go/src/net/http/request.go:573 +0x10fb
net/http.(*persistConn).writeLoop(0x10746870, 0x0)
	/usr/local/go/src/net/http/transport.go:1644 +0x200
created by net/http.(*Transport).dialConn
	/usr/local/go/src/net/http/transport.go:1058 +0x13a0
@bradfitz
Copy link
Contributor

Okay, we can document that it may be called multiple times.

@bradfitz bradfitz self-assigned this Dec 14, 2016
@bradfitz bradfitz added this to the Go1.8Maybe milestone Dec 14, 2016
@benburkert
Copy link
Contributor Author

thanks @bradfitz. I was also surprised to see that the WroteRequestInfo argument to the hook has a nil error for every call, even when the request write (supposedly) failed.

https://play.golang.org/p/HvqrsSN48F

@bradfitz
Copy link
Contributor

Who says the request write failed? It looks like the write was successful, but then the server hung up, so it retried because it was safe to do so.

@tombergan
Copy link
Contributor

Should we also add a hook for retries to help the user make sense of multiple WroteRequest/GetConn callbacks? I'm also reminded of #17152.

@bradfitz
Copy link
Contributor

Perhaps. But that is Go 1.9.

We can re-use this bug. I won't close this one.

@gopherbot
Copy link

CL https://golang.org/cl/34386 mentions this issue.

@bradfitz bradfitz modified the milestones: Go1.9, Go1.8Maybe Dec 15, 2016
gopherbot pushed a commit that referenced this issue Dec 15, 2016
Updates #18305

Change-Id: I63b28d511df1a6c54e32c8bfc7e2264f94e38cd7
Reviewed-on: https://go-review.googlesource.com/34386
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@bradfitz bradfitz changed the title net/http/httptrace: WroteRequest hook called twice on request retry net/http/httptrace: add retry hook Dec 15, 2016
@benburkert
Copy link
Contributor Author

benburkert commented Dec 15, 2016

Yes, that would be a successful write, my mistake.

It looks like the GetConn and GotConn hooks can also be called multiple times. I did not check any others but there may be more.

@bradfitz bradfitz modified the milestones: Go1.9Maybe, Go1.9 Jun 15, 2017
@bradfitz bradfitz removed their assignment Jun 15, 2017
@odeke-em
Copy link
Member

@benburkert interested in documenting your findings?

@ianlancetaylor ianlancetaylor modified the milestones: Go1.10, Go1.9Maybe Aug 3, 2017
@bradfitz bradfitz modified the milestones: Go1.10, Unplanned Nov 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants