Merge pull request #196 from jml/robust-spinner-timeout

Handle edge case in reactor running

3 files changed, 70 insertions, 11 deletions

diff --git a/NEWS b/NEWS
index 5fd51cf..e96c923 100644
--- a/NEWS
+++ b/NEWS
@@ -38,6 +38,11 @@ Improvements
   between runs. In particular, testtools tests can now be run with
   ``trial -u``.  (Jonathan Lange, #1517879)
 
+* Fixed bug where if an asynchronous ``Deferred`` test times out but the
+  ``Deferred`` then fires, the entire test run would abort with
+  ``KeyboardInterrupt``, failing the currently running test.
+  (Jonathan Lange, James Westby)
+
 
 Changes
 -------
diff --git a/testtools/_spinner.py b/testtools/_spinner.py
index 9c58055..fa1b18b 100644
--- a/testtools/_spinner.py
+++ b/testtools/_spinner.py
@@ -1,4 +1,4 @@
-# Copyright (c) 2010 testtools developers. See LICENSE for details.
+# Copyright (c) testtools developers. See LICENSE for details.
 
 """Evil reactor-spinning logic for running Twisted tests.
 
@@ -153,6 +153,7 @@ class Spinner(object):
         self._saved_signals = []
         self._junk = []
         self._debug = debug
+        self._spinning = False
 
     def _cancel_timeout(self):
         if self._timeout_call:
@@ -173,9 +174,23 @@ class Spinner(object):
         self._cancel_timeout()
         self._success = result
 
+    def _fake_stop(self):
+        """Use to replace ``reactor.stop`` while running a test.
+
+        Calling ``reactor.stop`` makes it impossible re-start the reactor.
+        Since the default signal handlers for TERM, BREAK and INT all call
+        ``reactor.stop()``, we patch it over with ``reactor.crash()``
+
+        Spinner never calls this method.
+        """
+        self._reactor.crash()
+
     def _stop_reactor(self, ignored=None):
         """Stop the reactor!"""
-        self._reactor.crash()
+        # XXX: Would like to emit a warning when called when *not* spinning.
+        if self._spinning:
+            self._reactor.crash()
+            self._spinning = False
 
     def _timed_out(self, function, timeout):
         e = TimeoutError(function, timeout)
@@ -268,16 +283,18 @@ class Spinner(object):
             # with crash.  XXX: It might be a better idea to either install
             # custom signal handlers or to override the methods that are
             # Twisted's signal handlers.
-            stop, self._reactor.stop = self._reactor.stop, self._reactor.crash
+            real_stop, self._reactor.stop = self._reactor.stop, self._fake_stop
+
             def run_function():
                 d = defer.maybeDeferred(function, *args, **kwargs)
                 d.addCallbacks(self._got_success, self._got_failure)
                 d.addBoth(self._stop_reactor)
             try:
                 self._reactor.callWhenRunning(run_function)
+                self._spinning = True
                 self._reactor.run()
             finally:
-                self._reactor.stop = stop
+                self._reactor.stop = real_stop
                 self._restore_signals()
             try:
                 return self._get_result()
diff --git a/testtools/tests/test_spinner.py b/testtools/tests/test_spinner.py
index 5052031..a916380 100644
--- a/testtools/tests/test_spinner.py
+++ b/testtools/tests/test_spinner.py
@@ -1,4 +1,4 @@
-# Copyright (c) 2010 testtools developers. See LICENSE for details.
+# Copyright (c) testtools developers. See LICENSE for details.
 
 """Tests for the evil Twisted reactor-spinning we do."""
 
@@ -118,7 +118,7 @@ class TestRunInReactor(NeedsTwistedTestCase):
         # If the given function raises an error, run_in_reactor re-raises that
         # error.
         self.assertThat(
-            lambda:self.make_spinner().run(self.make_timeout(), lambda: 1/0),
+            lambda: self.make_spinner().run(self.make_timeout(), lambda: 1/0),
             Raises(MatchesException(ZeroDivisionError)))
 
     def test_keyword_arguments(self):
@@ -162,7 +162,7 @@ class TestRunInReactor(NeedsTwistedTestCase):
         # _spinner.TimeoutError.
         timeout = self.make_timeout()
         self.assertThat(
-            lambda:self.make_spinner().run(timeout, lambda: defer.Deferred()),
+            lambda: self.make_spinner().run(timeout, lambda: defer.Deferred()),
             Raises(MatchesException(_spinner.TimeoutError)))
 
     def test_no_junk_by_default(self):
@@ -228,7 +228,8 @@ class TestRunInReactor(NeedsTwistedTestCase):
         reactor = self.make_reactor()
         spinner = self.make_spinner(reactor)
         port = spinner.run(
-            self.make_timeout(), reactor.listenTCP, 0, ServerFactory(), interface='127.0.0.1')
+            self.make_timeout(), reactor.listenTCP, 0, ServerFactory(),
+            interface='127.0.0.1')
         self.assertThat(spinner.get_junk(), Equals([port]))
 
     def test_will_not_run_with_previous_junk(self):
@@ -249,7 +250,8 @@ class TestRunInReactor(NeedsTwistedTestCase):
         reactor = self.make_reactor()
         spinner = self.make_spinner(reactor)
         timeout = self.make_timeout()
-        port = spinner.run(timeout, reactor.listenTCP, 0, ServerFactory(), interface='127.0.0.1')
+        port = spinner.run(timeout, reactor.listenTCP, 0, ServerFactory(),
+                           interface='127.0.0.1')
         junk = spinner.clear_junk()
         self.assertThat(junk, Equals([port]))
         self.assertThat(spinner.get_junk(), Equals([]))
@@ -264,7 +266,8 @@ class TestRunInReactor(NeedsTwistedTestCase):
         spinner = self.make_spinner(reactor)
         timeout = self.make_timeout()
         reactor.callLater(timeout, os.kill, os.getpid(), SIGINT)
-        self.assertThat(lambda:spinner.run(timeout * 5, defer.Deferred),
+        self.assertThat(
+            lambda: spinner.run(timeout * 5, defer.Deferred),
             Raises(MatchesException(_spinner.NoResultError)))
         self.assertEqual([], spinner._clean())
 
@@ -285,7 +288,8 @@ class TestRunInReactor(NeedsTwistedTestCase):
         spinner = self.make_spinner(reactor)
         timeout = self.make_timeout()
         reactor.callWhenRunning(os.kill, os.getpid(), SIGINT)
-        self.assertThat(lambda:spinner.run(timeout * 5, defer.Deferred),
+        self.assertThat(
+            lambda: spinner.run(timeout * 5, defer.Deferred),
             Raises(MatchesException(_spinner.NoResultError)))
         self.assertEqual([], spinner._clean())
 
@@ -293,6 +297,39 @@ class TestRunInReactor(NeedsTwistedTestCase):
     def test_fast_sigint_raises_no_result_error_second_time(self):
         self.test_fast_sigint_raises_no_result_error()
 
+    def test_fires_after_timeout(self):
+        # If we timeout, but the Deferred actually ends up firing after the
+        # time out (perhaps because Spinner's clean-up code is buggy, or
+        # perhaps because the code responsible for the callback is in a
+        # thread), then the next run of a spinner works as intended,
+        # completely isolated from the previous run.
+
+        # Ensure we've timed out, and that we have a handle on the Deferred
+        # that didn't fire.
+        reactor = self.make_reactor()
+        spinner1 = self.make_spinner(reactor)
+        timeout = self.make_timeout()
+        deferred1 = defer.Deferred()
+        self.expectThat(
+            lambda: spinner1.run(timeout, lambda: deferred1),
+            Raises(MatchesException(_spinner.TimeoutError)))
+
+        # Make a Deferred that will fire *after* deferred1 as long as the
+        # reactor keeps spinning. We don't care that it's a callback of
+        # deferred1 per se, only that it strictly fires afterwards.
+        marker = object()
+        deferred2 = defer.Deferred()
+        deferred1.addCallback(
+            lambda ignored: reactor.callLater(0, deferred2.callback, marker))
+
+        def fire_other():
+            """Fire Deferred from the last spin while waiting for this one."""
+            deferred1.callback(object())
+            return deferred2
+
+        spinner2 = self.make_spinner(reactor)
+        self.assertThat(spinner2.run(timeout, fire_other), Is(marker))
+
 
 def test_suite():
     from unittest import TestLoader