-
Notifications
You must be signed in to change notification settings - Fork 9
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
1002 Add option to disable signal handlers in vt::finalize #1027
1002 Add option to disable signal handlers in vt::finalize #1027
Conversation
Worth Noting: |
Codecov Report
@@ Coverage Diff @@
## develop #1027 +/- ##
===========================================
- Coverage 77.61% 77.60% -0.01%
===========================================
Files 672 672
Lines 25761 25770 +9
===========================================
+ Hits 19994 20000 +6
- Misses 5767 5770 +3
|
What if the option has to be controlled via the normal Args? (And perhaps defaults to true..) What if the handlers always disabled on the finalize (and always re-added if specified) in initialize? Is there a case that code might wish to only sometimes disable on finalize? Is there a reason not to ensure the handlers are (re-setup) in initialize, if done? |
Adding an option to re-enable handlers in initalize() is a good idea i think |
8ed61a6
to
65c3e96
Compare
Ok so i added the flag which will indicate whether the signal handlers were disabled on last Based on the comment on top of the runtime.cc file i assume that contents of
|
@PhilMiller @lifflander Should we also disable termination handler? |
c1226dc
to
3c5f417
Compare
3c5f417
to
78669b9
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.
the code looks good (I don't know if there's anything missing from the functional point of view?)
78669b9
to
4f04338
Compare
signal(SIGSEGV, SIG_DFL); | ||
signal(SIGUSR1, SIG_DFL); | ||
signal(SIGINT, SIG_DFL); | ||
sig_handlers_disabled_ = true; |
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.
Could also be in the positive case: sig_handlers_attached_ = false;
.
@JacobDomagala From now on, can you start your branch names with just the issue number, not a |
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.
Looks good to me
…me component was finalized before
4f04338
to
f7653cd
Compare
Fixes #1002