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

Recover middleware should not log panic for aborted handler #2133

Closed
3 tasks done
bbasic opened this issue Mar 15, 2022 · 1 comment
Closed
3 tasks done

Recover middleware should not log panic for aborted handler #2133

bbasic opened this issue Mar 15, 2022 · 1 comment

Comments

@bbasic
Copy link
Contributor

bbasic commented Mar 15, 2022

Issue Description

Using proxy and recover middleware would print the panic log in cases where the HTTP client aborts the request.
The error that is recovered is http.ErrAbortHandler. That one is a special error to signal aborting the HTTP handler and should not be logged.
From net/http/server.go https://github.com/golang/go/blob/88be85f18bf0244a2470fdf6719e1b5ca5a5e50a/src/net/http/server.go#L1775:

// 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.
var ErrAbortHandler = errors.New("net/http: abort Handler")

It should be ignored the same way as it is done in net/http/server.go.
https://github.com/golang/go/blob/88be85f18bf0244a2470fdf6719e1b5ca5a5e50a/src/net/http/server.go#L1799

See also the comment I found here golang/go#28239 (comment)

Checklist

  • Dependencies installed
  • No typos
  • Searched existing issues and docs

Expected behaviour

No stack trace for recovery of the ErrAbortHandler

Actual behaviour

I am seeing log such as:

"[PANIC RECOVER] net/http: abort Handler goroutine 45 [running]:
github.com/labstack/echo/v4/middleware.RecoverWithConfig.func1.1.1()
    github.com/labstack/echo/[email protected]/middleware/recover.go:89 +0x11a
panic({0xa18940, 0xc000112ad0})
    runtime/panic.go:1038 +0x215
net/http/httputil.(*ReverseProxy).ServeHTTP(0xc0004c0140, {0xba8d28, 0xc0005baae0}, 0xc0000ac100)
    net/http/httputil/reverseproxy.go:348 +0xeba
github.com/labstack/echo/v4/middleware.ProxyWithConfig.func1.1({0xbc4260, 0xc0000a8500})
    github.com/labstack/echo/[email protected]/middleware/proxy.go:260 +0x5f0
...

Steps to reproduce

Version/commit

github.com/labstack/echo/v4 v4.7.1

@lammel
Copy link
Contributor

lammel commented Mar 15, 2022

@bbasic Could you provide a pull request with tests to reproduce the log panic?

Based on the Go http server implementation I'm also in favor of not logging this panic in the recover middleware as it is not something that is an actual error or misbehavior.

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

2 participants