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

Description

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

...

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

twisted.internet.test.test_sigchld.SetWakeupSIGCHLDTests.test_uninstallHandler

Change History (6)

comment:1 Changed 21 months ago by itamarst

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

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

comment:2 Changed 21 months 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).

http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/sigchld-tests-6161 started.

comment:3 Changed 21 months ago by itamar

  • Owner set to exarkun

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

comment:4 Changed 21 months 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 test_sigchld.py.
  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 21 months 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 21 months 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.