diff --git a/changes/unreleased/4802.RMLufX4UazobYg5aZojyoD.toml b/changes/unreleased/4802.RMLufX4UazobYg5aZojyoD.toml new file mode 100644 index 00000000000..28745b0d9a8 --- /dev/null +++ b/changes/unreleased/4802.RMLufX4UazobYg5aZojyoD.toml @@ -0,0 +1,14 @@ +bugfixes = """ +Fixed a bug where calling ``Application.remove/add_handler`` during update handling can cause a ``RuntimeError`` in ``Application.process_update``. + +.. hint:: + Calling ``Application.add/remove_handler`` now has no influence on calls to :meth:`process_update` that are + already in progress. The same holds for ``Application.add/remove_error_handler`` and ``Application.process_error``, respectively. + + .. warning:: + This behavior should currently be considered an implementation detail and not as guaranteed behavior. +""" +[[pull_requests]] +uid = "4802" +author_uid = "Bibo-Joshi" +closes_threads = ["4803"] diff --git a/src/telegram/ext/_application.py b/src/telegram/ext/_application.py index e856fa85321..c48cd769918 100644 --- a/src/telegram/ext/_application.py +++ b/src/telegram/ext/_application.py @@ -1256,8 +1256,14 @@ async def process_update(self, update: object) -> None: context = None any_blocking = False # Flag which is set to True if any handler specifies block=True - for handlers in self.handlers.values(): + # We copy the lists to avoid issues with concurrent modification of the + # handlers (groups or handlers in groups) while iterating over it via add/remove_handler. + # Currently considered implementation detail as described in docstrings of + # add/remove_handler + # do *not* use `copy.deepcopy` here, as we don't want to deepcopy the handlers themselves + for handlers in [v.copy() for v in self.handlers.values()]: try: + # no copy needed b/c we copy above for handler in handlers: check = handler.check_update(update) # Should the handler handle this update? if check is None or check is False: @@ -1343,6 +1349,14 @@ def add_handler(self, handler: BaseHandler[Any, CCT, Any], group: int = DEFAULT_ might lead to race conditions and undesired behavior. In particular, current conversation states may be overridden by the loaded data. + Hint: + This method currently has no influence on calls to :meth:`process_update` that are + already in progress. + + .. warning:: + This behavior should currently be considered an implementation detail and not as + guaranteed behavior. + Args: handler (:class:`telegram.ext.BaseHandler`): A BaseHandler instance. group (:obj:`int`, optional): The group identifier. Default is ``0``. @@ -1444,6 +1458,14 @@ def remove_handler( ) -> None: """Remove a handler from the specified group. + Hint: + This method currently has no influence on calls to :meth:`process_update` that are + already in progress. + + .. warning:: + This behavior should currently be considered an implementation detail and not as + guaranteed behavior. + Args: handler (:class:`telegram.ext.BaseHandler`): A :class:`telegram.ext.BaseHandler` instance. @@ -1774,6 +1796,14 @@ def add_error_handler( Examples: :any:`Errorhandler Bot ` + Hint: + This method currently has no influence on calls to :meth:`process_error` that are + already in progress. + + .. warning:: + This behavior should currently be considered an implementation detail and not as + guaranteed behavior. + .. seealso:: :wiki:`Exceptions, Warnings and Logging ` Args: @@ -1797,6 +1827,14 @@ async def callback(update: Optional[object], context: CallbackContext) def remove_error_handler(self, callback: HandlerCallback[object, CCT, None]) -> None: """Removes an error handler. + Hint: + This method currently has no influence on calls to :meth:`process_error` that are + already in progress. + + .. warning:: + This behavior should currently be considered an implementation detail and not as + guaranteed behavior. + Args: callback (:term:`coroutine function`): The error handler to remove. @@ -1838,10 +1876,12 @@ async def process_error( :class:`telegram.ext.ApplicationHandlerStop`. :obj:`False`, otherwise. """ if self.error_handlers: - for ( - callback, - block, - ) in self.error_handlers.items(): + # We copy the list to avoid issues with concurrent modification of the + # error handlers while iterating over it via add/remove_error_handler. + # Currently considered implementation detail as described in docstrings of + # add/remove_error_handler + error_handler_items = list(self.error_handlers.items()) + for callback, block in error_handler_items: try: context = self.context_types.context.from_error( update=update, diff --git a/tests/ext/test_application.py b/tests/ext/test_application.py index f375559eb3a..c99a1311d27 100644 --- a/tests/ext/test_application.py +++ b/tests/ext/test_application.py @@ -2583,6 +2583,70 @@ async def callback(update, context): assert received_updates == {2} assert len(caplog.records) == 0 + @pytest.mark.parametrize("change_type", ["remove", "add"]) + async def test_process_update_handler_change_groups_during_iteration(self, app, change_type): + run_groups = set() + + async def dummy_callback(_, __, g: int): + run_groups.add(g) + + for group in range(10, 20): + handler = TypeHandler(int, functools.partial(dummy_callback, g=group)) + app.add_handler(handler, group=group) + + async def wait_callback(_, context): + # Trigger a change of the app.handlers dict during the iteration + if change_type == "remove": + context.application.remove_handler(handler, group) + else: + context.application.add_handler( + TypeHandler(int, functools.partial(dummy_callback, g=42)), group=42 + ) + + app.add_handler(TypeHandler(int, wait_callback)) + + async with app: + await app.process_update(1) + + # check that exactly those handlers were called that were configured when + # process_update was called + assert run_groups == set(range(10, 20)) + + async def test_process_update_handler_change_group_during_iteration(self, app): + async def dummy_callback(_, __): + pass + + checked_handlers = set() + + class TrackHandler(TypeHandler): + def __init__(self, name: str, *args, **kwargs): + self.name = name + super().__init__(*args, **kwargs) + + def check_update(self, update: object) -> bool: + checked_handlers.add(self.name) + return super().check_update(update) + + remove_handler = TrackHandler("remove", int, dummy_callback) + add_handler = TrackHandler("add", int, dummy_callback) + + class TriggerHandler(TypeHandler): + def check_update(self, update: object) -> bool: + # Trigger a change of the app.handlers *in the same group* during the iteration + app.remove_handler(remove_handler) + app.add_handler(add_handler) + # return False to ensure that additional handlers in the same group are checked + return False + + app.add_handler(TriggerHandler(str, dummy_callback)) + app.add_handler(remove_handler) + async with app: + await app.process_update("string update") + + # check that exactly those handlers were checked that were configured when + # process_update was called + assert checked_handlers == {"remove"} + async def test_process_error_exception_in_building_context(self, monkeypatch, caplog, app): # Makes sure that exceptions in building the context don't stop the application exception = ValueError("TestException") @@ -2622,3 +2686,28 @@ async def callback(update, context): assert received_errors == {2} assert len(caplog.records) == 0 + + @pytest.mark.parametrize("change_type", ["remove", "add"]) + async def test_process_error_change_during_iteration(self, app, change_type): + called_handlers = set() + + async def dummy_process_error(name: str, *_, **__): + called_handlers.add(name) + + add_error_handler = functools.partial(dummy_process_error, "add_handler") + remove_error_handler = functools.partial(dummy_process_error, "remove_handler") + + async def trigger_change(*_, **__): + if change_type == "remove": + app.remove_error_handler(remove_error_handler) + else: + app.add_error_handler(add_error_handler) + + app.add_error_handler(trigger_change) + app.add_error_handler(remove_error_handler) + async with app: + await app.process_error(update=None, error=None) + + # check that exactly those handlers were checked that were configured when + # add_error_handler was called + assert called_handlers == {"remove_handler"}