Code review comment for lp:~jml/launchpad/sftp-poppy

On Thu, Mar 18, 2010 at 11:57 AM, Gavin Panella
<email address hidden> wrote:
> Review: Approve
> Looks good. Two comments:
>
> * It's not necessary to do waitpid(pid, os.WNOHANG) followed by
>  kill(pid, 0); the return value from waitpid() will be (0, 0) when
>  the process has not exited yet, and it will error the same as kill()
>  if the process does not exist.
>

Hmm. I see. I wasn't quite sure what the behaviour actually is, so I
did Science.

$ ps ax | grep 3711
 7702 pts/2 R+ 0:00 grep 3711
$ python
Python 2.6.4 (r264:75706, Dec 7 2009, 18:43:55)
[GCC 4.4.1] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import os

Spawn a long-running process

>>> pid = os.spawnv(os.P_NOWAIT, '/bin/sleep', ['/bin/sleep', '10'])
>>> pid
7820

waitpid on-existent process raises errors

>>> os.waitpid(3711, os.WNOHANG)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: [Errno 10] No child processes

waitpid returns (0, 0) while it's running for (pid, result)
>>> os.waitpid(7820, os.WNOHANG)
(0, 0)

Once it's terminated, returns (pid, result) with a non-zero pid.
>>> os.waitpid(7820, os.WNOHANG)
(7820, 0)

Now that it's terminated and we've reaped it, it'll raise an exception
>>> os.waitpid(7820, os.WNOHANG)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: [Errno 10] No child processes

I'll fix the code accordingly, making sure that we don't force a 0.1s
sleep if the SIGTERM was successful.

> * It might be nice to be able to pass a subprocess.Popen into
>  two_stage_kill, so that its poll() method is used, and so the
>  returncode attribute is set correctly to avoid confusion. But,
>  really, this is probably not worth the effort.

It's valuable having a low-level function that takes a pid. I agree
that what you describe would also be useful, but maybe not right now.
I wonder what we have to do to encourage others who might need this to
stick code into this module and re-use two-stage kill.

Thanks!
jml