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

proposal: net/http: don't recover from handler panics #25245

Closed
neild opened this issue May 3, 2018 · 7 comments

Comments

Projects
None yet
7 participants
@neild
Copy link
Contributor

commented May 3, 2018

The http package recovers from panics in handlers, logs a stack trace, and continues. We should consider removing the recover and letting the process crash in the event of a handler panic.

A panic (other than ErrAbortHandler) indicates a bug in a handler. There is no guarantee that the handler has properly cleaned up after the panic. It is very possible that the panic has left the server in an inconsistent state; e.g., mutexes left locked. Crashing the process surfaces the problem to the user immediately and allows it to be restarted.

As a concrete example of this, I'm looking into a bug where a handler crashed in code that tracks request statistics and left a mutex locked. Future requests blocked on this lock, piling up deadlocked goroutines. We'd have been much better off if the process had simply crashed and been restarted.

@gopherbot gopherbot added this to the Proposal milestone May 3, 2018

@gopherbot gopherbot added the Proposal label May 3, 2018

@kardianos

This comment has been minimized.

Copy link
Contributor

commented May 3, 2018

If memory serves, recovery is new; the behavior you are looking was the previous behavior.

As to your situation, use the following rule of thumb:

l.Lock()
// Action can't panic.
l.Unlock()

// or

l.Lock()
defer l.Unlock()
// Action that can panic.
@neild

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2018

If you're offering advice on how not to deadlock on panic, then just not writing code which panics in the first place seems even better. And yet bugs of all kinds do seem to still happen. :)

@bradfitz

This comment has been minimized.

Copy link
Member

commented May 4, 2018

This has come up previously and IIRC the resolution was that it's regrettable but we can't change it in Go 1.x for compatibility. So this is probably a Go 2.x thing.

@neild neild added the Go2 label May 4, 2018

@neild

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2018

Agreed that we can't change it now; I was thinking of go2 when I filed this.

@dantoye

This comment has been minimized.

Copy link

commented Jun 4, 2018

Not a fan of this proposal. We're talking about unexpected panic's, not panic's which have come up in development and been ignored... There's no reason to unexpectedly bring down someone's production server because of such a panic which, in all likelihood, would be caused by an unusual http request (hacking attempt, et al). I do agree it should be a configurable option in the server to disable automatic recovery, though.

"There is no guarantee that the handler has properly cleaned up after the panic."

There's never a guarantee people are cleaning up correctly, panic or not. If they are defer'ing their cleanup's, there's no problem.

@perillo

This comment has been minimized.

Copy link

commented Jun 15, 2018

@bradfitz, why can't this feature be changed in Go 1.x? Isn't is possible to add a new field to the Server type, like ShutdownOnPanic? The default is false so there should be no API change.

I think that a server should be shutdown in case of a panic. It is responsibility of the supervisor process to restart the Go application.
I currently use systemd, but in future I'm planning to use Nginx Unit, that should make the deployment of a Go application more robust, even with ShutdownOnPanic set to true.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2018

Folding this into #5465.

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.