Bug 226236 – [webkitpy] Use existing signal handler when printing stacktrace

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

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.

[Radar WebKit Bug Importer]

<rdar://problem/78471882>

[Jonathan Bedard]

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?

[Jonathan Bedard]

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]

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?

[Jonathan Bedard]

(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]

(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]

Created attachment 429964 [details]
Patch for landing

[EWS]

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].