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/httputil: Question about new panic in ReverseProxy #28239

Closed
VojtechVitek opened this issue Oct 16, 2018 · 6 comments
Closed

net/http/httputil: Question about new panic in ReverseProxy #28239

VojtechVitek opened this issue Oct 16, 2018 · 6 comments
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.

Comments

@VojtechVitek
Copy link

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

go version go1.11.1 darwin/amd64

What did you do?

We use httputil.ReverseProxy in a HTTP Handler to proxy the S3 file download.

We started seeing panic: net/http: abort Handler messages in our logs/alert platform, since go1.11.

The stacktrace leads to a new panic(http.ErrAbortHandler) in (*ReverseProxy).ServeHTTP() method:

err = p.copyResponse(rw, res.Body, p.flushInterval(req, res))
if err != nil {
defer res.Body.Close()
// Since we're streaming the response, if we run into an error all we can do
// is abort the request. Issue 23643: ReverseProxy should use ErrAbortHandler
// on read error while copying body.
if !shouldPanicOnCopyError(req) {
p.logf("suppressing panic for copyResponse error in test; copy error: %v", err)
return
}
panic(http.ErrAbortHandler)
}

Question

Are we supposed to defer-recover from this new panic() from now on?

I feel this should be documented at https://golang.org/pkg/net/http/httputil/#ReverseProxy .. but it's not.

@VojtechVitek
Copy link
Author

Also, can we get the original err from the p.copyResponse() somehow?

The panic obfuscates the original error with a generic http.ErrAbortHandler error.

@bradfitz
Copy link
Contributor

Are you not using http.Server? The http.Server automatically handles ErrAbortHandler.

@bradfitz
Copy link
Contributor

Also, #23643 which was included in Go 1.11 was supposed to even fix the case of not running under an http.Server.

So I'm really curious about your environment.

Do you have a minimal repro?

@bradfitz bradfitz added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Oct 16, 2018
@VojtechVitek
Copy link
Author

@bradfitz yes, we're using http.Server. The panic is handled and the program doesn't crash.

However, we use some HTTP middlewares that report all panics to Sentry and to our Logs. And now I'm curious what to do with this generic error.

Should we start filtering http.ErrAbortHandler out?
Can we get the original cause error somehow?

@bradfitz
Copy link
Contributor

I don't know what your middleware wants to do, but here's the documentation for that panic value:

https://golang.org/pkg/net/http/#ErrAbortHandler

ErrAbortHandler is a sentinel panic value to abort a handler. While any panic from ServeHTTP aborts the response to the client, panicking with ErrAbortHandler also suppresses logging of a stack trace to the server's error log.

You can modify your middleware as appropriate.

There is no way to get the original cause. Usually it's because the client went away (closed browser tab, etc) and the proxy copy thus failed.

@VojtechVitek
Copy link
Author

ErrAbortHandler is a sentinel panic value to abort a handler. While any panic from ServeHTTP aborts the response to the client, panicking with ErrAbortHandler also suppresses logging of a stack trace to the server's error log.

OK. I think it makes sense to suppress these errors in our middleware too. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

3 participants