Code review comment for lp:~vila/udd/735477-kill-harder

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 3/17/2011 3:50 PM, Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging lp:~vila/udd/735477-kill-harder into lp:udd.
>
> Requested reviews:
> Ubuntu Distributed Development Developers (udd)
> Related bugs:
> Bug #735477 in Ubuntu Distributed Development: "some imports survive a kill -SIGTERM leading to massive log output and no kill"
> https://bugs.launchpad.net/udd/+bug/735477
>
> For more details, see:
> https://code.launchpad.net/~vila/udd/735477-kill-harder/+merge/53837
>
> At least one import didn't die when killed with SIGTERM (nexuiz-data).
>
> This led to a bloated log with repeated attempts to kill it again.
>
> While SIGTERM should still be the preferred way to kill the imports (or we won't get meaningful tracebacks), we still need to be able to kill the imports reliably :)
>
> So this patches ensures that only the first kill for a given subprocess use SIGTERM and that, after a grace period (defaulting to 2 seconds), we'll use SIGKILL instead.

I think 2 seconds is a bit short for a grace period. If bzr is doing
anything significant. Something like 1 minute would be generous, and
still avoid having a child sitting around for days before manually
killing it.

Maybe 30s if you want something lower.

Why not use 'mkfifo' and then actually do inter-process communication,
instead of playing tricks with file content. mkfifo + open() will block
until the file actually is opened by the other side.

If you are worried that they'll never open the file and you'll block.
You can O_NONBLOCK and poll like you are doing now. Or use something
like signal.alarm() so that the test suite dies if the file never opens.
(You could even install a SIGALRM handler and just have EINTR triggered
on the open() call.)

Otherwise something like this seems fine. I certainly support falling
back to SIGKILL if SIGTERM/SIGINT aren't doing their job.

 review: needsfixing
 merge: approve

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk2CIyoACgkQJdeBCYSNAAMWLgCfe/e5ZMU5yltRfWDg+BCJKbgl
8XQAn3JCj73LrgRgyKaLdIBESI3UAdoY
=1waC
-----END PGP SIGNATURE-----