Opened 10 years ago

Closed 10 years ago

Last modified 6 years ago

#3146 release blocker: regression closed fixed (fixed)

callWhenRunning is calling the function before the signal handlers are installed

Reported by: therve Owned by:
Priority: highest Milestone:
Component: core Keywords:
Cc: Branch: branches/trivial-sysev-3146-2
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun

Description (last modified by oubiwann)

When we spawn a process in a callWhenRunning method, we can see the warning: "spawnProcess called, but the SIGCHLD handler is not installed. ...". It seems to work well, but the message is wrong.

The root of this problem is that we used to do a callLater between startup events, and don't anymore. Maybe we can just move the initialization of signals handler sooner though.

Change History (19)

comment:1 Changed 10 years ago by therve

Owner: changed from Glyph to Jean-Paul Calderone

What do you think? Should we add the callLater back?

comment:2 Changed 10 years ago by Jean-Paul Calderone

This is a dumb question (because I should write a unit test and use that to know the answer, but anyway), would this be a reasonable fix?

Index: twisted/internet/
--- twisted/internet/       (revision 23147)
+++ twisted/internet/       (working copy)
@@ -214,9 +214,9 @@
         Forward call to ReactorBase, install signal handlers if asked.
-        ReactorBase.startRunning(self)
         if installSignalHandlers:
+        ReactorBase.startRunning(self)
     def run(self, installSignalHandlers=True):

comment:3 Changed 10 years ago by therve

Well, that fixes the reported problem, sure. But the fact is that the 'after startup' event is no longer ran once the reactor has started. I'm not sure if it can't have other side effects.

Also, we have to check for every reactors.

comment:4 Changed 10 years ago by Jean-Paul Calderone

Ah, that's a good point. I think we should clarify what "after startup" is actually supposed to mean. This is both a documentation and a testing issue, I guess. If clarification shows that we should put the callLater back, then I won't be too opposed to it (but I'll be happier if it turns out to not be necessary).

comment:5 Changed 10 years ago by Jean-Paul Calderone

#3168 is another manifestation of this change.

comment:6 Changed 10 years ago by Jean-Paul Calderone

author: exarkun
Branch: branches/system-event-ordering-3146

(In [23234]) Branching to 'system-event-ordering-3146'

comment:7 Changed 10 years ago by Jean-Paul Calderone

Branch: branches/system-event-ordering-3146branches/trivial-sysev-3146

(In [23280]) Branching to 'trivial-sysev-3146'

comment:8 Changed 10 years ago by Jean-Paul Calderone

Branch: branches/trivial-sysev-3146branches/trivial-sysev-3146-2

(In [23295]) Branching to 'trivial-sysev-3146-2'

comment:9 Changed 10 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted
Priority: highhighest

This issue should be fixed in the branch. system-event-ordering-3146 got bigger than I wanted, so I pulled out the unit tests that were directly applicable to this ticket and to #3168, dropped all the fancy test framework stuff, and then made the simplest change I could to the implementation to make them pass.

So, the trivial-sysev-3146-2 should resolve this ticket and #3168.

I'll make a new ticket later for the test improvements in the old branch.

comment:10 Changed 10 years ago by therve

Not a full review, but some comments:

  • trial starts printing a warning when running process. It was already there but in the log file. I suspect this could be disturbing. Maybe we could explicitely call _handleSignals? I don't have a better (or worse) suggestion right now.
  • I don't like the addition of a new flag :/

Otherwise, it's very well commented and documented, and the buildbot is green. Maybe I ask to much :).

comment:11 Changed 10 years ago by Jean-Paul Calderone

I filed #3178 for the first of these.

comment:12 Changed 10 years ago by Glyph

Keywords: review removed
Owner: set to Jean-Paul Calderone

OK, a full review:

  • The test case should be more selective about the processes it tears down; it should just wait for its own processes to exit, not shotgun through all reaping. I wish I could say something about the signal handling being more selective too, but I think it's doing as well as it can given the limitations of the platform.
  • The test case should have a more descriptive docstring; I don't think there was any test coverage for this functionality independent of the regressions, so it should just describe the properties it is verifying. Normally I say "this is a regression test" if I am verifying something I wouldn't normally, such as the absence of some behavior, but these tests look pretty decent otherwise.
  • Make a new ticket for the contents of the system-event-ordering branch, and refer to that from the comment about .clock.advance(0), so that users know what a better approach might be.

Modulo these minor changes, I believe this is ready to merge.

comment:13 Changed 10 years ago by Jean-Paul Calderone

(In [23340]) use os.waitpid instead of reapAllProcesses

refs #3146

comment:14 Changed 10 years ago by Jean-Paul Calderone

(In [23341]) slightly better docstring for SystemEventOrderRegressionTests

refs #3146 ;

comment:15 Changed 10 years ago by Jean-Paul Calderone

(In [23342]) Refer to another ticket

refs #3168 refs #3146 refs #3198

comment:16 Changed 10 years ago by Jean-Paul Calderone

(In [23344]) put back SIG_DFL before calling waitpid

refs #3146

comment:17 Changed 10 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [23345]) Merge trivial-sysev-3146-2

Author: exarkun, radix Reviewer: glyph Fixes: #3146

Change reactor startup so that calls scheduled with callWhenRunning are only processed after the reactor is properly started up.

Change reactor shutdown so that calling reactor.stop inside an event handler (such as clientConnectionFailed) is safe again.

Change the warning emitted by the process support code to use the warnings module instead of normal Twisted logging.

comment:18 Changed 7 years ago by <automation>

Owner: Jean-Paul Calderone deleted

comment:19 Changed 6 years ago by oubiwann

Description: modified (diff)
Note: See TracTickets for help on using tickets.