Opened 8 years ago

Closed 8 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, exarkun Branch:
Author: Launchpad Bug:

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 8 years ago.

Download all attachments as: .zip

Change History (12)

Changed 8 years ago by therve

comment:1 Changed 8 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 8 years ago by therve

  • Owner changed from glyph to therve
  • Priority changed from low to normal

comment:3 Changed 8 years ago by therve

  • Description modified (diff)

comment:4 Changed 8 years ago by therve

  • Cc exarkun added
  • Keywords review added; test_threads removed
  • Owner therve deleted
  • Priority changed from normal to highest
< 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 8 years ago by exarkun

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

  • Keywords review added
  • Owner changed from therve to exarkun

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

  • Keywords review removed
  • Owner changed from exarkun 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/base.py
===================================================================
--- twisted/internet/base.py    (revision 20035)
+++ twisted/internet/base.py    (working copy)
@@ -603,8 +603,13 @@
             self.threadpool = threadpool.ThreadPool(0, 10, 'twisted.internet.reactor')
             self.callWhenRunning(self.threadpool.start)
             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/util.py
===================================================================
--- twisted/trial/util.py       (revision 20035)
+++ twisted/trial/util.py       (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 8 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 8 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

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

comment:10 Changed 8 years ago by therve

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

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

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