Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

affected/package: x/sync: Program crash when handling panic with wrapped http.ErrAbortHandler #62511

Closed
ewohltman opened this issue Sep 7, 2023 · 3 comments

Comments

@ewohltman
Copy link

ewohltman commented Sep 7, 2023

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

$ go version
go version go1.21.0 linux/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output
$ go env
GO111MODULE=''
GOARCH='amd64'
GOBIN='/home/eric/work/bin'
GOCACHE='/home/eric/.cache/go-build'
GOENV='/home/eric/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/eric/work/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/eric/work'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/eric/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/home/eric/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.21.0'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build978392017=/tmp/go-build -gno-record-gcc-switches'

What did you do?

The net/http.conn.serve method is equipped to handle a special case for when panic is called with http.ErrAbortHandler by downstream http.Handler implementations where it silently recovers and prevents the program from crashing.

When chaining http.Handler implementations, if an http.Handler uses golang.org/x/sync/singleflight.Group, and an http.Handler downstream of that panics with http.ErrAbortHandler, the singleflight package recovers from the panic first, wraps the panic value in its own private error type, and re-panics with its private type. When the new panic gets to net/http.conn.serve, a direct comparison is made to see if err != ErrAbortHandler. Since technically err is not ErrAbortHandler, just another error type that wraps ErrAbortHandler, net/http.conn.serve does not handle the special case and allows the program to crash.

Example where net/http.conn.serve handles the panic and the program does not crash:

Example using golang.org/x/sync/singleflight.Group where net/http.conn.serve does not handle the panic and the program crashes:

What did you expect to see?

When using golang.org/x/sync/singleflight.Group in a chain of http.Handler implementations where a panic(http.ErrAbortHandler is called downstream, net/http.conn.serve is able to unwrap errors to determine if the error is actually http.ErrAbortHandler and handle the panic without allowing the program to crash.

What did you see instead?

The program crashes with the message panic: net/http: abort Handler.

This issue is related to #62510

@ewohltman ewohltman changed the title affected/package: x/sync affected/package: x/sync: Program crash when handling panic with wrapped http.ErrAbortHandler Sep 7, 2023
@ewohltman
Copy link
Author

ewohltman commented Sep 7, 2023

https://go.dev/cl/526171

@cherrymui
Copy link
Member

This is the same as #62510. No need to file two issues. Closing. Thanks.

@cherrymui cherrymui closed this as not planned Won't fix, can't repro, duplicate, stale Sep 7, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/526171 mentions this issue: singleflight: add panicError.Unwrap method

gopherbot pushed a commit to golang/sync that referenced this issue Sep 26, 2023
Currently when singleflight recovers from a panic, it wraps it with the private
error type panicError. This change adds an `Unwrap` method to panicError to
allow wrapped errors to be returned.

Updates golang/go#62511

Change-Id: Ia510ad7d5881207ef71f9eb89c1766835af19b6b
Reviewed-on: https://go-review.googlesource.com/c/sync/+/526171
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@golang golang locked and limited conversation to collaborators Sep 6, 2024
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

No branches or pull requests

3 participants