-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Handle background exceptions gracefully #5052
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
base: master
Are you sure you want to change the base?
Conversation
src/main/ApplicationImpl.cpp
Outdated
| { | ||
| JITTER_INJECT_DELAY(); | ||
| timer.Update(isSlow.checkElapsedTime()); | ||
| try |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m relucant to take a one-size-fits-all approach here. The system has a lot of nuanced failure modes, and in many cases catching an exception and shutting down gracefully isn’t actually the right thing to do. Sometimes the exception is a symptom of prior state corruption, and in those cases we really need to evaluate the specific scenario to decide whether we need to do something else, like a forced shutdown or an abort.
This change is specifically in the context of the new invariant, correct? If so, could we scope it to invariants only, so we don’t change failure behavior in other subsystems? Btw, currently strict invariants abort the program. With this change, it looks like we go into graceful shutdown, which doesn't seem right. I think we should probably abort for snapshot-based invariants as well (which would help spot problematic watchers).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for simplicity, I went ahead and just handled the snapshot invariant case. This is the only invariant that actually runs on the background, so it seemed to make sense just to special case the failure mode for this specific invariant.
47a7b06 to
1c8ce60
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds exception handling to the runStateSnapshotInvariant method to ensure background thread exceptions are caught and result in graceful shutdown via printErrorAndAbort, rather than leaving the exception unhandled.
Key changes:
- Wraps invariant snapshot checking in a try-catch block to handle exceptions in background thread execution
- Adds the
GlobalChecks.hinclude to accessprintErrorAndAbortfunction
1c8ce60 to
351aa86
Compare
Description
Currently, if a background thread throws, we don't properly go through graceful shutdown. This gracefully handles background exceptions.
Checklist
clang-formatv8.0.0 (viamake formator the Visual Studio extension)