[Twisted-Python] Race condition in callInThread()

Jp Calderone exarkun at divmod.com
Fri Jul 8 21:29:41 EDT 2005

James kindly added a failing test for the nasty behavior of twisted.python.threadpool.ThreadPool._startSomeWorkers().  It seems this will be difficult to fix completely without making API-incompatible changes all the way up to reactor.callInThread, though.

Fixing it for *most* cases is straightforward enough:

Index: twisted/python/threadpool.py
--- twisted/python/threadpool.py        (revision 14112)
+++ twisted/python/threadpool.py        (working copy)
@@ -99,10 +99,10 @@
         return state
     def _startSomeWorkers(self):
-        if self.workers >= self.max:
-            return
-        # FIXME: Wait for any waiters to eat of the queue.
-        while self.workers < self.max and self.q.qsize() > 0:
+        while (
+            self.workers < self.max and # Don't create too many
+            len(self.waiters) < self.q.qsize() # but create enough
+            ):
     def dispatch(self, owner, func, *args, **kw):

But this isn't a complete fix.  Annoyingly, the area it doesn't cover very well is exactly what the unit test covers most aggressively.  The problem is the implementation of _worker: it calls back into user code before adjusting the ThreadPool's state to indicate that it is no longer busy.  Since the concept of result callback/errbacks isn't present in _worker, fixing this would involve expanding the API to pass callback/errback functions all the way down so that _worker can invoke them at the right time.

At least, as far as I can tell.

Another option is to loosen up the test, so that it passes even if the ThreadPool creates one extra thread.  Unfortunately, even though the unit test will only ever create one extra thread, user code could create as many as the ThreadPool allows to exist total, by just adding multiple work units to the queue in a single callback on the Deferred returned by deferToThread.



More information about the Twisted-Python mailing list