Opened 6 years ago

Closed 6 years ago

Last modified 2 years ago

#3146 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
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

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 6 years ago by therve

  • Owner changed from glyph to exarkun

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

comment:2 Changed 6 years ago by exarkun

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/posixbase.py
===================================================================
--- twisted/internet/posixbase.py       (revision 23147)
+++ twisted/internet/posixbase.py       (working copy)
@@ -214,9 +214,9 @@
         """
         Forward call to ReactorBase, install signal handlers if asked.
         """
-        ReactorBase.startRunning(self)
         if installSignalHandlers:
             self._handleSignals()
+        ReactorBase.startRunning(self)
 
     def run(self, installSignalHandlers=True):
         self.startRunning(installSignalHandlers=installSignalHandlers)

comment:3 Changed 6 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 6 years ago by exarkun

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 6 years ago by exarkun

#3168 is another manifestation of this change.

comment:6 Changed 6 years ago by exarkun

  • author set to exarkun
  • Branch set to branches/system-event-ordering-3146

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

comment:7 Changed 6 years ago by exarkun

  • Branch changed from branches/system-event-ordering-3146 to branches/trivial-sysev-3146

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

comment:8 Changed 6 years ago by exarkun

  • Branch changed from branches/trivial-sysev-3146 to branches/trivial-sysev-3146-2

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

comment:9 Changed 6 years ago by exarkun

  • Keywords review added
  • Owner exarkun deleted
  • Priority changed from high to highest

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 6 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 6 years ago by exarkun

I filed #3178 for the first of these.

comment:12 Changed 6 years ago by glyph

  • Keywords review removed
  • Owner set to exarkun

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 6 years ago by exarkun

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

refs #3146

comment:14 Changed 6 years ago by exarkun

(In [23341]) slightly better docstring for SystemEventOrderRegressionTests

refs #3146
;

comment:15 Changed 6 years ago by exarkun

(In [23342]) Refer to another ticket

refs #3168
refs #3146
refs #3198

comment:16 Changed 6 years ago by exarkun

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

refs #3146

comment:17 Changed 6 years ago by exarkun

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

(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 3 years ago by <automation>

  • Owner exarkun deleted

comment:19 Changed 2 years ago by oubiwann

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