test_threads.ReactorThreadsTestCase is very fragile

[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).

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

[therve]

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

[exarkun]

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.

[therve]

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 :).

[exarkun]

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

)

[therve]

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.

[exarkun]

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

[therve]

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