From bec6ccd9462b6382adfb60ec7d24d8171702c4bf Mon Sep 17 00:00:00 2001 From: Michel Hidalgo Date: Wed, 14 Sep 2022 19:09:26 -0300 Subject: [PATCH 1/4] Warn on unintended signal wakeup fd override in asyncio --- Lib/asyncio/proactor_events.py | 16 +++++++++++-- Lib/asyncio/unix_events.py | 18 +++++++++++--- Lib/test/test_asyncio/test_proactor_events.py | 12 +++++++--- Lib/test/test_asyncio/test_unix_events.py | 24 +++++++++++++++++++ 4 files changed, 62 insertions(+), 8 deletions(-) diff --git a/Lib/asyncio/proactor_events.py b/Lib/asyncio/proactor_events.py index ddb9daca0269367..fc6f664d318f9c1 100644 --- a/Lib/asyncio/proactor_events.py +++ b/Lib/asyncio/proactor_events.py @@ -635,7 +635,13 @@ 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( + 'Overriding signal wakeup file ' + 'descriptor for loop signal handlers', + ResourceWarning, + source=self) def _make_socket_transport(self, sock, protocol, waiter=None, extra=None, server=None): @@ -684,7 +690,13 @@ 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 file ' + 'descriptor while removing it', + 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. diff --git a/Lib/asyncio/unix_events.py b/Lib/asyncio/unix_events.py index cf7683fee646211..d9270c1faf07c10 100644 --- a/Lib/asyncio/unix_events.py +++ b/Lib/asyncio/unix_events.py @@ -65,7 +65,6 @@ def __init__(self, selector=None): self._signal_handlers = {} def close(self): - super().close() if not sys.is_finalizing(): for sig in list(self._signal_handlers): self.remove_signal_handler(sig) @@ -77,6 +76,7 @@ def close(self): ResourceWarning, source=self) self._signal_handlers.clear() + super().close() def _process_self_data(self, data): for signum in data: @@ -102,7 +102,13 @@ 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( + 'Overriding signal wakeup file ' + 'descriptor for loop signal handlers', + ResourceWarning, + source=self) except (ValueError, OSError) as exc: raise RuntimeError(str(exc)) @@ -166,7 +172,13 @@ 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 file ' + 'descriptor while removing it', + ResourceWarning, + source=self) except (ValueError, OSError) as exc: logger.info('set_wakeup_fd(-1) failed: %s', exc) diff --git a/Lib/test/test_asyncio/test_proactor_events.py b/Lib/test/test_asyncio/test_proactor_events.py index 7fca0541ee75a27..50ea41b73834c21 100644 --- a/Lib/test/test_asyncio/test_proactor_events.py +++ b/Lib/test/test_asyncio/test_proactor_events.py @@ -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, 'Overriding signal wakeup' + ): + loop = BaseProactorEventLoop(self.proactor) self.assertIs(loop._ssock, ssock) self.assertIs(loop._csock, csock) self.assertEqual(loop._internal_fds, 1) @@ -740,7 +744,9 @@ 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 + self.loop.close() self.assertTrue(self.loop._close_self_pipe.called) self.assertTrue(self.proactor.close.called) self.assertIsNone(self.loop._proactor) diff --git a/Lib/test/test_asyncio/test_unix_events.py b/Lib/test/test_asyncio/test_unix_events.py index 5bad21ecbae4afa..f0a2561273e38a0 100644 --- a/Lib/test/test_asyncio/test_unix_events.py +++ b/Lib/test/test_asyncio/test_unix_events.py @@ -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, 'Overriding signal wakeup' + ): + 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 @@ -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, 'unexpected signal wakeup' + ): + 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 From 7badd94454476104207042ec0eb6278381ab0d06 Mon Sep 17 00:00:00 2001 From: Michel Hidalgo Date: Wed, 14 Sep 2022 19:15:23 -0300 Subject: [PATCH 2/4] Update NEWS --- .../next/Library/2022-09-14-19-15-01.gh-issue-87079.0zYmW5.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2022-09-14-19-15-01.gh-issue-87079.0zYmW5.rst diff --git a/Misc/NEWS.d/next/Library/2022-09-14-19-15-01.gh-issue-87079.0zYmW5.rst b/Misc/NEWS.d/next/Library/2022-09-14-19-15-01.gh-issue-87079.0zYmW5.rst new file mode 100644 index 000000000000000..989c6dda9158a9f --- /dev/null +++ b/Misc/NEWS.d/next/Library/2022-09-14-19-15-01.gh-issue-87079.0zYmW5.rst @@ -0,0 +1,2 @@ +Warn the user whenever asyncio event loops override a signal wake up file +descriptor that was previously set. From bc7b849d3b29b643f0352441142e95fb0a72bcbe Mon Sep 17 00:00:00 2001 From: Michel Hidalgo Date: Fri, 16 Sep 2022 19:52:09 -0300 Subject: [PATCH 3/4] Address peer review comments --- Lib/asyncio/proactor_events.py | 8 +++----- Lib/asyncio/unix_events.py | 6 ++---- Lib/test/test_asyncio/test_proactor_events.py | 7 +++++-- Lib/test/test_asyncio/test_unix_events.py | 4 ++-- 4 files changed, 12 insertions(+), 13 deletions(-) diff --git a/Lib/asyncio/proactor_events.py b/Lib/asyncio/proactor_events.py index fc6f664d318f9c1..4808c5dfc458563 100644 --- a/Lib/asyncio/proactor_events.py +++ b/Lib/asyncio/proactor_events.py @@ -638,8 +638,7 @@ def __init__(self, proactor): oldfd = signal.set_wakeup_fd(self._csock.fileno()) if oldfd != -1: warnings.warn( - 'Overriding signal wakeup file ' - 'descriptor for loop signal handlers', + "Signal wakeup fd was already set", ResourceWarning, source=self) @@ -691,10 +690,9 @@ def close(self): if threading.current_thread() is threading.main_thread(): oldfd = signal.set_wakeup_fd(-1) - if oldfd == self._csock.fileno(): + if oldfd != self._csock.fileno(): warnings.warn( - 'Got unexpected signal wakeup file ' - 'descriptor while removing it', + "Got unexpected signal wakeup fd", ResourceWarning, source=self) # Call these methods before closing the event loop (before calling diff --git a/Lib/asyncio/unix_events.py b/Lib/asyncio/unix_events.py index d9270c1faf07c10..5f864b033eb902d 100644 --- a/Lib/asyncio/unix_events.py +++ b/Lib/asyncio/unix_events.py @@ -105,8 +105,7 @@ def add_signal_handler(self, sig, callback, *args): oldfd = signal.set_wakeup_fd(self._csock.fileno()) if oldfd != -1 and oldfd != self._csock.fileno(): warnings.warn( - 'Overriding signal wakeup file ' - 'descriptor for loop signal handlers', + "Signal wakeup fd was already set", ResourceWarning, source=self) except (ValueError, OSError) as exc: @@ -175,8 +174,7 @@ def remove_signal_handler(self, sig): oldfd = signal.set_wakeup_fd(-1) if oldfd != -1 and oldfd != self._csock.fileno(): warnings.warn( - 'Got unexpected signal wakeup file ' - 'descriptor while removing it', + "Got unexpected signal wakeup fd", ResourceWarning, source=self) except (ValueError, OSError) as exc: diff --git a/Lib/test/test_asyncio/test_proactor_events.py b/Lib/test/test_asyncio/test_proactor_events.py index 50ea41b73834c21..6b28348a71e26f3 100644 --- a/Lib/test/test_asyncio/test_proactor_events.py +++ b/Lib/test/test_asyncio/test_proactor_events.py @@ -723,7 +723,7 @@ def test_ctor(self, socketpair): with mock.patch('signal.set_wakeup_fd') as set_wakeup_fd: set_wakeup_fd.return_value = -1000 with self.assertWarnsRegex( - ResourceWarning, 'Overriding signal wakeup' + ResourceWarning, 'Signal wakeup fd was already set' ): loop = BaseProactorEventLoop(self.proactor) self.assertIs(loop._ssock, ssock) @@ -746,7 +746,10 @@ def test_close(self): self.loop._close_self_pipe = mock.Mock() with mock.patch('signal.set_wakeup_fd') as set_wakeup_fd: set_wakeup_fd.return_value = -1000 - self.loop.close() + 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) diff --git a/Lib/test/test_asyncio/test_unix_events.py b/Lib/test/test_asyncio/test_unix_events.py index f0a2561273e38a0..1ec627f63eeef04 100644 --- a/Lib/test/test_asyncio/test_unix_events.py +++ b/Lib/test/test_asyncio/test_unix_events.py @@ -95,7 +95,7 @@ def test_add_signal_handler_setup_warn(self, m_signal): m_signal.set_wakeup_fd.return_value = -1000 with self.assertWarnsRegex( - ResourceWarning, 'Overriding signal wakeup' + ResourceWarning, 'Signal wakeup fd was already set' ): self.loop.add_signal_handler(signal.SIGINT, lambda: True) @@ -233,7 +233,7 @@ def test_remove_signal_handler_cleanup_warn(self, m_signal): m_signal.set_wakeup_fd.return_value = -1000 with self.assertWarnsRegex( - ResourceWarning, 'unexpected signal wakeup' + ResourceWarning, 'Got unexpected signal wakeup fd' ): self.loop.remove_signal_handler(signal.SIGHUP) From 3b4975a4241964fa28f5d7398604b36ba3b85d3d Mon Sep 17 00:00:00 2001 From: Michel Hidalgo Date: Fri, 16 Sep 2022 20:11:34 -0300 Subject: [PATCH 4/4] Comment on _UnixSelectorEventLoop.close() logic --- Lib/asyncio/unix_events.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Lib/asyncio/unix_events.py b/Lib/asyncio/unix_events.py index 5f864b033eb902d..6c9a89dbc5d09f0 100644 --- a/Lib/asyncio/unix_events.py +++ b/Lib/asyncio/unix_events.py @@ -65,6 +65,9 @@ def __init__(self, selector=None): self._signal_handlers = {} def close(self): + # 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)