From 7bef9b4e79dac28d3e37d42f4d95a625220fad31 Mon Sep 17 00:00:00 2001 From: poolitzer Date: Mon, 23 May 2022 22:49:27 +0200 Subject: [PATCH 1/6] Feat: Don't add signal handlers on Windows --- telegram/ext/_application.py | 15 ++++++++++++--- tests/test_application.py | 25 +++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/telegram/ext/_application.py b/telegram/ext/_application.py index 96a332a7cd5..b39ad0c3d4b 100644 --- a/telegram/ext/_application.py +++ b/telegram/ext/_application.py @@ -21,6 +21,7 @@ import inspect import itertools import logging +import platform import signal from collections import defaultdict from contextlib import AbstractAsyncContextManager @@ -547,7 +548,7 @@ def run_polling( allowed_updates: List[str] = None, drop_pending_updates: bool = None, close_loop: bool = True, - stop_signals: Optional[Sequence[int]] = (signal.SIGINT, signal.SIGTERM, signal.SIGABRT), + stop_signals: Optional[Sequence[int]] = None, ) -> None: """Convenience method that takes care of initializing and starting the app, polling updates from Telegram using :meth:`telegram.ext.Updater.start_polling` and @@ -596,7 +597,7 @@ def run_polling( stop_signals (Sequence[:obj:`int`] | :obj:`None`, optional): Signals that will shut down the app. Pass :obj:`None` to not use stop signals. Defaults to :data:`signal.SIGINT`, :data:`signal.SIGTERM` and - :data:`signal.SIGABRT`. + :data:`signal.SIGABRT` on non Windows platforms. Caution: Not every :class:`asyncio.AbstractEventLoop` implements @@ -615,6 +616,10 @@ def run_polling( def error_callback(exc: TelegramError) -> None: self.create_task(self.process_error(error=exc, update=None)) + if stop_signals is None: + if platform.system() != "windows": + stop_signals = (signal.SIGINT, signal.SIGTERM, signal.SIGABRT) + return self.__run( updater_coroutine=self.updater.start_polling( poll_interval=poll_interval, @@ -646,7 +651,7 @@ def run_webhook( ip_address: str = None, max_connections: int = 40, close_loop: bool = True, - stop_signals: Optional[Sequence[int]] = (signal.SIGINT, signal.SIGTERM, signal.SIGABRT), + stop_signals: Optional[Sequence[int]] = None, ) -> None: """Convenience method that takes care of initializing and starting the app, polling updates from Telegram using :meth:`telegram.ext.Updater.start_webhook` and @@ -711,6 +716,10 @@ def run_webhook( "Application.run_webhook is only available if the application has an Updater." ) + if stop_signals is None: + if platform.system() != "windows": + stop_signals = (signal.SIGINT, signal.SIGTERM, signal.SIGABRT) + return self.__run( updater_coroutine=self.updater.start_webhook( listen=listen, diff --git a/tests/test_application.py b/tests/test_application.py index ce7f48c138c..94767d88086 100644 --- a/tests/test_application.py +++ b/tests/test_application.py @@ -1680,3 +1680,28 @@ async def raise_method(*args, **kwargs): app.run_webhook(close_loop=False, stop_signals=None) assert len(recwarn) == 0 + + @pytest.mark.dev + def test_signal_handlers(self, app, monkeypatch): + # this test should make sure that signal handlers are set by default on Linux + Mac, + # and not on Windows. + + def signal_handler_test(*args, **kwargs): + # args[0] is the signal, [1] the callback + assert args[0] in (signal.SIGINT, signal.SIGTERM, signal.SIGABRT) + + loop = asyncio.get_event_loop() + monkeypatch.setattr(loop, "add_signal_handler", signal_handler_test) + + async def abort_app(context): + raise KeyboardInterrupt + + app.job_queue.run_once(abort_app, 2) + app.run_polling(close_loop=False) + + app.job_queue.run_once(abort_app, 2) + app.run_webhook(port=49152, webhook_url="example.com", close_loop=False) + + if platform.system() == "windows": + with pytest.raises(PTBUserWarning): + app.run_polling(close_loop=False, stop_signals=signal.SIGINT) From 66944bb2c535a434cbe69d809f188f671c819162 Mon Sep 17 00:00:00 2001 From: poolitzer Date: Sun, 29 May 2022 19:11:13 +0200 Subject: [PATCH 2/6] Fix: Better implementation? --- telegram/ext/_application.py | 22 +++++++++------------- tests/test_application.py | 5 +++-- 2 files changed, 12 insertions(+), 15 deletions(-) diff --git a/telegram/ext/_application.py b/telegram/ext/_application.py index b39ad0c3d4b..eb1242f4f30 100644 --- a/telegram/ext/_application.py +++ b/telegram/ext/_application.py @@ -548,7 +548,7 @@ def run_polling( allowed_updates: List[str] = None, drop_pending_updates: bool = None, close_loop: bool = True, - stop_signals: Optional[Sequence[int]] = None, + stop_signals: DVInput[Sequence[int]] = DEFAULT_NONE, ) -> None: """Convenience method that takes care of initializing and starting the app, polling updates from Telegram using :meth:`telegram.ext.Updater.start_polling` and @@ -616,10 +616,6 @@ def run_polling( def error_callback(exc: TelegramError) -> None: self.create_task(self.process_error(error=exc, update=None)) - if stop_signals is None: - if platform.system() != "windows": - stop_signals = (signal.SIGINT, signal.SIGTERM, signal.SIGABRT) - return self.__run( updater_coroutine=self.updater.start_polling( poll_interval=poll_interval, @@ -651,7 +647,7 @@ def run_webhook( ip_address: str = None, max_connections: int = 40, close_loop: bool = True, - stop_signals: Optional[Sequence[int]] = None, + stop_signals: DVInput[Sequence[int]] = DEFAULT_NONE, ) -> None: """Convenience method that takes care of initializing and starting the app, polling updates from Telegram using :meth:`telegram.ext.Updater.start_webhook` and @@ -716,10 +712,6 @@ def run_webhook( "Application.run_webhook is only available if the application has an Updater." ) - if stop_signals is None: - if platform.system() != "windows": - stop_signals = (signal.SIGINT, signal.SIGTERM, signal.SIGABRT) - return self.__run( updater_coroutine=self.updater.start_webhook( listen=listen, @@ -745,7 +737,7 @@ def _raise_system_exit() -> NoReturn: def __run( self, updater_coroutine: Coroutine, - stop_signals: Optional[Sequence[int]], + stop_signals: DVInput[Sequence[int]], close_loop: bool = True, ) -> None: # Calling get_event_loop() should still be okay even in py3.10+ as long as there is a @@ -753,9 +745,13 @@ def __run( # See the docs of get_event_loop() and get_running_loop() for more info loop = asyncio.get_event_loop() + if stop_signals is DEFAULT_NONE and platform.system() != "Windows": + stop_signals = (signal.SIGINT, signal.SIGTERM, signal.SIGABRT) + try: - for sig in stop_signals or []: - loop.add_signal_handler(sig, self._raise_system_exit) + if not isinstance(stop_signals, DefaultValue): + for sig in stop_signals or []: + loop.add_signal_handler(sig, self._raise_system_exit) except NotImplementedError as exc: warn( f"Could not add signal handlers for the stop signals {stop_signals} due to " diff --git a/tests/test_application.py b/tests/test_application.py index 94767d88086..cfd298ab675 100644 --- a/tests/test_application.py +++ b/tests/test_application.py @@ -1702,6 +1702,7 @@ async def abort_app(context): app.job_queue.run_once(abort_app, 2) app.run_webhook(port=49152, webhook_url="example.com", close_loop=False) - if platform.system() == "windows": - with pytest.raises(PTBUserWarning): + if platform.system() == "Windows": + monkeypatch.undo() + with pytest.raises(PTBUserWarning, match="Could not add signal handlers for the stop"): app.run_polling(close_loop=False, stop_signals=signal.SIGINT) From 96f05c92b46a086912618cd61f874960c4492e7f Mon Sep 17 00:00:00 2001 From: poolitzer Date: Sun, 29 May 2022 20:34:25 +0200 Subject: [PATCH 3/6] Fix: Wrong type submitted --- telegram/ext/_application.py | 6 +++--- tests/test_application.py | 3 +-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/telegram/ext/_application.py b/telegram/ext/_application.py index eb1242f4f30..63320ec690d 100644 --- a/telegram/ext/_application.py +++ b/telegram/ext/_application.py @@ -548,7 +548,7 @@ def run_polling( allowed_updates: List[str] = None, drop_pending_updates: bool = None, close_loop: bool = True, - stop_signals: DVInput[Sequence[int]] = DEFAULT_NONE, + stop_signals: ODVInput[Sequence[int]] = DEFAULT_NONE, ) -> None: """Convenience method that takes care of initializing and starting the app, polling updates from Telegram using :meth:`telegram.ext.Updater.start_polling` and @@ -647,7 +647,7 @@ def run_webhook( ip_address: str = None, max_connections: int = 40, close_loop: bool = True, - stop_signals: DVInput[Sequence[int]] = DEFAULT_NONE, + stop_signals: ODVInput[Sequence[int]] = DEFAULT_NONE, ) -> None: """Convenience method that takes care of initializing and starting the app, polling updates from Telegram using :meth:`telegram.ext.Updater.start_webhook` and @@ -737,7 +737,7 @@ def _raise_system_exit() -> NoReturn: def __run( self, updater_coroutine: Coroutine, - stop_signals: DVInput[Sequence[int]], + stop_signals: ODVInput[Sequence[int]], close_loop: bool = True, ) -> None: # Calling get_event_loop() should still be okay even in py3.10+ as long as there is a diff --git a/tests/test_application.py b/tests/test_application.py index cfd298ab675..25941b9b736 100644 --- a/tests/test_application.py +++ b/tests/test_application.py @@ -1681,7 +1681,6 @@ async def raise_method(*args, **kwargs): assert len(recwarn) == 0 - @pytest.mark.dev def test_signal_handlers(self, app, monkeypatch): # this test should make sure that signal handlers are set by default on Linux + Mac, # and not on Windows. @@ -1705,4 +1704,4 @@ async def abort_app(context): if platform.system() == "Windows": monkeypatch.undo() with pytest.raises(PTBUserWarning, match="Could not add signal handlers for the stop"): - app.run_polling(close_loop=False, stop_signals=signal.SIGINT) + app.run_polling(close_loop=False, stop_signals=(signal.SIGINT,)) From f6322f1b31b342f1855fa87633394397741402e9 Mon Sep 17 00:00:00 2001 From: poolitzer Date: Mon, 30 May 2022 09:55:09 +0200 Subject: [PATCH 4/6] Fix: Make tests make sense --- tests/test_application.py | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/tests/test_application.py b/tests/test_application.py index 25941b9b736..f6e0c1c1a34 100644 --- a/tests/test_application.py +++ b/tests/test_application.py @@ -1659,9 +1659,9 @@ async def raise_method(*args, **kwargs): with pytest.raises(RuntimeError, match="Prevent Actually Running"): if "polling" in method: - app.run_polling(close_loop=False) + app.run_polling(close_loop=False, stop_signals=(signal.SIGINT,)) else: - app.run_webhook(close_loop=False) + app.run_webhook(close_loop=False, stop_signals=(signal.SIGTERM,)) assert len(recwarn) >= 1 found = False @@ -1702,6 +1702,15 @@ async def abort_app(context): app.run_webhook(port=49152, webhook_url="example.com", close_loop=False) if platform.system() == "Windows": - monkeypatch.undo() - with pytest.raises(PTBUserWarning, match="Could not add signal handlers for the stop"): - app.run_polling(close_loop=False, stop_signals=(signal.SIGINT,)) + + def signal_handler_test_windows(*args, **kwargs): + # this should be empty, but still called once. + assert args == [] + + monkeypatch.setattr(loop, "add_signal_handler", signal_handler_test_windows) + # this should run through, and we expect the signals to be empty + app.job_queue.run_once(abort_app, 2) + # pass false so it is not default + app.run_webhook( + port=49152, webhook_url="example.com", close_loop=False, stop_signals=False + ) From 37fe746ceeda156e78a053df5cd44bdf5528c068 Mon Sep 17 00:00:00 2001 From: poolitzer Date: Mon, 30 May 2022 21:12:01 +0200 Subject: [PATCH 5/6] Fix: Improving tests, again. --- tests/test_application.py | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/tests/test_application.py b/tests/test_application.py index f6e0c1c1a34..c0eb9849a08 100644 --- a/tests/test_application.py +++ b/tests/test_application.py @@ -1681,13 +1681,16 @@ async def raise_method(*args, **kwargs): assert len(recwarn) == 0 + @pytest.mark.dev def test_signal_handlers(self, app, monkeypatch): # this test should make sure that signal handlers are set by default on Linux + Mac, # and not on Windows. + received_signals = [] + def signal_handler_test(*args, **kwargs): # args[0] is the signal, [1] the callback - assert args[0] in (signal.SIGINT, signal.SIGTERM, signal.SIGABRT) + received_signals.append(args[0]) loop = asyncio.get_event_loop() monkeypatch.setattr(loop, "add_signal_handler", signal_handler_test) @@ -1698,19 +1701,16 @@ async def abort_app(context): app.job_queue.run_once(abort_app, 2) app.run_polling(close_loop=False) + if platform.system() == "Windows": + assert received_signals == [] + else: + assert received_signals == [signal.SIGINT, signal.SIGTERM, signal.SIGABRT] + + received_signals.clear() app.job_queue.run_once(abort_app, 2) app.run_webhook(port=49152, webhook_url="example.com", close_loop=False) if platform.system() == "Windows": - - def signal_handler_test_windows(*args, **kwargs): - # this should be empty, but still called once. - assert args == [] - - monkeypatch.setattr(loop, "add_signal_handler", signal_handler_test_windows) - # this should run through, and we expect the signals to be empty - app.job_queue.run_once(abort_app, 2) - # pass false so it is not default - app.run_webhook( - port=49152, webhook_url="example.com", close_loop=False, stop_signals=False - ) + assert received_signals == [] + else: + assert received_signals == [signal.SIGINT, signal.SIGTERM, signal.SIGABRT] From d218e41eff1e6566a952bca53576dcbae77923b4 Mon Sep 17 00:00:00 2001 From: poolitzer <25934244+Poolitzer@users.noreply.github.com> Date: Tue, 31 May 2022 19:48:48 +0200 Subject: [PATCH 6/6] Fix: maybe not breaking windows? In case we break, we also have timeout now I decided --- requirements-dev.txt | 1 + tests/test_application.py | 12 +++++++----- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/requirements-dev.txt b/requirements-dev.txt index 5cd0fa698cf..6e30d0d3fc3 100644 --- a/requirements-dev.txt +++ b/requirements-dev.txt @@ -12,6 +12,7 @@ isort==5.10.1 pytest==7.1.2 pytest-asyncio==0.18.3 +pytest-timeout==2.1.0 # used to timeout tests flaky # Used for flaky tests (flaky decorator) beautifulsoup4 # used in test_official for parsing tg docs diff --git a/tests/test_application.py b/tests/test_application.py index c0eb9849a08..8070bafb408 100644 --- a/tests/test_application.py +++ b/tests/test_application.py @@ -1681,7 +1681,7 @@ async def raise_method(*args, **kwargs): assert len(recwarn) == 0 - @pytest.mark.dev + @pytest.mark.timeout(6) def test_signal_handlers(self, app, monkeypatch): # this test should make sure that signal handlers are set by default on Linux + Mac, # and not on Windows. @@ -1695,10 +1695,12 @@ def signal_handler_test(*args, **kwargs): loop = asyncio.get_event_loop() monkeypatch.setattr(loop, "add_signal_handler", signal_handler_test) - async def abort_app(context): - raise KeyboardInterrupt + async def abort_app(): + await asyncio.sleep(2) + raise SystemExit + + loop.create_task(abort_app()) - app.job_queue.run_once(abort_app, 2) app.run_polling(close_loop=False) if platform.system() == "Windows": @@ -1707,7 +1709,7 @@ async def abort_app(context): assert received_signals == [signal.SIGINT, signal.SIGTERM, signal.SIGABRT] received_signals.clear() - app.job_queue.run_once(abort_app, 2) + loop.create_task(abort_app()) app.run_webhook(port=49152, webhook_url="example.com", close_loop=False) if platform.system() == "Windows":