From 440e26cb0856fd5d7aec077134402263935d568f Mon Sep 17 00:00:00 2001 From: "Nathaniel J. Smith" Date: Thu, 24 Oct 2019 03:28:20 -0700 Subject: [PATCH] Move set_wakeup_fd into common code, used on all platforms Also switched signal_raise to always run the C-level signal handler in the current thread, which gives us more control over signal delivery and lets us test the new behavior. Fixes gh-109 --- newsfragments/109.bugfix.rst | 4 ++++ trio/_core/_io_windows.py | 8 -------- trio/_core/_run.py | 8 +++++--- trio/_core/_wakeup_socketpair.py | 33 ++++++++++++++++++++++++++++++-- trio/_core/tests/test_ki.py | 15 +++++---------- trio/_util.py | 3 ++- 6 files changed, 47 insertions(+), 24 deletions(-) create mode 100644 newsfragments/109.bugfix.rst diff --git a/newsfragments/109.bugfix.rst b/newsfragments/109.bugfix.rst new file mode 100644 index 0000000000..cff540558d --- /dev/null +++ b/newsfragments/109.bugfix.rst @@ -0,0 +1,4 @@ +Trio now uses `signal.set_wakeup_fd` on all platforms. This is mostly +an internal refactoring with no user-visible effect, but in theory it +should fix a few extremely-rare race conditions on Unix that could +have caused signal delivery to be delayed. diff --git a/trio/_core/_io_windows.py b/trio/_core/_io_windows.py index ef8726f322..e9fb0168b2 100644 --- a/trio/_core/_io_windows.py +++ b/trio/_core/_io_windows.py @@ -123,12 +123,6 @@ def __init__(self): wakeup_sock = self._main_thread_waker.wakeup_sock self._socket_waiters["read"][wakeup_sock] = None - # This is necessary to allow control-C to interrupt select(). - # https://github.com/python-trio/trio/issues/42 - if is_main_thread(): - fileno = self._main_thread_waker.write_sock.fileno() - self._old_signal_wakeup_fd = signal.set_wakeup_fd(fileno) - def statistics(self): return _WindowsStatistics( tasks_waiting_overlapped=len(self._overlapped_waiters), @@ -145,8 +139,6 @@ def close(self): if self._iocp_thread is not None: self._iocp_thread.join() self._main_thread_waker.close() - if is_main_thread(): - signal.set_wakeup_fd(self._old_signal_wakeup_fd) def __del__(self): # Need to make sure we clean up self._iocp (raw handle) and the IOCP diff --git a/trio/_core/_run.py b/trio/_core/_run.py index 509e243db9..905f477165 100644 --- a/trio/_core/_run.py +++ b/trio/_core/_run.py @@ -1783,9 +1783,11 @@ def run( ): try: with closing(runner): - # The main reason this is split off into its own function - # is just to get rid of this extra indentation. - run_impl(runner, async_fn, args) + with runner.entry_queue.wakeup.wakeup_on_signals(): + # The main reason this is split off into its own + # function is just to get rid of this extra + # indentation. + run_impl(runner, async_fn, args) except TrioInternalError: raise except BaseException as exc: diff --git a/trio/_core/_wakeup_socketpair.py b/trio/_core/_wakeup_socketpair.py index 80c3c6e15a..0c37928a55 100644 --- a/trio/_core/_wakeup_socketpair.py +++ b/trio/_core/_wakeup_socketpair.py @@ -1,6 +1,15 @@ import socket +import sys +from contextlib import contextmanager +import signal from .. import _core +from .._util import is_main_thread + +if sys.version_info >= (3, 7): + HAVE_WARN_ON_FULL_BUFFER = True +else: + HAVE_WARN_ON_FULL_BUFFER = False class WakeupSocketpair: @@ -16,8 +25,13 @@ def __init__(self): # Windows 10: 525347 # Windows you're weird. (And on Windows setting SNDBUF to 0 makes send # blocking, even on non-blocking sockets, so don't do that.) - self.wakeup_sock.setsockopt(socket.SOL_SOCKET, socket.SO_RCVBUF, 1) - self.write_sock.setsockopt(socket.SOL_SOCKET, socket.SO_SNDBUF, 1) + # + # But, if we're on an old Python and can't control the signal module's + # warn-on-full-buffer behavior, then we need to leave things alone, so + # the signal module won't spam the console with spurious warnings. + if HAVE_WARN_ON_FULL_BUFFER: + self.wakeup_sock.setsockopt(socket.SOL_SOCKET, socket.SO_RCVBUF, 1) + self.write_sock.setsockopt(socket.SOL_SOCKET, socket.SO_SNDBUF, 1) # On Windows this is a TCP socket so this might matter. On other # platforms this fails b/c AF_UNIX sockets aren't actually TCP. try: @@ -44,6 +58,21 @@ def drain(self): except BlockingIOError: pass + @contextmanager + def wakeup_on_signals(self): + if not is_main_thread(): + yield + return + fd = self.write_sock.fileno() + if HAVE_WARN_ON_FULL_BUFFER: + old_wakeup_fd = signal.set_wakeup_fd(fd, warn_on_full_buffer=False) + else: + old_wakeup_fd = signal.set_wakeup_fd(fd) + try: + yield + finally: + signal.set_wakeup_fd(old_wakeup_fd) + def close(self): self.wakeup_sock.close() self.write_sock.close() diff --git a/trio/_core/tests/test_ki.py b/trio/_core/tests/test_ki.py index 054cd9eac0..e0d3b97c50 100644 --- a/trio/_core/tests/test_ki.py +++ b/trio/_core/tests/test_ki.py @@ -480,10 +480,6 @@ async def inner(): # For details on why this test is non-trivial, see: # https://github.com/python-trio/trio/issues/42 # https://github.com/python-trio/trio/issues/109 -# To make it an even better test, we should try doing -# pthread_kill(pthread_self, SIGINT) -# in the child thread, to make sure signals in non-main threads also wake up -# the main loop... but currently that test would fail (see gh-109 again). @slow def test_ki_wakes_us_up(): assert is_main_thread() @@ -515,10 +511,9 @@ def test_ki_wakes_us_up(): # # PyPy was never affected. # - # The problem technically occurs on Unix as well, if a signal is delivered - # to a non-main thread, and if we were relying on the wakeup fd to wake - # us. Currently we don't use the wakeup fd on Unix anyway, though (see - # gh-109). + # The problem technically can occur on Unix as well, if a signal is + # delivered to a non-main thread, though we haven't observed this in + # practice. # # There's also this theoretical problem, but hopefully it won't actually # bite us in practice: @@ -526,8 +521,8 @@ def test_ki_wakes_us_up(): # https://bitbucket.org/pypy/pypy/issues/2623 import platform buggy_wakeup_fd = ( - os.name == "nt" and platform.python_implementation() == "CPython" - and sys.version_info < (3, 6, 2) + platform.python_implementation() == "CPython" and sys.version_info < + (3, 6, 2) ) # lock is only needed to avoid an annoying race condition where the diff --git a/trio/_util.py b/trio/_util.py index 414d3f1c05..bfef624dab 100644 --- a/trio/_util.py +++ b/trio/_util.py @@ -7,6 +7,7 @@ import pathlib from functools import wraps, update_wrapper import typing as t +import threading import async_generator @@ -64,7 +65,7 @@ else: def signal_raise(signum): - os.kill(os.getpid(), signum) + signal.pthread_kill(threading.get_ident(), signum) # Decorator to handle the change to __aiter__ in 3.5.2