Add IOLoop.add_callback_from_signal, which avoids deadlocks

which can occur when calling add_callback from a signal handler.

Also fix an issue in the recent set_wakeup_fd change when running
an IOLoop on a non-main thread.

diff --git a/tornado/ioloop.py b/tornado/ioloop.py
index c9af31b6169fc8f16aba8d45475713033c3d3545..2d4971854bcb0d90b2a32fbd5ed2c722ddc8084a 100644 (file)
--- a/tornado/ioloop.py
+++ b/tornado/ioloop.py
@@ -292,14 +292,14 @@ class IOLoop(object):
             # the python process on windows.
             try:
                 old_wakeup_fd = signal.set_wakeup_fd(self._waker.write_fileno())
+                if old_wakeup_fd != -1:
+                    # Already set, restore previous value.  This is a little racy,
+                    # but there's no clean get_wakeup_fd and in real use the
+                    # IOLoop is just started once at the beginning.
+                    signal.set_wakeup_fd(old_wakeup_fd)
+                    old_wakeup_fd = None
             except ValueError:  # non-main thread
                 pass
-            if old_wakeup_fd != -1:
-                # Already set, restore previous value.  This is a little racy,
-                # but there's no clean get_wakeup_fd and in real use the
-                # IOLoop is just started once at the beginning.
-                signal.set_wakeup_fd(old_wakeup_fd)
-                old_wakeup_fd = None
 
         while True:
             poll_timeout = 3600.0
@@ -442,11 +442,15 @@ class IOLoop(object):
     def add_callback(self, callback):
         """Calls the given callback on the next I/O loop iteration.
 
-        It is safe to call this method from any thread at any time.
-        Note that this is the *only* method in IOLoop that makes this
-        guarantee; all other interaction with the IOLoop must be done
-        from that IOLoop's thread.  add_callback() may be used to transfer
+        It is safe to call this method from any thread at any time,
+        except from a signal handler.  Note that this is the *only*
+        method in IOLoop that makes this thread-safety guarantee; all
+        other interaction with the IOLoop must be done from that
+        IOLoop's thread.  add_callback() may be used to transfer
         control from other threads to the IOLoop's thread.
+
+        To add a callback from a signal handler, see
+        `add_callback_from_signal`.
         """
         with self._callback_lock:
             list_empty = not self._callbacks
@@ -460,6 +464,32 @@ class IOLoop(object):
             # avoid it when we can.
             self._waker.wake()
 
+    def add_callback_from_signal(self, callback):
+        """Calls the given callback on the next I/O loop iteration.
+
+        Safe for use from a Python signal handler; should not be used
+        otherwise.
+
+        Callbacks added with this method will be run without any
+        stack_context, to avoid picking up the context of the function
+        that was interrupted by the signal.
+        """
+        with stack_context.NullContext():
+            if thread.get_ident() != self._thread_ident:
+                # if the signal is handled on another thread, we can add
+                # it normally (modulo the NullContext)
+                self.add_callback(callback)
+            else:
+                # If we're on the IOLoop's thread, we cannot use
+                # the regular add_callback because it may deadlock on
+                # _callback_lock.  Blindly insert into self._callbacks.
+                # This is safe because the GIL makes list.append atomic.
+                # One subtlety is that if the signal interrupted the
+                # _callback_lock block in IOLoop.start, we may modify
+                # either the old or new version of self._callbacks,
+                # but either way will work.
+                self._callbacks.append(stack_context.wrap(callback))
+
     def _run_callback(self, callback):
         try:
             callback()

diff --git a/tornado/process.py b/tornado/process.py
index 42349c40ad9f9bf41459057a05f63836154baf71..ebd0d90bb08489ab7b5383e573b469d82dde8fb5 100644 (file)
--- a/tornado/process.py
+++ b/tornado/process.py
@@ -267,7 +267,7 @@ class Subprocess(object):
             return
         assert ret_pid == pid
         subproc = cls._waiting.pop(pid)
-        subproc.io_loop.add_callback(
+        subproc.io_loop.add_callback_from_signal(
             functools.partial(subproc._set_returncode, status))
 
     def _set_returncode(self, status):

diff --git a/tornado/test/ioloop_test.py b/tornado/test/ioloop_test.py
index a679b07f88ff4ff97e458bc2acebdf4ee53e75ca..8bfffe0ace4c74ca18a291fdc37c20dcdf65b01a 100644 (file)
--- a/tornado/test/ioloop_test.py
+++ b/tornado/test/ioloop_test.py
@@ -3,6 +3,7 @@
 
 from __future__ import absolute_import, division, with_statement
 import datetime
+import threading
 import time
 
 from tornado.ioloop import IOLoop
@@ -28,6 +29,18 @@ class TestIOLoop(AsyncTestCase):
         self.assertAlmostEqual(time.time(), self.start_time, places=2)
         self.assertTrue(self.called)
 
+    def test_add_callback_wakeup_other_thread(self):
+        def target():
+            # sleep a bit to let the ioloop go into its poll loop
+            time.sleep(0.01)
+            self.stop_time = time.time()
+            self.io_loop.add_callback(self.stop)
+        thread = threading.Thread(target=target)
+        self.io_loop.add_callback(thread.start)
+        self.wait()
+        self.assertAlmostEqual(time.time(), self.stop_time, places=2)
+        thread.join()
+
     def test_add_timeout_timedelta(self):
         self.io_loop.add_timeout(datetime.timedelta(microseconds=1), self.stop)
         self.wait()
@@ -45,6 +58,23 @@ class TestIOLoop(AsyncTestCase):
         finally:
             sock.close()
 
+    def test_add_callback_from_signal(self):
+        # cheat a little bit and just run this normally, since we can't
+        # easily simulate the races that happen with real signal handlers
+        self.io_loop.add_callback_from_signal(self.stop)
+        self.wait()
+
+    def test_add_callback_from_signal_other_thread(self):
+        # Very crude test, just to make sure that we cover this case.
+        # This also happens to be the first test where we run an IOLoop in
+        # a non-main thread.
+        other_ioloop = IOLoop()
+        thread = threading.Thread(target=other_ioloop.start)
+        thread.start()
+        other_ioloop.add_callback_from_signal(other_ioloop.stop)
+        thread.join()
+        other_ioloop.close()
+
 
 if __name__ == "__main__":
     unittest.main()