doCreate should be overridable in dumbwin32proc

[k0s]

doCreate lives within the scope of a function:

http://twistedmatrix.com/trac/browser/trunk/twisted/internet/_dumbwin32proc.py#L172

if doCreate was made either a callback or an accessible function (so it could be monkey-patched) then this would give the flexibility needed for end consumers of twisted software (such as buildbot) to pass appropriate process creation flags, etc.

Since, for instance, a patch we must apply for our buildslaves:

 http://people.mozilla.org/~anodelman/killprocess/_dumbwin32proc.py

[exarkun]

This seems like a duplicate of #2726 to me. The patch in question is the same (and arguably the user requesting the feature is the same ;). Only the details of the solution are different.

[k0s]

Replying to exarkun:

This seems like a duplicate of #2726 to me. The patch in question is the same (and arguably the user requesting the feature is the same ;). Only the details of the solution are different.

The patch and the intention behind the patch are the same as in #2726. However, the patch is given only as an example of why a patch must be applied using existing twisted code. If doCreate was overridable, then there wouldn't be a need for a patch.

[exarkun]

reactor.spawnProcess is a cross-platform API. There's no equivalent to the doCreate function on POSIX, so exposing it introduces (another) platform-specific behavior. It would be preferable to fix the specific issues people encounter (such as not being able to kill processes reliably) while preserving the cross-platform-ness of the API. So, if the only use case is that one, then fixing #2726 makes sense. If there are other use cases, let's talk about them.

[k0s]

Replying to exarkun:

reactor.spawnProcess is a cross-platform API. There's no equivalent to the doCreate function on POSIX, so exposing it introduces (another) platform-specific behavior. It would be preferable to fix the specific issues people encounter (such as not being able to kill processes reliably) while preserving the cross-platform-ness of the API. So, if the only use case is that one, then fixing #2726 makes sense. If there are other use cases, let's talk about them.

I'm not sure if I fully understand the platform-specific comment. Since dumbwin32proc is by nature a windows API, I'm not sure what this has to do with POSIX. Jobs are handled differently on the different platforms. In order to confidently kill processes under windows, one should use job objects, hence #2726. However, if the process that is desired to be spawned also wishes to create job objects, then the patches on #2726 are not adequate and assigning to the job object in the child process will fail. The creation and job flags that you want for creation are likely to be use-case specific and have no analog in the POSIX world. Hence, I would like doCreate, and possibly other functionality as well, to be overridable. If it lives as an instance method, this is already doable. Buried as it is inside a function, doCreate is inaccessible and cannot be sensibly overridden.

My specific use case is thus:

• I have a test harness, mozmill, that spawns processes and a job object to clean up after itself
• I am running this test harness under buildbot; to be specific, a build slave
• The twisted code is patched on the buildslave using a patch from #2726 . The slave is patched so that other test harnesses and build steps are correctly cleaned up
• However, the patch prohibits mozmill from creating and assigning to job objects
• if doCreate and similar functionality were first class objects, and the API written such that e.g. monkey-patching were available, our deployment story would be much better

As such, and until such a time that twisted can do these things, our deployment story consists of downgrading twisted and applying a patch:  https://wiki.mozilla.org/ReferencePlatforms/Win32#Downgrade_twisted_and_apply_process_group_patch . The fix is therefore likely to be to alter this patch to serve our current needs. I would prefer if twisted incorporated this functionality and perhaps others would find it useful. Currently, twisted cannot guarantee killing descendent processes under windows (or, if it can, then we shouldn't be using the patch, but my coworkers strongly believe that it is necessary). Applying the patch doesn't allow launching of anything that allows a job object.

So my actual requirements, reflective of this type of problem are:

• twisted should kill all subprocesses on windows robustly
• twisted should allow subprocesses on windows to use jobs

I would assume that different use-cases would want different process creation and job creation flags, but ultimately, as long as the flags match my use-case that is all I care about.

[glyph]

Replying to k0s:

Replying to exarkun:

reactor.spawnProcess is a cross-platform API. There's no equivalent to the doCreate function on POSIX, so exposing it introduces (another) platform-specific behavior. It would be preferable to fix the specific issues people encounter (such as not being able to kill processes reliably) while preserving the cross-platform-ness of the API. So, if the only use case is that one, then fixing #2726 makes sense. If there are other use cases, let's talk about them.

I'm not sure if I fully understand the platform-specific comment. Since dumbwin32proc is by nature a windows API,

_dumbwin32proc (the underscore is important) is not an "API". An API is an application programming interface, i.e. a published, documented function which should be used by application programmers to accomplished some task.

_dumbwin32proc is a pile of code. Any publication or advertisement of this to application programmers is a mistake or a bug. The underscore prefix is supposed to tell you "do not call this, it is not for public consumption".

Of course you can call it, but that's not our problem :). The supported API you should use to reach this code is spawnProcess, so any modification to the interface needs to consider spawnProcess.

I'm not sure what this has to do with POSIX. Jobs are handled differently on the different platforms. In order to confidently kill processes under windows, one should use job objects, hence #2726. However, if the process that is desired to be spawned also wishes to create job objects, then the patches on #2726 are not adequate and assigning to the job object in the child process will fail. The creation and job flags that you want for creation are likely to be use-case specific and have no analog in the POSIX world. Hence, I would like doCreate, and possibly other functionality as well, to be overridable. If it lives as an instance method, this is already doable. Buried as it is inside a function, doCreate is inaccessible and cannot be sensibly overridden.

The way to override it, if it's necessary, would be a platform-specific parameter to spawnProcess. I am with exarkun here in that I would like to avoid the creation of such a parameter if we can. So the question is, can we satisfy these requirements...

So my actual requirements, reflective of this type of problem are: - twisted should kill all subprocesses on windows robustly - twisted should allow subprocesses on windows to use jobs

... which seem perfectly reasonable to me, with the same set of flags. Are they really mutually exclusive? Is there no way to reliably terminate a process without using job objects?

I would assume that different use-cases would want different process creation and job creation flags, but ultimately, as long as the flags match my use-case that is all I care about.

Since the flags that you've specified here are (PLEASE_LET_TERMINATION_WORK | PLEASE_LET_JOBS_WORK), I can't see why anyone would not want to set these. Also, as discussed extensively on #2726, different flags may affect the way that the implementation (for example, the technique used for signalProcess) needs to work, so if we are going to support arbitrary win32-specific flags in spawnProcess, the code would also need to inspect those flags and understand them as well, which means that the maintainers would need to go through the laborious process of examining each flag and determining whether it needs to affect our implementation or not.