From: Ben Darnell Date: Sat, 29 Sep 2012 20:08:10 +0000 (-0700) Subject: Add IOLoop.add_callback_from_signal, which avoids deadlocks X-Git-Tag: v3.0.0~268 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ae7cfe4034f8fc6213124b4afd3746b3fa81095e;p=thirdparty%2Ftornado.git 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 c9af31b61..2d4971854 100644 --- 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 42349c40a..ebd0d90bb 100644 --- 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 a679b07f8..8bfffe0ac 100644 --- 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()