6 ms·

I found this "best practice" curious to read: > The standard net/http server violates this advice and recovers panics from request handlers. Consensus among ex

by cytzol 4y ago

I found this "best practice" curious to read: > The standard net/http server violates this advice and recovers panics from request handlers. Consensus among experienced Go engineers is that this was a historical mistake. If you sample server logs from application servers in other languages, it is common to find large stacktraces that are left unhandled. Avoid this pitfall in your servers. I don't think I've ever seen a server library — HTTP or otherwise — that didn't have a top-level "catch all exceptions" or "recover from panic" step in place, so that if there's a problem, it can return 500 (or the Internal Server Error equivalent) to the user and then carry on serving other requests. My reasoning is that any panic-worthy programming error is almost certainly going to be in the "business logic" part of the server, rather than the protocol-parsing "deal with the network" part, and thus, recoving from a panic caused by processing a request is "safe". One incoming request could cause a panic, but the next request may touch completely unrelated parts of the program, and still be processed as normal. Furthermore, returning a 500 error but having nobody read the stacktrace is bad, yes, but it's way, way, way better than having your server crash meaning nobody can use it ever. Oh wait, is the assumption here that your service is being run under Borg and has another 1000 instances running ready to jump in and take the crashed one's place? Is this another case of Google forgetting that people use Go outside of Google, or am I reading too much into this?

marcus_holmes 4y ago
If your program fails in testing, and no-one reads the logs, does it ever get fixed? There is one advantage to not having a top-level catch: if it fails in testing, it's very obvious immediately. Though I do agree with you - and this can be done with a feature flag for dev/prod servers.
andrew_eu 4y ago
The Echo web framework is an interesting example of this [0]. By default it doesn't handle panics, but there is a configurable `Recover` middleware which does so. I agree though it's unusual, and this stuck out to me as an exception that proves the rule. [0] https://echo.labstack.com/middleware/recover/ https://echo.labstack.com/middleware/recover/
evercast 4y ago
I believe the idea is that 'panic' is considered something fatal. If it happens, the application should die unless you have a strong reason for it not to. If you can recover e.g. by returning HTTP 500 for a single request, it should be handled by returning errors up throughout the call stack, i.e., by error handling instead of panicking. It's definitely an opinionated approach though.
- xienze 4y ago
  > I believe the idea is that 'panic' is considered something fatal. I think opinions in 3rd party Go code on what “fatal” means vary, that’s the issue. Sure it was intended to mean “this error is so bad the entire program needs to die right now” but in practice there’s cases where it’s treated more like an unchecked exception in Java, i.e., “I can’t recover from this so _I’m_ going to give up but _you_ can keep going.” Or put another way, what a library might consider fatal the caller doesn’t. It can be argued whether or not that’s the right thing to do, but the fact is it happens in the wild, so for something like a server you probably should trap it.
  - fn1 4y ago
    > but in practice there’s cases where it’s treated more like an unchecked exception in Java, i.e., “I can’t recover from this so _I’m_ going to give up but _you_ can keep going.” Java has a supertype for unrecoverable problems like out-of-memory-errors. It's called "Error", a subtype from Throwable. Exceptions are also Throwables, but they are NOT errors.
    - vbezhenar 4y ago
      There's nothing unrecoverable about Error exceptions. If one thread hit a bug and threw StackOverflowError, it's not a reason to kill the entire application or even thread itself, just unwind stack, show some error and continue processing next task. The only tricky situation I'm aware of is OutOfMemoryError, because it can affect other threads. I'd prefer to restart the server. But again it's just because typical code allocated heap all the time. One can write code carefully without heap allocations and this code will work just fine with OOM errors thrown around.
enneff 4y ago
The question: what is the state of your server after a handler panics? The answer: you have no idea. It is not wise to continue serving requests when you may have serious issues in the state of your server. Maybe some central data structure is now corrupt. You have no way of knowing. Fail fast and fail hard. OTOH, maybe your priorities are different and you would prefer to be more available than correct. In that case by all means add a recover to the top level of your request handlers. But it was a mistake to have made this decision for all users of net/http ahead of time.
- Zach_the_Lizard 4y ago
  In my experience, the panic is most likely because someone accessed a nil field when adapting some data. Nothing is corrupt, we just threw an exception in a mundane way. The reality is this is far more common than something truly fatal. Mistake for someone or not, it probably is correct for most use cases
  - throw827474737 4y ago
    Yes, why they made that decision back then. However in the end the lib cannot and shouldn't know, and this decision needs to be intentionally done by the middleware. Correct for most use cases is not good enough here, should be always correct for such fundamental thing.
  - Cthulhu_ 4y ago
    But that implies your request checking is off; if your server expects a field to be filled in and a client doesn't send it, that should be a HTTP 400 Bad Request error, not an internal server error. C/C++ based servers running into an error like that would possibly be open for attacks. Go will be a bit more resilient, but it's still better to avoid situations like that. That said, logging the error and going on serving responses is fine I think (pragmatic), as long as the error is analyzed. But an error that doesn't trigger immediate action is a warning, and warnings are noise [0]. [0] https://dave.cheney.net/2015/11/05/lets-talk-about-logging#:~:text=Let%E2%80%99s%20talk%20about%20warnings https://dave.cheney.net/2015/11/05/lets-talk-about-logging#:...
  - 4y ago
Thaxll 4y ago
How I understand it is that it's up to the user to create a panic handler middleware, which is perfectly valid and what most people are doing anyway. Something like: https://github.com/go-chi/chi/blob/master/middleware/recoverer.go#L22 https://github.com/go-chi/chi/blob/master/middleware/recover...
otabdeveloper4 4y ago
Presumably you have a load balancer in front of your Go code that's written using saner languages and assumptions that does handle errors correctly.
zerr 4y ago
No, that's about error handling: panic/recover in the standard net/http server is used in lieu of exception handling, which is absent in Go. Instead, they (and official Go docs as well) recommend to use "if-hell".
kevincox 4y ago
I've always thought that the ideal is somewhere in between the two. 1. Catch the panic/exception. 2. Track the rate of these panics or exceptions. If it is too high some data structure has probably been corrupted or some lock has been poisoned. If a lot of requests are failing abort. And ideally: 3 signal that you are in a degraded state so that some external process can gracefully drain your traffic and restart you. Although very few people have this level of self-healing infrastructure set up.
- llimllib 4y ago
  I wrote a web server that handled a lot of requests, and my solution to 2. was to have it notify me via our alert chat channel every time it panicked. This was rare enough that I could investigate them individually, and if it got overwhelming (it never did) I could choose to do something fancier.
sakras 4y ago
> Is this another case of Google forgetting that people use Go outside of Google, or am I reading too much into this? Whether or not they are forgetting aside, this is Google’s style guide for code bases in Google. I don’t think non-Google Go programmers were a consideration for them. On a broader note, it seems as though anytime Google publishes something people interpret it as “industry standard” (see their C++ style guide) and apply it to their non-Google projects. I personally don’t see this as healthy.
deleted 4y ago
[deleted]

by cytzol 4y ago

marcus_holmes 4y ago

If your program fails in testing, and no-one reads the logs, does it ever get fixed? There is one advantage to not having a top-level catch: if it fails in testing, it's very obvious immediately. Though I do agree with you - and this can be done with a feature flag for dev/prod servers.

andrew_eu 4y ago

The Echo web framework is an interesting example of this [0]. By default it doesn't handle panics, but there is a configurable `Recover` middleware which does so. I agree though it's unusual, and this stuck out to me as an exception that proves the rule. [0] https://echo.labstack.com/middleware/recover/ https://echo.labstack.com/middleware/recover/

evercast 4y ago

I believe the idea is that 'panic' is considered something fatal. If it happens, the application should die unless you have a strong reason for it not to. If you can recover e.g. by returning HTTP 500 for a single request, it should be handled by returning errors up throughout the call stack, i.e., by error handling instead of panicking. It's definitely an opinionated approach though.

xienze 4y ago
> I believe the idea is that 'panic' is considered something fatal. I think opinions in 3rd party Go code on what “fatal” means vary, that’s the issue. Sure it was intended to mean “this error is so bad the entire program needs to die right now” but in practice there’s cases where it’s treated more like an unchecked exception in Java, i.e., “I can’t recover from this so _I’m_ going to give up but _you_ can keep going.” Or put another way, what a library might consider fatal the caller doesn’t. It can be argued whether or not that’s the right thing to do, but the fact is it happens in the wild, so for something like a server you probably should trap it.
- fn1 4y ago
  > but in practice there’s cases where it’s treated more like an unchecked exception in Java, i.e., “I can’t recover from this so _I’m_ going to give up but _you_ can keep going.” Java has a supertype for unrecoverable problems like out-of-memory-errors. It's called "Error", a subtype from Throwable. Exceptions are also Throwables, but they are NOT errors.
  - vbezhenar 4y ago
    There's nothing unrecoverable about Error exceptions. If one thread hit a bug and threw StackOverflowError, it's not a reason to kill the entire application or even thread itself, just unwind stack, show some error and continue processing next task. The only tricky situation I'm aware of is OutOfMemoryError, because it can affect other threads. I'd prefer to restart the server. But again it's just because typical code allocated heap all the time. One can write code carefully without heap allocations and this code will work just fine with OOM errors thrown around.

enneff 4y ago

The question: what is the state of your server after a handler panics? The answer: you have no idea. It is not wise to continue serving requests when you may have serious issues in the state of your server. Maybe some central data structure is now corrupt. You have no way of knowing. Fail fast and fail hard. OTOH, maybe your priorities are different and you would prefer to be more available than correct. In that case by all means add a recover to the top level of your request handlers. But it was a mistake to have made this decision for all users of net/http ahead of time.

Zach_the_Lizard 4y ago
In my experience, the panic is most likely because someone accessed a nil field when adapting some data. Nothing is corrupt, we just threw an exception in a mundane way. The reality is this is far more common than something truly fatal. Mistake for someone or not, it probably is correct for most use cases
- throw827474737 4y ago
  Yes, why they made that decision back then. However in the end the lib cannot and shouldn't know, and this decision needs to be intentionally done by the middleware. Correct for most use cases is not good enough here, should be always correct for such fundamental thing.
- Cthulhu_ 4y ago
  But that implies your request checking is off; if your server expects a field to be filled in and a client doesn't send it, that should be a HTTP 400 Bad Request error, not an internal server error. C/C++ based servers running into an error like that would possibly be open for attacks. Go will be a bit more resilient, but it's still better to avoid situations like that. That said, logging the error and going on serving responses is fine I think (pragmatic), as long as the error is analyzed. But an error that doesn't trigger immediate action is a warning, and warnings are noise [0]. [0] https://dave.cheney.net/2015/11/05/lets-talk-about-logging#:~:text=Let%E2%80%99s%20talk%20about%20warnings https://dave.cheney.net/2015/11/05/lets-talk-about-logging#:...
- 4y ago

Thaxll 4y ago

How I understand it is that it's up to the user to create a panic handler middleware, which is perfectly valid and what most people are doing anyway. Something like: https://github.com/go-chi/chi/blob/master/middleware/recoverer.go#L22 https://github.com/go-chi/chi/blob/master/middleware/recover...

otabdeveloper4 4y ago

Presumably you have a load balancer in front of your Go code that's written using saner languages and assumptions that does handle errors correctly.

zerr 4y ago

No, that's about error handling: panic/recover in the standard net/http server is used in lieu of exception handling, which is absent in Go. Instead, they (and official Go docs as well) recommend to use "if-hell".

kevincox 4y ago

I've always thought that the ideal is somewhere in between the two. 1. Catch the panic/exception. 2. Track the rate of these panics or exceptions. If it is too high some data structure has probably been corrupted or some lock has been poisoned. If a lot of requests are failing abort. And ideally: 3 signal that you are in a degraded state so that some external process can gracefully drain your traffic and restart you. Although very few people have this level of self-healing infrastructure set up.

llimllib 4y ago
I wrote a web server that handled a lot of requests, and my solution to 2. was to have it notify me via our alert chat channel every time it panicked. This was rare enough that I could investigate them individually, and if it got overwhelming (it never did) I could choose to do something fancier.

sakras 4y ago

> Is this another case of Google forgetting that people use Go outside of Google, or am I reading too much into this? Whether or not they are forgetting aside, this is Google’s style guide for code bases in Google. I don’t think non-Google Go programmers were a consideration for them. On a broader note, it seems as though anytime Google publishes something people interpret it as “industry standard” (see their C++ style guide) and apply it to their non-Google projects. I personally don’t see this as healthy.

deleted 4y ago

[deleted]