Refine Dispatcher.dispatch_error (#2660)

Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com>

telegram/ext/dispatcher.py

@@ -62,7 +62,8 @@
 
 class DispatcherHandlerStop(Exception):
     """
-    Raise this in handler to prevent execution of any other handler (even in different group).
+    Raise this in a handler or an error handler to prevent execution of any other handler (even in
+    different group).
 
     In order to use this exception in a :class:`telegram.ext.ConversationHandler`, pass the
     optional ``state`` parameter instead of returning the next state:
@@ -73,6 +74,9 @@ def callback(update, context):
             ...
             raise DispatcherHandlerStop(next_state)
 
+    Note:
+        Has no effect, if the handler or error handler is run asynchronously.
+
     Attributes:
         state (:obj:`object`): Optional. The next state of the conversation.
 
@@ -320,15 +324,16 @@ def _pooled(self) -> None:
 
             # Avoid infinite recursion of error handlers.
             if promise.pooled_function in self.error_handlers:
-                self.logger.error('An uncaught error was raised while handling the error.')
+                self.logger.exception(
+                    'An error was raised and an uncaught error was raised while '
+                    'handling the error with an error_handler.',
+                    exc_info=promise.exception,
+                )
                 continue
 
             # If we arrive here, an exception happened in the promise and was neither
             # DispatcherHandlerStop nor raised by an error handler. So we can and must handle it
-            try:
-                self.dispatch_error(promise.update, promise.exception, promise=promise)
-            except Exception:
-                self.logger.exception('An uncaught error was raised while handling the error.')
+            self.dispatch_error(promise.update, promise.exception, promise=promise)
 
     def run_async(
         self, func: Callable[..., object], *args: object, update: object = None, **kwargs: object
@@ -452,10 +457,7 @@ def process_update(self, update: object) -> None:
         """
         # An error happened while polling
         if isinstance(update, TelegramError):
-            try:
-                self.dispatch_error(None, update)
-            except Exception:
-                self.logger.exception('An uncaught error was raised while handling the error.')
+            self.dispatch_error(None, update)
             return
 
         context = None
@@ -483,14 +485,9 @@ def process_update(self, update: object) -> None:
 
             # Dispatch any error.
             except Exception as exc:
-                try:
-                    self.dispatch_error(update, exc)
-                except DispatcherHandlerStop:
-                    self.logger.debug('Error handler stopped further handlers')
+                if self.dispatch_error(update, exc):
+                    self.logger.debug('Error handler stopped further handlers.')
                     break
-                # Errors should not stop the thread.
-                except Exception:
-                    self.logger.exception('An uncaught error was raised while handling the error.')
 
         # Update persistence, if handled
         handled_only_async = all(sync_modes)
@@ -606,72 +603,37 @@ def __update_persistence(self, update: object = None) -> None:
                         self.bot.callback_data_cache.persistence_data
                     )
                 except Exception as exc:
-                    try:
-                        self.dispatch_error(update, exc)
-                    except Exception:
-                        message = (
-                            'Saving callback data raised an error and an '
-                            'uncaught error was raised while handling '
-                            'the error with an error_handler'
-                        )
-                        self.logger.exception(message)
+                    self.dispatch_error(update, exc)
             if self.persistence.store_data.bot_data:
                 try:
                     self.persistence.update_bot_data(self.bot_data)
                 except Exception as exc:
-                    try:
-                        self.dispatch_error(update, exc)
-                    except Exception:
-                        message = (
-                            'Saving bot data raised an error and an '
-                            'uncaught error was raised while handling '
-                            'the error with an error_handler'
-                        )
-                        self.logger.exception(message)
+                    self.dispatch_error(update, exc)
             if self.persistence.store_data.chat_data:
                 for chat_id in chat_ids:
                     try:
                         self.persistence.update_chat_data(chat_id, self.chat_data[chat_id])
                     except Exception as exc:
-                        try:
-                            self.dispatch_error(update, exc)
-                        except Exception:
-                            message = (
-                                'Saving chat data raised an error and an '
-                                'uncaught error was raised while handling '
-                                'the error with an error_handler'
-                            )
-                            self.logger.exception(message)
+                        self.dispatch_error(update, exc)
             if self.persistence.store_data.user_data:
                 for user_id in user_ids:
                     try:
                         self.persistence.update_user_data(user_id, self.user_data[user_id])
                     except Exception as exc:
-                        try:
-                            self.dispatch_error(update, exc)
-                        except Exception:
-                            message = (
-                                'Saving user data raised an error and an '
-                                'uncaught error was raised while handling '
-                                'the error with an error_handler'
-                            )
-                            self.logger.exception(message)
+                        self.dispatch_error(update, exc)
 
     def add_error_handler(
         self,
         callback: Callable[[object, CCT], None],
         run_async: Union[bool, DefaultValue] = DEFAULT_FALSE,  # pylint: disable=W0621
     ) -> None:
         """Registers an error handler in the Dispatcher. This handler will receive every error
-        which happens in your bot.
+        which happens in your bot. See the docs of :meth:`dispatch_error` for more details on how
+        errors are handled.
 
         Note:
             Attempts to add the same callback multiple times will be ignored.
 
-        Warning:
-            The errors handled within these handlers won't show up in the logger, so you
-            need to make sure that you reraise the error.
-
         Args:
             callback (:obj:`callable`): The callback function for this error handler. Will be
                 called when an error is raised.
@@ -700,16 +662,31 @@ def remove_error_handler(self, callback: Callable[[object, CCT], None]) -> None:
         self.error_handlers.pop(callback, None)
 
     def dispatch_error(
-        self, update: Optional[object], error: Exception, promise: Promise = None
-    ) -> None:
-        """Dispatches an error.
+        self,
+        update: Optional[object],
+        error: Exception,
+        promise: Promise = None,
+    ) -> bool:
+        """Dispatches an error by passing it to all error handlers registered with
+         :meth:`add_error_handler`. If one of the error handlers raises
+        :class:`telegram.ext.DispatcherHandlerStop`, the update will not be handled by other error
+        handlers or handlers (even in other groups). All other exceptions raised by an error
+        handler will just be logged.
+
+        .. versionchanged:: 14.0
+            * Exceptions raised by error handlers are now properly logged.
+            * :class:`telegram.ext.DispatcherHandlerStop` is no longer reraised but converted into
+                the return value.
 
         Args:
             update (:obj:`object` | :class:`telegram.Update`): The update that caused the error.
             error (:obj:`Exception`): The error that was raised.
             promise (:class:`telegram.utils.Promise`, optional): The promise whose pooled function
                 raised the error.
 
+        Returns:
+            :obj:`bool`: :obj:`True` if one of the error handlers raised
+                :class:`telegram.ext.DispatcherHandlerStop`. :obj:`False`, otherwise.
         """
         async_args = None if not promise else promise.args
         async_kwargs = None if not promise else promise.kwargs
@@ -722,9 +699,19 @@ def dispatch_error(
                 if run_async:
                     self.run_async(callback, update, context, update=update)
                 else:
-                    callback(update, context)
+                    try:
+                        callback(update, context)
+                    except DispatcherHandlerStop:
+                        return True
+                    except Exception as exc:
+                        self.logger.exception(
+                            'An error was raised and an uncaught error was raised while '
+                            'handling the error with an error_handler.',
+                            exc_info=exc,
+                        )
+            return False
 
-        else:
-            self.logger.exception(
-                'No error handlers are registered, logging exception.', exc_info=error
-            )
+        self.logger.exception(
+            'No error handlers are registered, logging exception.', exc_info=error
+        )
+        return False

telegram/ext/jobqueue.py

@@ -73,15 +73,7 @@ def _update_persistence(self, _: JobEvent) -> None:
         self._dispatcher.update_persistence()
 
     def _dispatch_error(self, event: JobEvent) -> None:
-        try:
-            self._dispatcher.dispatch_error(None, event.exception)
-        # Errors should not stop the thread.
-        except Exception:
-            self.logger.exception(
-                'An error was raised while processing the job and an '
-                'uncaught error was raised while handling the error '
-                'with an error_handler.'
-            )
+        self._dispatcher.dispatch_error(None, event.exception)
 
     @overload
     def _parse_time_input(self, time: None, shift_day: bool = False) -> None:
@@ -524,15 +516,7 @@ def run(self, dispatcher: 'Dispatcher') -> None:
         try:
             self.callback(dispatcher.context_types.context.from_job(self, dispatcher))
         except Exception as exc:
-            try:
-                dispatcher.dispatch_error(None, exc)
-            # Errors should not stop the thread.
-            except Exception:
-                dispatcher.logger.exception(
-                    'An error was raised while processing the job and an '
-                    'uncaught error was raised while handling the error '
-                    'with an error_handler.'
-                )
+            dispatcher.dispatch_error(None, exc)
 
     def schedule_removal(self) -> None:
         """

tests/test_dispatcher.py

@@ -298,7 +298,9 @@ def test_async_handler_error_handler_that_raises_error(self, dp, caplog):
             dp.update_queue.put(self.message_update)
             sleep(0.1)
             assert len(caplog.records) == 1
-            assert caplog.records[-1].getMessage().startswith('An uncaught error was raised')
+            assert (
+                caplog.records[-1].getMessage().startswith('An error was raised and an uncaught')
+            )
 
         # Make sure that the main loop still runs
         dp.remove_handler(handler)
@@ -316,7 +318,9 @@ def test_async_handler_async_error_handler_that_raises_error(self, dp, caplog):
             dp.update_queue.put(self.message_update)
             sleep(0.1)
             assert len(caplog.records) == 1
-            assert caplog.records[-1].getMessage().startswith('An uncaught error was raised')
+            assert (
+                caplog.records[-1].getMessage().startswith('An error was raised and an uncaught')
+            )
 
         # Make sure that the main loop still runs
         dp.remove_handler(handler)
@@ -631,7 +635,7 @@ def test_sensible_worker_thread_names(self, dp2):
         for thread_name in thread_names:
             assert thread_name.startswith(f"Bot:{dp2.bot.id}:worker:")
 
-    def test_error_while_persisting(self, dp, monkeypatch):
+    def test_error_while_persisting(self, dp, caplog):
         class OwnPersistence(BasePersistence):
             def update(self, data):
                 raise Exception('PersistenceError')
@@ -681,27 +685,30 @@ def flush(self):
         def callback(update, context):
             pass
 
-        test_flag = False
+        test_flag = []
 
         def error(update, context):
             nonlocal test_flag
-            test_flag = str(context.error) == 'PersistenceError'
+            test_flag.append(str(context.error) == 'PersistenceError')
             raise Exception('ErrorHandlingError')
 
-        def logger(message):
-            assert 'uncaught error was raised while handling' in message
-
         update = Update(
             1, message=Message(1, None, Chat(1, ''), from_user=User(1, '', False), text='Text')
         )
         handler = MessageHandler(Filters.all, callback)
         dp.add_handler(handler)
         dp.add_error_handler(error)
-        monkeypatch.setattr(dp.logger, 'exception', logger)
 
         dp.persistence = OwnPersistence()
-        dp.process_update(update)
-        assert test_flag
+
+        with caplog.at_level(logging.ERROR):
+            dp.process_update(update)
+
+        assert test_flag == [True, True, True, True]
+        assert len(caplog.records) == 4
+        for record in caplog.records:
+            message = record.getMessage()
+            assert message.startswith('An error was raised and an uncaught')
 
     def test_persisting_no_user_no_chat(self, dp):
         class OwnPersistence(BasePersistence):

tests/test_jobqueue.py

@@ -449,15 +449,13 @@ def test_dispatch_error_that_raises_errors(self, job_queue, dp, caplog):
         sleep(0.1)
         assert len(caplog.records) == 1
         rec = caplog.records[-1]
-        assert 'processing the job' in rec.getMessage()
-        assert 'uncaught error was raised while handling' in rec.getMessage()
+        assert 'An error was raised and an uncaught' in rec.getMessage()
         caplog.clear()
 
         with caplog.at_level(logging.ERROR):
             job.run(dp)
         assert len(caplog.records) == 1
         rec = caplog.records[-1]
-        assert 'processing the job' in rec.getMessage()
         assert 'uncaught error was raised while handling' in rec.getMessage()
         caplog.clear()