From beb81ea23cb088ff0d9c957120088e95d9e4ab0b Mon Sep 17 00:00:00 2001 From: Harshil <37377066+harshil21@users.noreply.github.com> Date: Tue, 14 Sep 2021 21:56:16 +0400 Subject: [PATCH 1/5] Use Parallel Execution Of Pylint (#2658) --- .pre-commit-config.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index d3056152e3f..e3ad3ff4388 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -20,6 +20,8 @@ repos: files: ^(telegram|examples)/.*\.py$ args: - --rcfile=setup.cfg + # run pylint across multiple cpu cores to speed it up- + - --jobs=0 # See https://pylint.pycqa.org/en/latest/user_guide/run.html?#parallel-execution to know more additional_dependencies: - certifi - tornado>=6.1 From 504f9d0550a44a40c98ae1f5e16425a9d92ba9ec Mon Sep 17 00:00:00 2001 From: Hinrich Mahler <22366557+Bibo-Joshi@users.noreply.github.com> Date: Fri, 3 Sep 2021 22:18:53 +0200 Subject: [PATCH 2/5] Move error_handler error handing into Dispatcher.dispatch_error --- telegram/ext/dispatcher.py | 121 +++++++++++++++++-------------------- telegram/ext/jobqueue.py | 20 +----- tests/test_dispatcher.py | 29 +++++---- tests/test_jobqueue.py | 4 +- 4 files changed, 76 insertions(+), 98 deletions(-) diff --git a/telegram/ext/dispatcher.py b/telegram/ext/dispatcher.py index 55c1485202b..22d3736122b 100644 --- a/telegram/ext/dispatcher.py +++ b/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 handler or 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,56 +603,24 @@ 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, @@ -663,15 +628,12 @@ def add_error_handler( 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,9 +662,21 @@ 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 + handler or handlers (even in other groups). All other exceptions raised by an error handler + will 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. @@ -710,9 +684,13 @@ def dispatch_error( 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 + dispatcher_handler_stop = False if self.error_handlers: for callback, run_async in self.error_handlers.items(): # pylint: disable=W0621 @@ -722,9 +700,20 @@ def dispatch_error( if run_async: self.run_async(callback, update, context, update=update) else: - callback(update, context) + try: + callback(update, context) + except DispatcherHandlerStop: + dispatcher_handler_stop = True + break + 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 dispatcher_handler_stop - 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 diff --git a/telegram/ext/jobqueue.py b/telegram/ext/jobqueue.py index 444ebe22c3f..ac255ad355b 100644 --- a/telegram/ext/jobqueue.py +++ b/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: """ diff --git a/tests/test_dispatcher.py b/tests/test_dispatcher.py index 11e766f60ce..de83d73cefb 100644 --- a/tests/test_dispatcher.py +++ b/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): diff --git a/tests/test_jobqueue.py b/tests/test_jobqueue.py index 67e6242b5e4..cfeb94a30b0 100644 --- a/tests/test_jobqueue.py +++ b/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() From bae00ea8e64ad7a87339478deec6c7374c02c8af Mon Sep 17 00:00:00 2001 From: Hinrich Mahler <22366557+Bibo-Joshi@users.noreply.github.com> Date: Wed, 15 Sep 2021 17:29:39 +0200 Subject: [PATCH 3/5] remove merge-artifact --- .pre-commit-config.yaml | 2 -- 1 file changed, 2 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index e3ad3ff4388..d3056152e3f 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -20,8 +20,6 @@ repos: files: ^(telegram|examples)/.*\.py$ args: - --rcfile=setup.cfg - # run pylint across multiple cpu cores to speed it up- - - --jobs=0 # See https://pylint.pycqa.org/en/latest/user_guide/run.html?#parallel-execution to know more additional_dependencies: - certifi - tornado>=6.1 From 36a777d044ef9d9ff56bed9bf3b5a18152eea7b8 Mon Sep 17 00:00:00 2001 From: Bibo-Joshi <22366557+Bibo-Joshi@users.noreply.github.com> Date: Thu, 16 Sep 2021 17:54:19 +0200 Subject: [PATCH 4/5] wording Co-authored-by: Harshil <37377066+harshil21@users.noreply.github.com> --- telegram/ext/dispatcher.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/telegram/ext/dispatcher.py b/telegram/ext/dispatcher.py index 22d3736122b..6b53162644d 100644 --- a/telegram/ext/dispatcher.py +++ b/telegram/ext/dispatcher.py @@ -62,7 +62,7 @@ class DispatcherHandlerStop(Exception): """ - Raise this in handler or error handler to prevent execution of any other handler (even in + 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 From 6ad636f47015f1a6a4973b5bf551534ed8bc0310 Mon Sep 17 00:00:00 2001 From: Hinrich Mahler <22366557+Bibo-Joshi@users.noreply.github.com> Date: Thu, 16 Sep 2021 18:14:52 +0200 Subject: [PATCH 5/5] Address review --- telegram/ext/dispatcher.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/telegram/ext/dispatcher.py b/telegram/ext/dispatcher.py index 6b53162644d..f33126e4c6e 100644 --- a/telegram/ext/dispatcher.py +++ b/telegram/ext/dispatcher.py @@ -670,8 +670,8 @@ def dispatch_error( """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 - handler or handlers (even in other groups). All other exceptions raised by an error handler - will be logged. + 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. @@ -690,7 +690,6 @@ def dispatch_error( """ async_args = None if not promise else promise.args async_kwargs = None if not promise else promise.kwargs - dispatcher_handler_stop = False if self.error_handlers: for callback, run_async in self.error_handlers.items(): # pylint: disable=W0621 @@ -703,15 +702,14 @@ def dispatch_error( try: callback(update, context) except DispatcherHandlerStop: - dispatcher_handler_stop = True - break + 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 dispatcher_handler_stop + return False self.logger.exception( 'No error handlers are registered, logging exception.', exc_info=error