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 Turner-Trauring Owned by: Itamar Turner-Trauring
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

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 4 years ago by itamarst

Author: itamarst
Branch: branches/sigchld-tests-6161

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

comment:2 Changed 4 years ago by Itamar Turner-Trauring

Keywords: review added
Owner: Itamar Turner-Trauring 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 4 years ago by Itamar Turner-Trauring

Owner: set to Jean-Paul Calderone

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

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

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Itamar Turner-Trauring

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 4 years ago by Itamar Turner-Trauring

  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: fixed
Status: newclosed

(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.