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

Jump to bottom

net/http: TimeoutHandler allows wrapped handlers crash server with panics #22084

Closed
artyom opened this issue Sep 28, 2017 · 2 comments
Closed

net/http: TimeoutHandler allows wrapped handlers crash server with panics #22084

artyom opened this issue Sep 28, 2017 · 2 comments
Labels
FrozenDueToAge
Milestone
Go1.10

Comments

@artyom
Copy link
Member

artyom commented Sep 28, 2017

Please answer these questions before submitting your issue. Thanks!

What did you do?

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.

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

What did you expect to see?

I expect panics from http.Handlers to be handled by http.Server as documented (recovered), i.e. with the sentinel panic value http.ErrAbortHandler this program outputs something like this:

2017/09/28 22:55:18 Get http://127.0.0.1:52111: EOF

What did you see instead?

Program crashes:

panic: net/http: abort Handler

goroutine 21 [running]:
main.do.func1(0x13eb0e0, 0xc42011a0a0, 0xc420134000)
	/tmp/repro.go:19 +0x42
net/http.HandlerFunc.ServeHTTP(0x12c3438, 0x13eb0e0, 0xc42011a0a0, 0xc420134000)
	/Users/artyom/Library/go/src/net/http/server.go:1918 +0x44
net/http.(*timeoutHandler).ServeHTTP.func1(0xc420076c00, 0xc42011a0a0, 0xc420134000, 0xc420138060)
	/Users/artyom/Library/go/src/net/http/server.go:3043 +0x53
created by net/http.(*timeoutHandler).ServeHTTP
	/Users/artyom/Library/go/src/net/http/server.go:3042 +0x158
exit status 2

System details

go version go1.9 darwin/amd64
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/tmp/go:/Users/artyom/go"
GORACE=""
GOROOT="/Users/artyom/Library/go"
GOTOOLDIR="/Users/artyom/Library/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/lb/3rk8rqs53czgb4v35w_342xc0000gn/T/go-build484141897=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
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"
GOROOT/bin/go version: go version go1.9 darwin/amd64
GOROOT/bin/go tool compile -V: compile version go1.9
uname -v: Darwin Kernel Version 17.0.0: Thu Aug 24 21:48:19 PDT 2017; root:xnu-4570.1.46~2/RELEASE_X86_64
ProductName:	Mac OS X
ProductVersion:	10.13
BuildVersion:	17A365
lldb --version: lldb-900.0.45
  Swift-4.0

This happens because TimeoutHandler starts original handler in a separate goroutine:

go/src/net/http/server.go

Lines 3042 to 3045 in 78a0a31

go func() {
h.handler.ServeHTTP(tw, r)
close(done)
}()

I can work on fixing this by handling panic in this goroutine and re-raising it from the timeoutHandler.ServeHTTP itself, so it's handled by the Server. Please assign this to me if this solution looks ok.
@mvdan
Copy link
Member

mvdan commented Sep 28, 2017

/cc @tombergan @bradfitz

@tombergan tombergan self-assigned this Sep 29, 2017
@tombergan tombergan added this to the Go1.10 milestone Sep 29, 2017
@gopherbot
Copy link

Change https://golang.org/cl/67410 mentions this issue: net/http: make TimeoutHandler recover child handler panics

@golang golang locked and limited conversation to collaborators Oct 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.10
Development

No branches or pull requests
4 participants
@artyom @mvdan @gopherbot @tombergan