1475058 - Try SIGINT before escalating to SIGKILL when interrupting the build process

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Chris Manchester (limited bugmail, email directly)]

As mshal found recently, the behavior of mozprocess' kill() method is to send SIGTERM, wait for a second, and then send SIGKILL. This can cause problems if the build process is doing critical clean up work in response to the first signal.

Comment 1

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 1475058 - Send SIGINT when interrupting interactive in mach before sending SIGKILL.

Review commit: https://reviewboard.mozilla.org/r/256362/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256362/

Comment 2

[Michael Shal [:mshal]]

Comment on attachment 8991480 [details]
Bug 1475058 - Send SIGINT when interrupting interactive in mach before sending SIGKILL.

https://reviewboard.mozilla.org/r/256362/#review263438

::: python/mach/mach/mixin/process.py:151
(Diff revision 1)
> -                    status = p.kill()
> +                    if sig is None:
> +                        sig = signal.SIGINT
> +                    elif sig == signal.SIGINT:
> +                        # If we've already tried SIGINT, escalate.
> +                        sig = signal.SIGKILL
> +                    status = p.kill(sig=sig)

It looks like the p.kill() call has a built-in wait, so if I hit Ctrl-C, we call p.kill(sig=SIGINT), and then if I hit Ctrl-C again, mach is already in this exception handler and dies with:

mach interrupted by signal or user action. Stopping.

This leaves the tup process running in the background (which soon exits since it received the SIGINT). So it never ends up doing a p.kill(sig=SIGKILL).

I think for this to work we need to also update mozprocess to have ProcessHandler:kill() and ProcessHandlerMixin:kill() avoid calling wait() directly (and I suppose not return a status code?)

Then this while loop would call p.kill(sig=SIGINT), loop back to status = p.wait(), and on a second ctrl-C would do the p.kill(sig=SIGKILL)

I'm not sure if removing the wait() from kill() would have side-effects for other consumers, though.

Comment 3

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8991480 [details]
Bug 1475058 - Send SIGINT when interrupting interactive in mach before sending SIGKILL.

https://reviewboard.mozilla.org/r/256362/#review263438

> It looks like the p.kill() call has a built-in wait, so if I hit Ctrl-C, we call p.kill(sig=SIGINT), and then if I hit Ctrl-C again, mach is already in this exception handler and dies with:
> 
> mach interrupted by signal or user action. Stopping.
> 
> This leaves the tup process running in the background (which soon exits since it received the SIGINT). So it never ends up doing a p.kill(sig=SIGKILL).
> 
> I think for this to work we need to also update mozprocess to have ProcessHandler:kill() and ProcessHandlerMixin:kill() avoid calling wait() directly (and I suppose not return a status code?)
> 
> Then this while loop would call p.kill(sig=SIGINT), loop back to status = p.wait(), and on a second ctrl-C would do the p.kill(sig=SIGKILL)
> 
> I'm not sure if removing the wait() from kill() would have side-effects for other consumers, though.

Good catch, I didn't realize kill() contained a wait(). I think we can rework what we have in process.py to fix this...

Comment 4

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8991480 [details]
Bug 1475058 - Send SIGINT when interrupting interactive in mach before sending SIGKILL.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256362/diff/1-2/

Comment 5

[Michael Shal [:mshal]]

Comment on attachment 8991480 [details]
Bug 1475058 - Send SIGINT when interrupting interactive in mach before sending SIGKILL.

https://reviewboard.mozilla.org/r/256362/#review263532

Looks good to me - thanks for fixing this!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8658af4e92de
Send SIGINT when interrupting interactive in mach before sending SIGKILL. r=mshal

Comment 7

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/8658af4e92de