WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(3.98 KB, patch)
2021-05-27 16:25 PDT
,
Jonathan Bedard
no flags
Details
Formatted Diff
Diff
Patch for landing
(3.95 KB, patch)
2021-05-27 17:42 PDT
,
Jonathan Bedard
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2021-05-25 13:00:35 PDT
<
rdar://problem/78471882
>
Jonathan Bedard
Comment 2
2021-05-25 13:14:28 PDT
Created
attachment 429686
[details]
Patch
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
Created
attachment 429950
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug