feat: add safe package for unified goroutine recovery#5528
Conversation
janos
requested review from
acud,
akrem-chabchoub,
gacevicljubisa,
martinconic and
sbackend123
|
if goroutines that are supposed to panic are recovered gracefully and do not cause the application to crash, how do we recover from a state where: |
That is per goroutine for us to decide. Some goroutines are contained for a specific operation that should not terminate the application. For example, the joiner slice out of bounds panic was related only to one resource to be loaded and had no impact on the operations of the whole application and that panic could have been be recovered. And for the goroutines related to the database should be evaluated if they should be recovered. That is why I have added protection in goroutines that I could find so that they can be evaluated, case per case, as are visible in this PR changeset. |
gacevicljubisa
left a comment
gacevicljubisa
left a comment
There was a problem hiding this comment.
Overall, the PR is well scoped and with clear idea. I would suggest adding some metrics that could track total recovered panics.
| safe.Run(logger, "reserve-validation-worker", func() { | ||
| buf := make([]byte, swarm.SocMaxChunkSize) | ||
| for item := range iteratateItemsC { | ||
| validChunk(item, buf[:item.Location.Length]) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Would be better if we recover per item? That way one corrupt item will not tear down the worker?
| if !validChunk(item, buf[:item.Location.Length]) { | ||
| invalid.Add(1) | ||
| } |
There was a problem hiding this comment.
Maybe here as well we could recover per item?
| func Run(logger log.Logger, name string, fn func()) { | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| logger.Error(nil, "panic recovered", "name", name, "panic", fmt.Sprintf("%v", r), "stack", string(debug.Stack())) |
There was a problem hiding this comment.
it is a minor thing, but maybe we could check if logger is nil?
There was a problem hiding this comment.
and then you completely lose sight of the fact that there was a panic...
There was a problem hiding this comment.
and then you completely lose sight of the fact that there was a panic...
actually, it will panic if the logger is nil, as the recover is already called, but the check for nil logger is a good suggestion
There was a problem hiding this comment.
yes so what i meant it... if it panicked and there is no logger, at least you get a panic now (so you know there was even a panic in the first place).
if you check if the logger is nil and don't even call it - then you will: a. never know that a panic was recovered (unless you add metrics), and b. never know that the logger was not set (which is a severe engineering human error, because someone wrote code that did not set the logger where it should have been set). so my claim is that when we do all of these defensive programming we actually lose feedback over the code.
if the choice is between: 1. having a panic that propagates because of a real error, or 2. having a panic that does not show up at all (and be silently recovered, potentially putting the process into an inconsistent state because someone forgot to set a logger up properly then i prefer to have a panic crash the process honestly but that's just me.
or to conclude: i like panics :)
There was a problem hiding this comment.
Actually, you will get the panic that the logger is nil as it was a method call on the nil logger. That does mean that the original panic is lost, but because of that I have added the check on the nil logger as Ljubisa suggested.
We do not need to recover from panics. This PR is just mu suggestion, it depends on your preference. My opinion is that recovering from panics depends on the actual logic in the goroutine. Recent panics in the code show that there are localized problems that could be errors, not crashing the node (joiner bounds and nil bzz address).
The defensive programming is mostly related to the user experience. Where an error can be logged instead of crashing the node, it appears to me like a better user experience.
| return | ||
| } | ||
|
|
||
| mtx.Lock() |
There was a problem hiding this comment.
maybe to defer mtx.Unlock() here if panic happens in IsWithinStorageRadius
3 participants
Checklist
Description
This PR introduces a unified panic recovery approach to protect the Bee node from unexpected crashes in goroutines.
By catching panics inside worker routines, checker loops, and handler callbacks, it is ensures that unexpected panics are safely recovered and converted into either structured log messages or standard Go errors, rather than causing the entire node process to crash.
I have added a new small package in ./pkg/safe for functions that unify the recovery for the bee node.
Other changes are the usage of these functions in various goroutines in the code. Waith groups and errors groups create goroutines internally.
Open API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Prevent bee node from crashing on panics.
Related Issue (Optional)
Screenshots (if appropriate):
AI Disclosure