os.kill in twisted.internet.process has no error handling

[itamarst]

It may be that our code will never trigger os.kill complaining about non-existent process, but we should handle it anyway.

[itamarst]

Implementation of error handling needs to be moved out of #2341, and then tests need to be added.

Any interest in doing this, therve?

[therve]

Of course :).

But if we don't wait for #2341, we'll have to duplicate the code here, no ?

[therve]

After discussion, we'll wait for #2341.

[exarkun]

#2787 is related probably.

[therve]

(In [23848]) Branching to 'process-kill-2420'

[therve]

(In [23849]) Handle ESRCH in os.kill.

Refs #2420

[therve]

Ready to review.

[exarkun]

• the way test_killExited relies on the call to os.waitpid in registerReapProcessHandler is pretty obscure (I couldn't figure it out, you had to explain it to me on IRC :). Some clarification would be great.
• I think the docstring for test_killExitedButNotDetected is slightly misleading. If a process exits but it hasn't been waited on yet (ie, it is a zombie) then os.kill will not raise an ESRCH error. The kill will succeed. You can only get ESRCH after the zombie is reaped. This is related to what has always been the problem with this ticket - no one knows how to trigger the ESRCH case in the real world. :P That makes it hard to describe the case test_killExitedButNotDetected is testing. Maybe the thing that could really cause this is if some other library/application code calls wait and gets a process Twisted started. So maybe describe it that way, instead? Of course, this case has a bunch of other bugs - like the fact that the process will never get processEnded notification, so fixing signalProcess only half fixes it.
• Another errno that seems more likely (though I've never seen it, so who knows) to happen is EPERM. At least I can imagine a legitimate use of the API which would result in this (eg, start a process as root, spawn a child, both processes then change uid/gid to values different from each other, then signalProcess is attempted - permission denied). It should be easy to test/handle this case as well, at least for POSIX. I suspect something similar can happen on windows if someone plays around with permissions enough, so it might be good to have a uniform exception for this case (or I might be wrong and they might not be similar enough for that, I dunno). This can be a separate ticket if you want, I just mention it because it's pretty related to these changes, and because it will slightly invalidate the docstring of test_killErrorInKill, although not the test code itself.

[therve]

(In [24162]) Add a comment, fix docstring.

Refs #2420

[jml]

Hey Thomas, you should fix this. :)

[therve]

(In [30368]) Branching to 'process-kill-2420-2'

[therve]

OK, it's ready to review I think. I didn't do anything about EPERM, we can open another ticket if it's considered important.

[exarkun]

1. Needs a news fragment
2. Some documentation somewhere needs to be updated, I guess, to explain that spawnProcess is allowed to fail with OSError sometimes (but not in the ESRCH case).
3. It would be nice if a similar test/fix could be done to the Windows process implementation. But this could be a separate ticket.
4. It would also be nice if this could be a test in twisted/internet/test/. I can see how that might challenging since no one knows how to trigger this case for real. :) Maybe for EPERM something is possible. This could be a separate ticket too.

Address to your satisfaction and then merge. Thanks!

[thijs]

Replying to exarkun:

Address to your satisfaction and then merge. Thanks!

I think this got lost, assigning it back to therve (of course feel free to unassign again).

[therve]

(In [34700]) Merge process-kill-2420-2

Author: therve Reviewer: exarkun Fixes: #2420

Handle the ESRCH errno possibly raised by os.kill in Process.signalProcess and raise ProcessExitedAlready instead.