Skip to content

Commit

Permalink
pythongh-87079: Warn on unintended signal wakeup fd override in `asyn…
Browse files Browse the repository at this point in the history
…cio` (python#96807)

Warn on loop initialization, when setting the wakeup fd disturbs a previously set wakeup fd, and on loop closing, when upon resetting the wakeup fd, we find it has been changed by someone else.
  • Loading branch information
hidmic authored Sep 17, 2022
1 parent 2cd70ff commit 0587810
Show file tree
Hide file tree
Showing 5 changed files with 66 additions and 8 deletions.
14 changes: 12 additions & 2 deletions Lib/asyncio/proactor_events.py
Original file line number Diff line number Diff line change
Expand Up @@ -635,7 +635,12 @@ def __init__(self, proactor):
self._make_self_pipe()
if threading.current_thread() is threading.main_thread():
# wakeup fd can only be installed to a file descriptor from the main thread
signal.set_wakeup_fd(self._csock.fileno())
oldfd = signal.set_wakeup_fd(self._csock.fileno())
if oldfd != -1:
warnings.warn(
"Signal wakeup fd was already set",
ResourceWarning,
source=self)

def _make_socket_transport(self, sock, protocol, waiter=None,
extra=None, server=None):
Expand Down Expand Up @@ -684,7 +689,12 @@ def close(self):
return

if threading.current_thread() is threading.main_thread():
signal.set_wakeup_fd(-1)
oldfd = signal.set_wakeup_fd(-1)
if oldfd != self._csock.fileno():
warnings.warn(
"Got unexpected signal wakeup fd",
ResourceWarning,
source=self)
# Call these methods before closing the event loop (before calling
# BaseEventLoop.close), because they can schedule callbacks with
# call_soon(), which is forbidden when the event loop is closed.
Expand Down
19 changes: 16 additions & 3 deletions Lib/asyncio/unix_events.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,9 @@ def __init__(self, selector=None):
self._signal_handlers = {}

def close(self):
super().close()
# remove signal handlers first to verify
# the loop's signal handling setup has not
# been tampered with
if not sys.is_finalizing():
for sig in list(self._signal_handlers):
self.remove_signal_handler(sig)
Expand All @@ -77,6 +79,7 @@ def close(self):
ResourceWarning,
source=self)
self._signal_handlers.clear()
super().close()

def _process_self_data(self, data):
for signum in data:
Expand All @@ -102,7 +105,12 @@ def add_signal_handler(self, sig, callback, *args):
# main thread. By calling it early we ensure that an
# event loop running in another thread cannot add a signal
# handler.
signal.set_wakeup_fd(self._csock.fileno())
oldfd = signal.set_wakeup_fd(self._csock.fileno())
if oldfd != -1 and oldfd != self._csock.fileno():
warnings.warn(
"Signal wakeup fd was already set",
ResourceWarning,
source=self)
except (ValueError, OSError) as exc:
raise RuntimeError(str(exc))

Expand Down Expand Up @@ -166,7 +174,12 @@ def remove_signal_handler(self, sig):

if not self._signal_handlers:
try:
signal.set_wakeup_fd(-1)
oldfd = signal.set_wakeup_fd(-1)
if oldfd != -1 and oldfd != self._csock.fileno():
warnings.warn(
"Got unexpected signal wakeup fd",
ResourceWarning,
source=self)
except (ValueError, OSError) as exc:
logger.info('set_wakeup_fd(-1) failed: %s', exc)

Expand Down
15 changes: 12 additions & 3 deletions Lib/test/test_asyncio/test_proactor_events.py
Original file line number Diff line number Diff line change
Expand Up @@ -720,8 +720,12 @@ def setUp(self):
def test_ctor(self, socketpair):
ssock, csock = socketpair.return_value = (
mock.Mock(), mock.Mock())
with mock.patch('signal.set_wakeup_fd'):
loop = BaseProactorEventLoop(self.proactor)
with mock.patch('signal.set_wakeup_fd') as set_wakeup_fd:
set_wakeup_fd.return_value = -1000
with self.assertWarnsRegex(
ResourceWarning, 'Signal wakeup fd was already set'
):
loop = BaseProactorEventLoop(self.proactor)
self.assertIs(loop._ssock, ssock)
self.assertIs(loop._csock, csock)
self.assertEqual(loop._internal_fds, 1)
Expand All @@ -740,7 +744,12 @@ def test_close_self_pipe(self):

def test_close(self):
self.loop._close_self_pipe = mock.Mock()
self.loop.close()
with mock.patch('signal.set_wakeup_fd') as set_wakeup_fd:
set_wakeup_fd.return_value = -1000
with self.assertWarnsRegex(
ResourceWarning, 'Got unexpected signal wakeup fd'
):
self.loop.close()
self.assertTrue(self.loop._close_self_pipe.called)
self.assertTrue(self.proactor.close.called)
self.assertIsNone(self.loop._proactor)
Expand Down
24 changes: 24 additions & 0 deletions Lib/test/test_asyncio/test_unix_events.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,17 @@ def test_add_signal_handler_setup_error(self, m_signal):
self.loop.add_signal_handler,
signal.SIGINT, lambda: True)

@mock.patch('asyncio.unix_events.signal')
def test_add_signal_handler_setup_warn(self, m_signal):
m_signal.NSIG = signal.NSIG
m_signal.valid_signals = signal.valid_signals
m_signal.set_wakeup_fd.return_value = -1000

with self.assertWarnsRegex(
ResourceWarning, 'Signal wakeup fd was already set'
):
self.loop.add_signal_handler(signal.SIGINT, lambda: True)

@mock.patch('asyncio.unix_events.signal')
def test_add_signal_handler_coroutine_error(self, m_signal):
m_signal.NSIG = signal.NSIG
Expand Down Expand Up @@ -213,6 +224,19 @@ def test_remove_signal_handler_cleanup_error(self, m_logging, m_signal):
self.loop.remove_signal_handler(signal.SIGHUP)
self.assertTrue(m_logging.info)

@mock.patch('asyncio.unix_events.signal')
def test_remove_signal_handler_cleanup_warn(self, m_signal):
m_signal.NSIG = signal.NSIG
m_signal.valid_signals = signal.valid_signals
self.loop.add_signal_handler(signal.SIGHUP, lambda: True)

m_signal.set_wakeup_fd.return_value = -1000

with self.assertWarnsRegex(
ResourceWarning, 'Got unexpected signal wakeup fd'
):
self.loop.remove_signal_handler(signal.SIGHUP)

@mock.patch('asyncio.unix_events.signal')
def test_remove_signal_handler_error(self, m_signal):
m_signal.NSIG = signal.NSIG
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Warn the user whenever asyncio event loops override a signal wake up file
descriptor that was previously set.

0 comments on commit 0587810

Please sign in to comment.