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

Comments

Projects
None yet
6 participants
@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

This comment has been minimized.

Copy link
Member

bradfitz commented Dec 14, 2016

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

This comment has been minimized.

Copy link
Contributor Author

benburkert commented Dec 14, 2016

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

This comment has been minimized.

Copy link
Member

bradfitz commented Dec 14, 2016

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

This comment has been minimized.

Copy link
Contributor

tombergan commented Dec 14, 2016

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

This comment has been minimized.

Copy link
Member

bradfitz commented Dec 14, 2016

Perhaps. But that is Go 1.9.

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

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Dec 14, 2016

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

net/http/httptrace: clarify WroteRequest may be called multiple times
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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

odeke-em commented Jul 23, 2017

@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
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.