Skip to content

Commit

Permalink
Move set_wakeup_fd into common code, used on all platforms
Browse files Browse the repository at this point in the history
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 python-triogh-109
  • Loading branch information
njsmith committed Oct 24, 2019
1 parent 05ae049 commit 440e26c
Show file tree
Hide file tree
Showing 6 changed files with 47 additions and 24 deletions.
4 changes: 4 additions & 0 deletions newsfragments/109.bugfix.rst
Original file line number Diff line number Diff line change
@@ -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.
8 changes: 0 additions & 8 deletions trio/_core/_io_windows.py
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -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
Expand Down
8 changes: 5 additions & 3 deletions trio/_core/_run.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
33 changes: 31 additions & 2 deletions trio/_core/_wakeup_socketpair.py
Original file line number Diff line number Diff line change
@@ -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:
Expand All @@ -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:
Expand All @@ -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()
15 changes: 5 additions & 10 deletions trio/_core/tests/test_ki.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -515,19 +511,18 @@ 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:
# https://bugs.python.org/issue31119
# 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
Expand Down
3 changes: 2 additions & 1 deletion trio/_util.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import pathlib
from functools import wraps, update_wrapper
import typing as t
import threading

import async_generator

Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 440e26c

Please sign in to comment.