Opened 16 years ago

Closed 15 years ago

#1832 defect closed fixed (fixed)

test_threads.ReactorThreadsTestCase is very fragile

Reported by: therve Owned by:
Priority: highest Milestone:
Component: core Keywords:
Cc: therve, Jean-Paul Calderone Branch:

Description (last modified by therve)

While debugging with threads, and looking at #1115, I got some problems with test_threads.ReactorThreadsTestCase: 2 test methods can't be run alone (testCallInThread and testWakerOverflow), because reactor doesn't seem to be initialized like it should be.

In fact, the tests run well by luck due to the order chosen by trial.

To reproduce:

  1. trial twisted.test.test_threads.ReactorThreadsTestCase.testCallInThread
  2. trial twisted.test.test_threads.ReactorThreadsTestCase.testWakerOverflow
  3. Rename testCallFromThread to testCallInThread and testCallInThread to testCallFromThread, and the test suite doesn't pass anymore (trial twisted.test.test_threads.ReactorThreadsTestCase)

I didn't find an easy way to correct this (if this isn't a known problem).

Attachments (1)

test_threads_1832.diff (894 bytes) - added by therve 16 years ago.

Download all attachments as: .zip

Change History (12)

Changed 16 years ago by therve

Attachment: test_threads_1832.diff added

comment:1 Changed 16 years ago by therve

OK I attach a patch correcting the problem. I identify the blocking part in reactor._initThreadPool, because the start of the threadpool is delayed until reactor starts, which seems to happen too late for the tests using a threading.Event.

So starting the pool in the tests resolves the problem, even though it may not be the cleanest way.

comment:2 Changed 15 years ago by therve

Owner: changed from Glyph to therve
Priority: lownormal

comment:3 Changed 15 years ago by therve

Description: modified (diff)

comment:4 Changed 15 years ago by therve

Cc: Jean-Paul Calderone added
Keywords: review added; test_threads removed
Owner: therve deleted
Priority: normalhighest
< exarkun> possibly the exact behavior trial should provide should be
defined, then implemented and documented, and based on that,
test_threads should be fixed

I considered that the good behavior was that threadpool is started only after the reactor. This may lack documentation, but anyway it's ready to review test-threads-1832.

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

Keywords: review removed
Owner: set to therve
if hasattr(reactor, 'threadpool') and reactor.threadpool is not None:

is probably better spelled

if getattr(reactor, 'threadpool', None) is not None:

both because it is shorter and because it avoids the potentially error-prone hasattr call.

The two tests which do deferToThread(time.sleep, 0) now should probably have a brief comment explaining the purpose of that, since I imagine it looks pointless to someone who isn't pretty familiar with how threading and reactor startup interact and how trial drives the reactor.

Added docstrings and the test_suggestThreadPoolSize improvement are nice. Go ahead and merge once the above stuff is taken care of.

comment:6 Changed 15 years ago by therve

Keywords: review added
Owner: changed from therve to Jean-Paul Calderone

There was a problem remaining with the tests, when doing

trial -u twisted.test.test_threads.ReactorThreadsTestCase.testCallInThread

I had to remove the restart of the threadpool in trial.util, so I don't merge it like this :).

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

Keywords: review removed
Owner: changed from Jean-Paul Calderone to therve

Okay, as long as I have another chance to complain, I found a couple other things I think we can improve :)

  • threadpoolShutdownID should be documented, at least with a comment. Maybe it should also be private.
  • The shutdown system event trigger it is associated with should also reset threadpoolShutdownID back to None
  • Since that will require a new method, trial might as well call that method to stop the threadpool and make sure the rest of the reactor's state is consistent, instead of poking the threadpool directly. This still isn't ideal, since the new method won't be part of any interface the reactor actually declares, but it's slightly better than poking attributes directly.

On IRC we talked about having do_cleanThreads fire a shutdown event instead of poking the threadpool directly. This seems like a good idea but in ten minutes of fiddling I haven't actually gotten it to work. If you want to work on the idea, feel free, otherwise it can probably be done in another branch.

(For posterity, this was the final version of my attempt:

Index: twisted/internet/
--- twisted/internet/    (revision 20035)
+++ twisted/internet/    (working copy)
@@ -603,8 +603,13 @@
             self.threadpool = threadpool.ThreadPool(0, 10, 'twisted.internet.reactor')
             self.threadpoolShutdownID = self.addSystemEventTrigger(
-                'during', 'shutdown', self.threadpool.stop)
+                'during', 'shutdown', self._deinitThreadPool)

+        def _deinitThreadPool(self):
+            self.threadpoolShutdownID = None
+            self.threadpool.stop()
+            self.threadpool = None
         def callInThread(self, _callable, *args, **kwargs):
             """See twisted.internet.interfaces.IReactorThreads.callInThread.
Index: twisted/trial/
--- twisted/trial/       (revision 20035)
+++ twisted/trial/       (working copy)
@@ -98,23 +98,10 @@

     def do_cleanThreads(cls):
         from twisted.internet import reactor
-        if interfaces.IReactorThreads.providedBy(reactor):
-            reactor.suggestThreadPoolSize(0)
-            if hasattr(reactor, 'threadpool') and reactor.threadpool is not None:
-                try:
-                    reactor.removeSystemEventTrigger(reactor.threadpoolShutdownID)
-                except KeyError:
-                    pass
-                reactor.threadpool.stop()
-                reactor.threadpool = None
-                # *Put it back* and *start it up again*.  The
-                # reactor's threadpool is *private*: we cannot just
-                # rape it and walk away.
-                reactor._initThreadPool()
+        reactor.fireSystemEvent('shutdown')
     do_cleanThreads = classmethod(do_cleanThreads)

     def do_cleanReactor(cls):
         s = []
         from twisted.internet import reactor


comment:8 Changed 15 years ago by therve

Keywords: review added
Owner: therve deleted

I've applied the first modifications. I've also tried the other fix, but it completely bombs trial, and I wanted to keep a little bit of sanity for today.

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

Keywords: review removed
Owner: set to therve

One typo in the _waitForThread docstring: L{threadsdeferToThread}​. The rest looks good.

comment:10 Changed 15 years ago by therve

Resolution: fixed
Status: newclosed

(In [20103]) Merge test-threads-1832

Author: therve Reviewer: exarkun Fixes #1832

Change the way trial manages the reactor's threadpool, letting the reactor starts in the tests. Fix the tests that relied on the precedent behavior.

comment:11 Changed 11 years ago by <automation>

Owner: therve deleted
Note: See TracTickets for help on using tickets.