Opened 13 years ago

Closed 5 years ago

#77 closed defect (fixed)

Better way for python to kill subprocesses on win32?

Reported by: joduinn Owned by: warner
Priority: major Milestone: 0.9.+
Version: 0.8.5 Keywords: windows, kill
Cc: bhearsum@…, bugs@…, afri, vasslitvinov@…

Description (last modified by dustin)

From offline discussions, seems that there might be a better way to kill subprocesses when running on win32. Details are at: http://benjamin.smedbergs.us/blog/2006-12-11/killableprocesspy/

Source code available at: SVN: http://svn.smedbergs.us/python-processes/trunk/

Change History (23)

comment:1 Changed 13 years ago by warner

  • Milestone changed from undecided to 0.7.7

comment:2 Changed 12 years ago by warner

  • Milestone changed from 0.7.7 to 0.7.8

no progress on this yet, bumping to 0.7.8 . We need a patch, and unit tests.

comment:3 follow-up: Changed 12 years ago by exarkun

What does better mean in this ticket's description?

There are cases where buildbot fails to kill the processes it starts entirely; there are cases where buildbot kills the process it started, but grandchildren are left running. Those would be useful to fix.

From looking at the linked page, I get the impression that the former case isn't fixed by the referenced code, though the latter is. However, switching buildbot to a process control API that doesn't use Twisted seems counterproductive. Adding job support to Twisted's process API would be simpler, since it won't require the tricky contortions that a blocking process control API will require.

There's a ticket in the Twisted tracker for supporting job groups.

comment:4 Changed 12 years ago by rhelmer

http://twistedmatrix.com/trac/ticket/2726 seems to make subprocesses reliably killable on win32, on my staging buildbot setup.

comment:5 in reply to: ↑ 3 Changed 12 years ago by rhelmer

Replying to exarkun:

What does better mean in this ticket's description?

Sorry if this seems silly, I just want to go through a real-life example we hit all the time:

Buildbot does not currently kill any child processes spawned on win32. So make for example might spawn link. Let's imagine it hangs and times out, and so Buildbot uses process.signalProcess to kill make, but link is still running.

This is particularly bad on win32 because the files held open by link cannot be removed, so manual intervention is needed to get the slave functional again.

There are cases where buildbot fails to kill the processes it starts entirely; there are cases where buildbot kills the process it started, but grandchildren are left running. Those would be useful to fix.

From looking at the linked page, I get the impression that the former case isn't fixed by the referenced code, though the latter is. However, switching buildbot to a process control API that doesn't use Twisted seems counterproductive. Adding job support to Twisted's process API would be simpler, since it won't require the tricky contortions that a blocking process control API will require.

There's a ticket in the Twisted tracker for supporting job groups.

In the POSIX case, Buildbot uses os.kill(-pid), which kills the process group. I imagine that if there was a cross-platform way to do this, then Buildbot would use it instead (job objects are pretty much analogous to process groups, as far as this particular use case is concerned, I think). I'll let Brian comment here, but seems like it'd simplify life on the Buildbot side not to do this: http://buildbot.net/trac/browser/buildbot/slave/commands.py#L499

The comment in there implies that process groups can't be used unless spawnProcess() was called with usePTY=1 (I have not idea what this implies :) ), so not sure what the right thing to do is.

This is discussed further in http://twistedmatrix.com/trac/ticket/2726 Is there another ticket about Job groups or is this the one you mean?

comment:6 Changed 12 years ago by bhearsum

  • Cc bhearsum@… added

comment:7 follow-up: Changed 12 years ago by bhearsum

What are we planning to do here? Are we just going to suffer through this until Twisted can kill processes better?

We already depend on Pywin32, can we just use it's win32api module to do the win32 equivalent of 'os.kill(-pid)'?

comment:8 in reply to: ↑ 7 ; follow-up: Changed 12 years ago by Almad

Replying to bhearsum:

We already depend on Pywin32, can we just use it's win32api module to do the win32 equivalent of 'os.kill(-pid)'?

+1 for this. I'll try to do it in my branch of buildbot, as we're suffering from it too (and it's blocker for our usage on windows, as if our application hangs, it blocks it's port.

Through I understand reasons from twisted team, I'd vote for monkeypatch until it's fix (especially when I can add half a year until fixed twisted will be available under debian...).

comment:9 Changed 12 years ago by Almad

  • Cc bugs@… added

comment:10 in reply to: ↑ 8 ; follow-up: Changed 12 years ago by bhearsum

Replying to Almad:

Replying to bhearsum:

We already depend on Pywin32, can we just use it's win32api module to do the win32 equivalent of 'os.kill(-pid)'?

+1 for this. I'll try to do it in my branch of buildbot, as we're suffering from it too (and it's blocker for our usage on windows, as if our application hangs, it blocks it's port.

Any luck with this?

comment:11 in reply to: ↑ 10 Changed 12 years ago by Almad

Replying to bhearsum:

Any luck with this?

I dug it in and found that fixing it in buildbot is not easy and ended up with patching twisted on windows machines :-]

comment:12 follow-up: Changed 12 years ago by exarkun

Patching Twisted how?

comment:13 in reply to: ↑ 12 Changed 12 years ago by Almad

Replying to exarkun:

Patching Twisted how?

Using #2726 mentioned above.

comment:14 Changed 12 years ago by afri

  • Cc afri added

comment:15 Changed 11 years ago by dustin

  • Milestone changed from 0.8.0 to undecided
  • Resolution set to fixed
  • Status changed from new to closed

It *sounds* like this is fixed, or should be, in Twisted. Please re-open if there's anything to do in bbot?

comment:16 Changed 8 years ago by marcusl

  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Version changed from 0.7.5 to 0.8.5

This is not fixed in 0.8.5.

Some options are listed in http://stackoverflow.com/questions/1230669/subprocess-deleting-child-processes-in-windows

I recommend just calling taskkill.exe, as it works realiably on windows xp and up.

Will see if I can provide a patch.

Last edited 8 years ago by marcusl (previous) (diff)

comment:17 Changed 8 years ago by dustin

  • Milestone changed from undecided to 0.8.+

comment:18 Changed 7 years ago by dustin

  • Description modified (diff)
  • Keywords windows kill added

comment:19 Changed 7 years ago by tom.prince

This seems like something that it would be worthwhile addressing upstream in twisted (and then monkey-patching the fix in buildbot)

comment:20 Changed 6 years ago by dustin

  • Milestone changed from 0.8.+ to 0.9.+

Ticket retargeted after milestone closed

comment:21 Changed 5 years ago by JustAMan

Is this still alive? I mean, Buildslave kills processes on Windows using taskkill.exe for quite a while now...

comment:22 Changed 5 years ago by JustAMan

  • Cc vasslitvinov@… added

comment:23 Changed 5 years ago by dustin

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

Indeed, Amber landed this 4 years ago:

commit ac07b5d2678ef2d1fd7fc23b2777faf5072c9852
Author: Amber Yust <ayust@yelp.com>
Date:   Thu Feb 10 15:34:49 2011 -0800

    Use taskkill to kill windows procs
Note: See TracTickets for help on using tickets.