Better way for python to kill subprocesses on win32?

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

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.

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

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?

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)'?

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

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?

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 :-]

Patching Twisted how?

Replying to exarkun:

Patching Twisted how?

Using #2726 mentioned above.