RESOLVED FIXED 226236
[webkitpy] Use existing signal handler when printing stacktrace
https://bugs.webkit.org/show_bug.cgi?id=226236
Summary [webkitpy] Use existing signal handler when printing stacktrace
Jonathan Bedard
Reported 2021-05-25 13:00:03 PDT
It is useful to retain the existing signal handler when printing a stack trace. This is particularly important when multiprocessing, since the SIGTERM and SIGINT handlers may not exit the process or raise an exception.
Attachments
Patch (2.04 KB, patch)
2021-05-25 13:14 PDT, Jonathan Bedard
no flags
Patch (3.98 KB, patch)
2021-05-27 16:25 PDT, Jonathan Bedard
no flags
Patch for landing (3.95 KB, patch)
2021-05-27 17:42 PDT, Jonathan Bedard
no flags
Radar WebKit Bug Importer
Comment 1 2021-05-25 13:00:35 PDT
Jonathan Bedard
Comment 2 2021-05-25 13:14:28 PDT
dewei_zhu
Comment 3 2021-05-27 15:35:34 PDT
Comment on attachment 429686 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429686&action=review > Tools/Scripts/webkitpy/common/interrupt_debugging.py:95 > def log_stack_trace_on_term(output_file=None): > + if not getattr(signal, 'SIGTERM'): > + return > + > + previous = signal.getsignal(signal.SIGTERM) > + > def handler(signum, frame): > with StackTraceFileContext(output_file=output_file) as file: > file.write('SIGTERM signal received') > log_stack_trace(frame, file) > > - exit(-1) > + return previous(signum, frame) > > - signal.signal(signal.SIGTERM, handler) > + return signal.signal(signal.SIGTERM, handler) > > > def log_stack_trace_on_ctrl_c(output_file=None): > + if not getattr(signal, 'SIGINT'): > + return > + > + previous = signal.getsignal(signal.SIGINT) > + > def handler(signum, frame): > with StackTraceFileContext(output_file=output_file) as file: > file.write('CTRL+C received\n') > log_stack_trace(frame, file) > > - raise KeyboardInterrupt > + return previous(signum, frame) > > - signal.signal(signal.SIGINT, handler) > + return signal.signal(signal.SIGINT, handler) Those two are almost identical, maybe we can do some refactor here?
Jonathan Bedard
Comment 4 2021-05-27 15:39:04 PDT
Comment on attachment 429686 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429686&action=review >> Tools/Scripts/webkitpy/common/interrupt_debugging.py:95 >> + return signal.signal(signal.SIGINT, handler) > > Those two are almost identical, maybe we can do some refactor here? We could convert them both to "log_stack_trace_on_signal"
Jonathan Bedard
Comment 5 2021-05-27 16:25:46 PDT
dewei_zhu
Comment 6 2021-05-27 16:36:54 PDT
Comment on attachment 429950 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429950&action=review r=me > Tools/Scripts/webkitpy/common/interrupt_debugging.py:66 > +def log_stack_trace_on_signal(signum, output_file=None): Would it a better logging info if we pass sig_name instead of signum? log_stack_trace_on_signal('SIGTERM', output_file=stack_trace_path) log_stack_trace_on_signal('SIGINT', output_file=stack_trace_path)
dewei_zhu
Comment 7 2021-05-27 16:40:18 PDT
Comment on attachment 429950 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429950&action=review > Tools/Scripts/webkitpy/common/interrupt_debugging.py:83 > + if previous == getattr(signal, 'SIG_DFL'): > + if signum_ == getattr(signal, 'SIGTERM'): > + sys.exit(-1) > + if signum_ == getattr(signal, 'SIGINT'): > + raise KeyboardInterrupt In previous version patch, we don't do those?
Jonathan Bedard
Comment 8 2021-05-27 17:23:19 PDT
(In reply to dewei_zhu from comment #7) > Comment on attachment 429950 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=429950&action=review > > > Tools/Scripts/webkitpy/common/interrupt_debugging.py:83 > > + if previous == getattr(signal, 'SIG_DFL'): > > + if signum_ == getattr(signal, 'SIGTERM'): > > + sys.exit(-1) > > + if signum_ == getattr(signal, 'SIGINT'): > > + raise KeyboardInterrupt > > In previous version patch, we don't do those? No, I realized (with the keyboard interrupt, specifically), the only reason it was "working" is that attempting to invoke an integer resulted in an exception.
Jonathan Bedard
Comment 9 2021-05-27 17:23:32 PDT
(In reply to dewei_zhu from comment #6) > Comment on attachment 429950 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=429950&action=review > > r=me > > > Tools/Scripts/webkitpy/common/interrupt_debugging.py:66 > > +def log_stack_trace_on_signal(signum, output_file=None): > > Would it a better logging info if we pass sig_name instead of signum? > > log_stack_trace_on_signal('SIGTERM', output_file=stack_trace_path) > log_stack_trace_on_signal('SIGINT', output_file=stack_trace_path) Good idea! Let me update for this!
Jonathan Bedard
Comment 10 2021-05-27 17:42:54 PDT
Created attachment 429964 [details] Patch for landing
EWS
Comment 11 2021-05-27 18:07:38 PDT
Committed r278184 (238227@main): <https://commits.webkit.org/238227@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 429964 [details].
Note You need to log in before you can comment on or make changes to this bug.