Opened 8 years ago

Closed 2 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 Branch: branches/process-kill-2420-2
(diff, github, buildbot, log)
Author: therve Launchpad Bug:

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 8 years ago by itamarst

  • Owner changed from glyph to itamarst

comment:2 Changed 8 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 8 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 8 years ago by therve

  • Owner changed from itamarst to therve

After discussion, we'll wait for #2341.

comment:5 Changed 7 years ago by exarkun

#2787 is related probably.

comment:6 Changed 7 years ago by therve

  • author set to therve
  • Branch set to branches/process-kill-2420

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

comment:7 Changed 7 years ago by therve

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

Refs #2420

comment:8 Changed 7 years ago by therve

  • Keywords review added
  • Owner therve deleted

Ready to review.

comment:9 Changed 7 years ago by exarkun

  • 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 6 years ago by therve

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

Refs #2420

comment:11 Changed 4 years ago by jml

Hey Thomas, you should fix this. :)

comment:12 Changed 4 years ago by therve

  • Branch changed from branches/process-kill-2420 to branches/process-kill-2420-2

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

comment:13 Changed 4 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 follow-up: Changed 4 years ago by exarkun

  • 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 4 years ago by <automation>

  • Owner therve deleted

comment:16 Changed 3 years ago by thijs

  • Cc thijs added

comment:17 in reply to: ↑ 14 Changed 3 years ago by thijs

  • 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 2 years ago by therve

  • Resolution set to fixed
  • Status changed from new to closed

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