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.
<rdar://problem/78471882>
Created attachment 429686 [details] Patch
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?
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"
Created attachment 429950 [details] Patch
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)
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?
(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.
(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!
Created attachment 429964 [details] Patch for landing
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].