Justin Mazzola Paluska writes: Unrelated.

def unstick(): reapAllProcesses() reactor.callLater(1, unstick)

Why do you use `unstick` instead of `reapAllProcesses` directly. Do you expect that `reapAllProcesses` name will be rebound in the future? -- Akira

Problem solved... see below On 2 September 2014 12:05, Justin Mazzola Paluska wrote:

On 09/02/2014 05:08 AM, Adi Roiban wrote: [snip]

if pid: self.processEnded(status) unregisterReapProcessHandler(pid, self) elif pid == 0: # Twisted seems to get stuck if pid is 0, which means that # the child process hasn't changed status, but if called # after SIGCHLD probably means that the child process is # in the process of dying, but hasn't quite died yet. # We'll try to kick the reactor to reap the processes # again in a bit. # # We're testing specifically against 0 because pid may # also be None in an error case. def unstick(): reapAllProcesses() reactor.callLater(1, unstick)

_BaseProcess.reapProcess = reapProcess

----

To use this, import your reactor and then call workaround_reapProcess(reactor).

Now that two of us have seen the same problem, we should probably file a ticket in the bug tracker. --Justin

My quick fix was to only call reactor.callLater(self.reapProcess) and not to reap all processes ---------- I dig deeper and I found out that since I was using reactor.run(installSignalHandlers=False) _SIGCHLDWaker was not installed. I have switched to using just reactor.run() and the process is now killed. Thanks! -- Adi Roiban

On 01:38 pm, jmp@editshare.com wrote:

FWIW, looking at your original code, you may also want to call your Deferreds in a reactor.callLater. I learned the hard way that if you callback a Deferred from processEnded and the callback spawns another process, your process will still be Twisted's process table even though it's dying.

...

----------

I dig deeper and I found out that since I was using reactor.run(installSignalHandlers=False) _SIGCHLDWaker was not installed.

I have switched to using just reactor.run() and the process is now killed.

Without my workaround, I continue to have the problem with the gtk2reactor.

Have you reported this bug? Jean-Paul

On 2 September 2014 14:50, wrote:

On 01:38 pm, jmp@editshare.com wrote:

...

[snip]

...

Without my workaround, I continue to have the problem with the gtk2reactor.

Have you reported this bug?

Jean-Paul

In my initial use case, signal handlers were not installed since I wanted a custom behaviour for SIGINT, SIGTERM, SIGBREAK so that I can use my custom Unix Daemon and Windows Service adapters. I understand why reactor.run has the installSignalHandlers argument. Do you think it would make sense to have some kind of public API which would skip handlers for SIGINT, SIGTERM, SIGBREAK (as they only do reactor.stop()) but would install the SIGCHLD handler? Thanks! -- Adi Roiban

On 3 September 2014 14:39, wrote:

On 01:05 pm, adi@roiban.ro wrote:

...

On 2 September 2014 14:50, wrote:

...

On 01:38 pm, jmp@editshare.com wrote:

...

[snip]

...

...

Without my workaround, I continue to have the problem with the gtk2reactor.

Have you reported this bug?

Jean-Paul

In my initial use case, signal handlers were not installed since I wanted a custom behaviour for SIGINT, SIGTERM, SIGBREAK so that I can use my custom Unix Daemon and Windows Service adapters.

I understand why reactor.run has the installSignalHandlers argument.

Do you think it would make sense to have some kind of public API which would skip handlers for SIGINT, SIGTERM, SIGBREAK (as they only do reactor.stop()) but would install the SIGCHLD handler?

Yes. Providing more fine-grain control over signal handlers would be a fine improvement.

Do you have any suggestion for how the calls should be made? reactor.run(installSignalHandlers=True, installStopHandlers=False) or reactor.installStopHandlers = False reactor.run()

Another fine improvement would be making child processes work even if a SIGCHLD handler cannot be installed (for example, by polling children periodically, by using wait() in a sidecar process, or by using a better system-specific child process monitoring API (eg kqueue's EVFILT_PROC)).

I see that GlibReactorBase inherits from PosixReactorBase so it should install the SIGCHLD handler... this should not be the reason why gtk2 reactor don't work. As a poor man's fix and Unix independent fix maybe we can call reapAllProcess in a task.LoopingCall... What are the reasons why SIGCHLD handler cannot be installed? I am asking since I hope it could help me understant where and how to enable the poor man's fix... and how to fill the bug report. kqueue's EVFILT_PROC would be great, but I guess that we still need a general fix --------- For the record: Right now, to ignore SIGINT, SIGTERM, SIGBREAK handles but keep SIGCHLD I do: # Patch base reactor to not install SIGINT, SIGTERM and SIGBREAK handlers _SignalReactorMixin._handleSignals = lambda self: None reactor.run() -- Adi Roiban

On 03:27 pm, adi@roiban.ro wrote:

On 3 September 2014 14:39, wrote:

...

Yes. Providing more fine-grain control over signal handlers would be a fine improvement.

Do you have any suggestion for how the calls should be made?

reactor.run(installSignalHandlers=True, installStopHandlers=False)

Perhaps.

or

reactor.installStopHandlers = False reactor.run()

Probably not this one. Setting attributes on random things is kind of sad. :) It might be nice to try to be somewhat flexible - in case there's some reason to change what signals the reactor wants to handle in the future. Perhaps: reactor.run(installSignalHandlers={SIGCHLD}) An entirely different direction could be to make this bit of configuration into initialization for the reactor. from twisted.internet.epollreactor import install install(installSignalHandlers={SIGCHLD}) from twisted.internet import reactor ... reactor.run() By keeping these details away from `IReactorCore.run`, that method remains maximally useful. For example, if you could set up the reactor this way, a normal `twistd` plugin would still be able to benefit from your choice, even with twistd's naive call of `reactor.run()` with no extra arguments. Application code calling these `install` functions is already supported (it's how you select a specific reactor, after all). Some of the install functions even accept arguments already. This would actually eliminate another existing issue - `IReactorCore.run` is actually defined to take no arguments. The implementations ignore this because someone thought it was important to be able to disable installation of signal handlers.

...

Another fine improvement would be making child processes work even if a SIGCHLD handler cannot be installed (for example, by polling children periodically, by using wait() in a sidecar process, or by using a better system-specific child process monitoring API (eg kqueue's EVFILT_PROC)).

I see that GlibReactorBase inherits from PosixReactorBase so it should install the SIGCHLD handler... this should not be the reason why gtk2 reactor don't work.

Gtk messes with signals too. There are confusing order-of-execution dependencies and Gtk changes subtly from release to release, re-breaking things after we fix them or changing them to be broken in a different way. So that's *why* it probably doesn't work with Gtk2 reactor - if not *how*. ;) I think I missed the explanation of what in particular wasn't working with Gtk2 reactor though.

As a poor man's fix and Unix independent fix maybe we can call reapAllProcess in a task.LoopingCall... What are the reasons why SIGCHLD handler cannot be installed?

Either because you want to run the reactor in a non-main thread (where Python won't let you install signal handlers for the good reason that mixing signals and threads has crazy behavior) or because you want to use a different library that depends on having its own SIGCHLD handler and you're not interested in Twisted's process support.

I am asking since I hope it could help me understant where and how to enable the poor man's fix... and how to fill the bug report.

kqueue's EVFILT_PROC would be great, but I guess that we still need a general fix

Perhaps, perhaps not. A general fix might be less code but having half a dozen specialized fixes, one for each reactor, would also still fix the problem. The different reactor implementations are essentially the big piles of specialized fixes needed to do non-blocking I/O on different platforms. This would just be a little more of the same. The sidecar process is an example of a general fix, though. The idea there is that Twisted itself runs a private child process (perhaps only when the first call to spawnProcess is made). It talks to that process using a file descriptor. That process can install a SIGCHLD handler (because Twisted owns it, application developers don't get to say they don't want one installed) or use another more invasive strategy for child process management. When you want to spawn a process, the main process tells the sidecar to do it. The sidecar relays traffic between the child and the original parent (or does something involving passing file descriptors across processes). This removes the need to ever install a SIGCHLD handler in the main process. It also probably enables some optimizations (reapProcesses is O(N!) on the number of child processes right now) that are very tricky or impossible otherwise. Jean-Paul

---------

For the record: Right now, to ignore SIGINT, SIGTERM, SIGBREAK handles but keep SIGCHLD I do:

# Patch base reactor to not install SIGINT, SIGTERM and SIGBREAK handlers _SignalReactorMixin._handleSignals = lambda self: None reactor.run()

-- Adi Roiban

On 3 September 2014 18:55, wrote:

On 03:27 pm, adi@roiban.ro wrote:

...

On 3 September 2014 14:39, wrote:

[snip]

...

Do you have any suggestion for how the calls should be made?

reactor.run(installSignalHandlers=True, installStopHandlers=False)

Perhaps. [snip]

It might be nice to try to be somewhat flexible - in case there's some reason to change what signals the reactor wants to handle in the future. Perhaps:

reactor.run(installSignalHandlers={SIGCHLD})

An entirely different direction could be to make this bit of configuration into initialization for the reactor.

from twisted.internet.epollreactor import install install(installSignalHandlers={SIGCHLD})

from twisted.internet import reactor ... reactor.run()

By keeping these details away from `IReactorCore.run`, that method remains maximally useful. For example, if you could set up the reactor this way, a normal `twistd` plugin would still be able to benefit from your choice, even with twistd's naive call of `reactor.run()` with no extra arguments.

Application code calling these `install` functions is already supported (it's how you select a specific reactor, after all). Some of the install functions even accept arguments already.

This would actually eliminate another existing issue - `IReactorCore.run` is actually defined to take no arguments. The implementations ignore this because someone thought it was important to be able to disable installation of signal handlers.

I am happy to have a simple reactor.run() and move installSignalHandlers somewhere else. working with install(installSignalHandlers={SIGCHLD}) seems a bit complicated, as I assume that many developers rely on the automatic reactor installation. In the same time, I assume that 'installSignalHandlers' argument would be supported by all reactors this is why maybe we can have something like: from twisted.internet import reactor def customHandler(signum, frame): pass reactor.installSignalHandlers( SIGCHLD=True, # Install default handler SIGTERM=None, # Don't install handler SIGINT=customHandler, # Install custom handler # SIGBREAK is not request so that default handler is installed. ) # reactor.installSignalHandlers() installs all default handlers. reactor.run() ---- reactor.run(InstallSignalHandlers=True|False) would be deprecated. In case reactor.installSignalHandlers is not called before run(), all default handlers will be installed. [snip]

The sidecar process is an example of a general fix, though. The idea there is that Twisted itself runs a private child process (perhaps only when the first call to spawnProcess is made). It talks to that process using a file descriptor. That process can install a SIGCHLD handler (because Twisted owns it, application developers don't get to say they don't want one installed) or use another more invasive strategy for child process management. When you want to spawn a process, the main process tells the sidecar to do it. The sidecar relays traffic between the child and the original parent (or does something involving passing file descriptors across processes).

This removes the need to ever install a SIGCHLD handler in the main process. It also probably enables some optimizations (reapProcesses is O(N!) on the number of child processes right now) that are very tricky or impossible otherwise.

Jean-Paul

Thanks for the details regarding the side-process dedicated to child process management. Not sure if we need a separate ticket for that, or add it as a comment to https://twistedmatrix.com/trac/ticket/5710 Thanks! -- Adi Roiban

On 4 September 2014 12:59, wrote: [snip]

What would be really nice is if someone collected *all* of the complaints about `spawnProcess` into one place and integrated solutions to them into a design for a replacement. :)

Jean-Paul

Since I can not create wiki pages here are my notes: To do: * Always return a deferred. #2415 * Provide the option to timeout the execution... and on timeout errback with a dedicated failure #2415 * Add childs to the same process group #2415 * allways errback when failing to spawn (rather than raising OSError) and errback with a different failure when failing to spawn the executable #4184 * on Unix provide a method to install SIGCHLD handler, independent of the general reactor.run() signal handlers. ... maybe related to #5710 * Allow passing Unicode environment (at least on Windows) maybe needed for Python3 #5968 * Allow passing Unicode command and arguments on Windows (current not supported by pywin32 CreateProcess) #6470... maybe needed for Python3 #5968 * Improve fork/exec for speed and memory usage #5710 .... maybe by creating a auxiliary process which handles forks and child management. * Execute as a different account in Windows * Spawn only after reactor starts ? I am ok with forking before. * Support client endpoint? ------- For a new design: * I like the ProcessProtocol API * spawnProcess will always return a deferred... for backward compatibility we need to decide a name for this new method which returns a deferred. Maybe createProcess or connectProcess * create spawnUnixProcess which has childFDs uid/gid * create spawnWindowsProcess which has Windows specific options * createProcess / connectProcess will call spawnUnixProcess or spawnWindowsProcess depending on current OS * ProcessProtocol.processEnded is similar to connectionLost.. so maybe it can be rename it and add other methods so that IProcessProtocol can inherit from IProtocol. I guess that this will help with connectors. ------ I think that child management problem, does not affect the public API so I am not discussing it here. Timeouts are handled at protocol level and I see that we have 2 timeout types: total execution time and timeout after last child data was received. ----- Does it make sense to put it on wiki or what should be the Twisted Enhancement Proposal process? Can I get 'create page' rights on Trac? Thanks! -- Adi Roiban

On 5 September 2014 13:13, wrote:

On 06:30 am, adi@roiban.ro wrote: [snip]

...

For a new design:

* I like the ProcessProtocol API

I don't. :) ProcessProtocol means you can't re-use any of your existing IProtocol implementations. Oops.

It seems like a better API would let you say "Here is a protocol, hook its output up to fd 0 in the process, hook fd 1 in the process up to its input. Here is another protocol, hook fd 2 in the process up to its input" (this would probably be a common configuration - "speak some regular protocol over stdin/stdout, have a little special logic (probably logging) for stderr).

On the other hand, it's possible to build this on top of ProcessProtocol and no one ever has...

I lied about ProcessProtocol API... I don't like the current API as it is ... but I like the idea of a single protocol. Maybe is not that hard to convert IProcessProtocol into IProtocol. create makeConnection, rename outRecevied to dataReceived, rename processEnded to connectionLost... rename errReceived to errorReceived I assume that most executables will implement a single protocol and adding another IProtocol to handle stderror seems complicated. I assume that the stderr protocol will want to share a lot of state/data with the stdout/stdin protocol. [snip]

...

-----

Does it make sense to put it on wiki or what should be the Twisted Enhancement Proposal process?

I think it's easier to maintain a document on the wiki than in threads on the mailing list.

...

Can I get 'create page' rights on Trac?

Hm. I thought everyone had wiki edit rights (except for a small number of privileged pages) by default - so I'm not sure what's going on here or how to give you permissions. Perhaps something changed in the trac upgrade or as part of the recent ticket permission reconfiguration. My bad... I tried trac/spawnProcess instead of trac/wiki/spawnProcess and I did not read the error message and assume that somehow I don't have WIKI_CREATE permissions.

Here it is: https://twistedmatrix.com/trac/wiki/spawnProcess-refactoring -- Adi Roiban

On Sep 5, 2014, at 5:13 AM, exarkun@twistedmatrix.com wrote:

It seems like a better API would let you say "Here is a protocol, hook its output up to fd 0 in the process, hook fd 1 in the process up to its input. Here is another protocol, hook fd 2 in the process up to its input" (this would probably be a common configuration - "speak some regular protocol over stdin/stdout, have a little special logic (probably logging) for stderr).

+1

On the other hand, it's possible to build this on top of ProcessProtocol and no one ever has...

Of course we have _sort of_ done it a gajillion times: https://twistedmatrix.com/documents/current/api/twisted.internet.endpoints._... -glyph