Opened 4 years ago

Closed 4 years ago

#6161 defect closed fixed (fixed)

twisted.internet.test.test_sigchld way too aggressive in assuming no previous fd or SIGCHLD handler was installed

Reported by: itamar Owned by: itamar
Priority: normal Milestone: Python 3.3 Minimal
Component: core Keywords:
Cc: Branch: branches/sigchld-tests-6161
branch-diff, diff-cov, branch-cov, buildbot
Author: itamarst


twisted.internet.test.test_sigchld.SetWakeupSIGCHLDTests.setUp assumes no one has previously set a SIGCHLD handler or registered a wakeup fd. If someone has, that is considered an error. However, any random test that uses the reactor will cause this registration; AFAICT this never caused problems before due to a specific sort order of running tests.

On Ubuntu 12.10:

$ trial -z 1 twisted.test.test_udp twisted.internet.test.test_sigchld


Traceback (most recent call last):
  File "/home/itamarst/Devel/Twisted/trunk/twisted/internet/test/", line 58, in setUp
    raise RuntimeError("You used some signal APIs wrong!  Try again.")
exceptions.RuntimeError: You used some signal APIs wrong!  Try again.


Change History (6)

comment:1 Changed 4 years ago by itamarst

  • Author set to itamarst
  • Branch set to branches/sigchld-tests-6161

(In [36265]) Branching to 'sigchld-tests-6161'

comment:2 Changed 4 years ago by itamar

  • Keywords review added
  • Owner itamar deleted

I fixed this by just not raising a RuntimeError if a previous handler was installed; tearDown already has code to more or less restore the previous signal handler. I also changed the signal handler registered for SIGCHLD to have a name, so that the log message in this situation is more meaningful (the name of the function is logged). started.

comment:3 Changed 4 years ago by itamar

  • Owner set to exarkun

This is, sadly, a dependency for the gireactor port (#6055).

comment:4 Changed 4 years ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to itamar

AFAICT this never caused problems before due to a specific sort order of running tests.

Actually it has shown up on buildbot a few times - we have random test order builders. So it'll be nice to have it fixed. :)

  1. Maybe we don't need the warning either? It does seem perfectly true that a prior test may have legitimately installed some kind of signal handler. I don't remember why this check was added in the first place - perhaps only as a way to track down bugs in
  2. I'll bike shed on the name of doNothing a little bit. Perhaps noopSignalHandler?

That's all, please address these to your satisfaction and merge.

comment:5 Changed 4 years ago by itamar

  1. It seems kinda untidy to leave global state changes around as a result of tests. If these tests can restore it to where it was when they started (module siginterrupt), why not other tests? So keeping this as reminder seems OK to me.
  2. Done.

comment:6 Changed 4 years ago by itamarst

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

(In [36329]) Merge sigchld-tests-6161: Don't fail test_sigchld if SIGCHLD was previously installed.

Author: itamar Review: exarkun Fixes: #6161

The twisted.internet.test.test_sigchld tests were previously somewhat fragile, erroneously raising RuntimeError if a previous test had left a SIGCHLD handler lying around.

Note: See TracTickets for help on using tickets.