Opened 10 years ago

Closed 5 years ago

#2420 defect closed fixed (fixed)

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

Reported by: itamarst Owned by: therve
Priority: normal Milestone:
Component: core Keywords:
Cc: therve, Thijs Triemstra Branch: branches/process-kill-2420-2
branch-diff, diff-cov, branch-cov, buildbot
Author: therve

Description

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

Change History (18)

comment:1 Changed 10 years ago by itamarst

Owner: changed from Glyph to itamarst

comment:2 Changed 10 years ago by itamarst

Cc: therve added

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

Any interest in doing this, therve?

comment:3 Changed 10 years ago by therve

Of course :).

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

comment:4 Changed 10 years ago by therve

Owner: changed from itamarst to therve

After discussion, we'll wait for #2341.

comment:5 Changed 10 years ago by Jean-Paul Calderone

#2787 is related probably.

comment:6 Changed 9 years ago by therve

author: therve
Branch: branches/process-kill-2420

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

comment:7 Changed 9 years ago by therve

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

Refs #2420

comment:8 Changed 9 years ago by therve

Keywords: review added
Owner: therve deleted

Ready to review.

comment:9 Changed 9 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to therve
  • 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.

comment:10 Changed 9 years ago by therve

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

Refs #2420

comment:11 Changed 7 years ago by Jonathan Lange

Hey Thomas, you should fix this. :)

comment:12 Changed 6 years ago by therve

Branch: branches/process-kill-2420branches/process-kill-2420-2

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

comment:13 Changed 6 years ago by therve

Keywords: review added
Owner: therve deleted

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.

comment:14 Changed 6 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to therve
  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!

comment:15 Changed 6 years ago by <automation>

Owner: therve deleted

comment:16 Changed 6 years ago by Thijs Triemstra

Cc: Thijs Triemstra added

comment:17 in reply to:  14 Changed 5 years ago by Thijs Triemstra

Owner: set to therve

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

comment:18 Changed 5 years ago by therve

Resolution: fixed
Status: newclosed

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

Note: See TracTickets for help on using tickets.