Repository: python-telegram-bot/python-telegram-bot
Pull request: Don't call `done_callback` if Promise raises an exception.
Closes #2542 

Problem statement:
[BUG] promise mistreats pooled_function exceptions as done_callback exceptions
<!--
Thanks for reporting issues of python-telegram-bot!

Use this template to notify us if you found a bug.

To make it easier for us to help you please enter detailed information below.

Please note, we only support the latest version of python-telegram-bot and
master branch. Please make sure to upgrade & recreate the issue on the latest
version prior to opening an issue.
-->
### Steps to reproduce
1. At the lowest level, create a `telegram.ext.utils.Promise`, throw an exception from the `pooled_function` on which the promise was built.
2. Use `DelayQueue` using `@mq.queuedmessage` decorator on a subclassed Bot class. any exception like TimeOut or flood control within a `send_message` for example ends up with this problem
3. Use MessageQueue - I know this is deprecated, but that isn't the problematic code here. Promise itself isn't correctly handling exceptions.

If you get any telegram exception, it just gets eaten up by Promise's run method in this specific area:
```python
            if self._done_callback:
                try:
                    self._done_callback(self.result())
                except Exception as exc:
                    logger.warning(
                        "`done_callback` of a Promise raised the following exception."
                        " The exception won't be handled by error handlers."
                    )
                    logger.warning("Full traceback:", exc_info=exc)
```

Here's the bug: even if there's an exception in `self.result()` it will treat it as an exception from `self._done_callback()` which isn't correct.

The funny thing is that code speaks out the problem on its own:
```python
      try:
            self._result = self.pooled_function(*self.args, **self.kwargs)

        except Exception as exc:
            self._exception = exc
```
if there's an exception in `pooled_function`, it is stored in `self._exception`

and then it is raised from `self.result()`:
```python
    def result(self, timeout: float = None) -> Optional[RT]:
        self.done.wait(timeout=timeout)
        if self._exception is not None:
            raise self._exception  # pylint: disable=raising-bad-type
        return self._result
```
why? this way all exception from `pooled_function` are forcibly going to be taken as exceptions of `done_callback`, eaten up by the promise code and never raised.

### Expected behaviour
`Promise` should ideally have another method called `add_exception_callback` or similar just like `add_done_callback`, and pass any exception from `pooled_function` to that callback

### Actual behaviour
Promise mistreats `pooled_function` exceptions as `done_callback` exceptions

### Configuration
**Operating System:**
linux

**Version of Python, python-telegram-bot & dependencies:**
```
Bot API 5.2
certifi 2020.12.05
Python 3.8.5 (default, Jul 21 2020, 10:42:08)  [Clang 11.0.0 (clang-1100.0.33.17)]
```
### Logs
Insert logs here (if necessary)
```
2021-06-04 12:23:17:WARNING:DelayQueue-1:telegram.ext.utils.promise:91: `done_callback` of a Promise raised the following exception. The exception won't be handled by error handlers.
2021-06-04 12:23:17:WARNING:DelayQueue-1:telegram.ext.utils.promise:95: Full traceback:
Traceback (most recent call last):
  File "/Users/asingh/workspace/tgbot/venv/lib/python3.8/site-packages/telegram/vendor/ptb_urllib3/urllib3/connectionpool.py", line 402, in _make_request
    six.raise_from(e, None)
  File "<string>", line 2, in raise_from
  File "/Users/asingh/workspace/tgbot/venv/lib/python3.8/site-packages/telegram/vendor/ptb_urllib3/urllib3/connectionpool.py", line 398, in _make_request
    httplib_response = conn.getresponse()
  File "/usr/local/Cellar/python@3.8/3.8.5/Frameworks/Python.framework/Versions/3.8/lib/python3.8/http/client.py", line 1347, in getresponse
    response.begin()
  File "/usr/local/Cellar/python@3.8/3.8.5/Frameworks/Python.framework/Versions/3.8/lib/python3.8/http/client.py", line 307, in begin
    version, status, reason = self._read_status()
  File "/usr/local/Cellar/python@3.8/3.8.5/Frameworks/Python.framework/Versions/3.8/lib/python3.8/http/client.py", line 268, in _read_status
    line = str(self.fp.readline(_MAXLINE + 1), "iso-8859-1")
  File "/usr/local/Cellar/python@3.8/3.8.5/Frameworks/Python.framework/Versions/3.8/lib/python3.8/socket.py", line 669, in readinto
    return self._sock.recv_into(b)
  File "/usr/local/Cellar/python@3.8/3.8.5/Frameworks/Python.framework/Versions/3.8/lib/python3.8/ssl.py", line 1241, in recv_into
    return self.read(nbytes, buffer)
  File "/usr/local/Cellar/python@3.8/3.8.5/Frameworks/Python.framework/Versions/3.8/lib/python3.8/ssl.py", line 1099, in read
    return self._sslobj.read(len, buffer)
socket.timeout: The read operation timed out

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/asingh/workspace/tgbot/venv/lib/python3.8/site-packages/telegram/utils/request.py", line 253, in _request_wrapper
    resp = self._con_pool.request(*args, **kwargs)
  File "/Users/asingh/workspace/tgbot/venv/lib/python3.8/site-packages/telegram/vendor/ptb_urllib3/urllib3/request.py", line 68, in request
    return self.request_encode_body(method, url, fields=fields,
  File "/Users/asingh/workspace/tgbot/venv/lib/python3.8/site-packages/telegram/vendor/ptb_urllib3/urllib3/request.py", line 148, in request_encode_body
    return self.urlopen(method, url, **extra_kw)
  File "/Users/asingh/workspace/tgbot/venv/lib/python3.8/site-packages/telegram/vendor/ptb_urllib3/urllib3/poolmanager.py", line 244, in urlopen
    response = conn.urlopen(method, u.request_uri, **kw)
  File "/Users/asingh/workspace/tgbot/venv/lib/python3.8/site-packages/telegram/vendor/ptb_urllib3/urllib3/connectionpool.py", line 665, in urlopen
    retries = retries.increment(method, url, error=e, _pool=self,
  File "/Users/asingh/workspace/tgbot/venv/lib/python3.8/site-packages/telegram/vendor/ptb_urllib3/urllib3/util/retry.py", line 347, in increment
    raise six.reraise(type(error), error, _stacktrace)
  File "/Users/asingh/workspace/tgbot/venv/lib/python3.8/site-packages/telegram/vendor/ptb_urllib3/urllib3/packages/six.py", line 686, in reraise
    raise value
  File "/Users/asingh/workspace/tgbot/venv/lib/python3.8/site-packages/telegram/vendor/ptb_urllib3/urllib3/connectionpool.py", line 614, in urlopen
    httplib_response = self._make_request(conn, method, url,
  File "/Users/asingh/workspace/tgbot/venv/lib/python3.8/site-packages/telegram/vendor/ptb_urllib3/urllib3/connectionpool.py", line 404, in _make_request
    self._raise_timeout(err=e, url=url, timeout_value=read_timeout,
  File "/Users/asingh/workspace/tgbot/venv/lib/python3.8/site-packages/telegram/vendor/ptb_urllib3/urllib3/connectionpool.py", line 321, in _raise_timeout
    raise exc_cls(*args)
telegram.vendor.ptb_urllib3.urllib3.exceptions.ReadTimeoutError: HTTPSConnectionPool(host='api.telegram.org', port=443): Read timed out. (read timeout=5.0)

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/Users/asingh/workspace/tgbot/venv/lib/python3.8/site-packages/telegram/ext/utils/promise.py", line 89, in run
    self._done_callback(self.result())
  File "/Users/asingh/workspace/tgbot/venv/lib/python3.8/site-packages/telegram/ext/utils/promise.py", line 116, in result
    raise self._exception  # pylint: disable=raising-bad-type
  File "/Users/asingh/workspace/tgbot/venv/lib/python3.8/site-packages/telegram/ext/utils/promise.py", line 80, in run
    self._result = self.pooled_function(*self.args, **self.kwargs)
  File "/Users/asingh/workspace/tgbot/tgbotschedulebot/tg_mq_bot.py", line 31, in editMessageText
    return super(MQBot, self).edit_message_text(*args, **kwargs)
  File "/Users/asingh/workspace/tgbot/venv/lib/python3.8/site-packages/telegram/bot.py", line 127, in decorator
    result = func(*args, **kwargs)
  File "/Users/asingh/workspace/tgbot/venv/lib/python3.8/site-packages/telegram/bot.py", line 2483, in edit_message_text
    return self._message(
  File "/Users/asingh/workspace/tgbot/venv/lib/python3.8/site-packages/telegram/bot.py", line 296, in _message
    result = self._post(endpoint, data, timeout=timeout, api_kwargs=api_kwargs)
  File "/Users/asingh/workspace/tgbot/venv/lib/python3.8/site-packages/telegram/bot.py", line 259, in _post
    return self.request.post(
  File "/Users/asingh/workspace/tgbot/venv/lib/python3.8/site-packages/telegram/utils/request.py", line 350, in post
    result = self._request_wrapper(
  File "/Users/asingh/workspace/tgbot/venv/lib/python3.8/site-packages/telegram/utils/request.py", line 255, in _request_wrapper
    raise TimedOut() from error
telegram.error.TimedOut: Timed out
```

Hints from issue discussion:
Hi. Thanks for the detailed report! 👍 

Seems like we overlooked the fact that `result()` raises `_exception` in #2417. I guess it only makes sense to call `done_callback` when `pooled_function` didn't raise a exception, anyway. So we could adjust  `run` to do that and document this fact in `add_done_callback`.

Please note that `add_done_callback` was mainly introduced for conversation timeouts, where we have proper error handling already within the pooled function. Also `Promise` is not really part of the public interface and will likely be removed/overhauled completey in v14 (see #2352), so personally I don't see a need to add an `add_exception_callback`.
> Also `Promise` is not really part of the public interface and will likely be removed/overhauled completey in v14 (see #2352), so personally I don't see a need to add an `add_exception_callback`.

+1, as we don't know what will be the future of Promise in asyncio, it's better to just call the done callback when promise didn't raise an exception.


IMO the fix is simple: https://github.com/python-telegram-bot/python-telegram-bot/pull/2543
I understand all of this will go away in the future with asyncio, but for now this fix is the only way for letting users of the library retry on timeouts, other network exceptions or flood limit exceptions from telegram's API

Commit message: Don't call done_cb on exceptions

Signed-off-by: starry69 <starry369126@outlook.com>

Patch to review:
diff --git a/telegram/ext/filters.py b/telegram/ext/filters.py
index 15610d27362..b6cc6f6b68e 100644
--- a/telegram/ext/filters.py
+++ b/telegram/ext/filters.py
@@ -634,7 +634,7 @@ def __init__(self, pattern: Union[str, Pattern]):
             self.name = f'Filters.regex({self.pattern})'
 
         def filter(self, message: Message) -> Optional[Dict[str, List[Match]]]:
-            """"""  # remove method from docs
+            """ """  # remove method from docs
             if message.text:
                 match = self.pattern.search(message.text)
                 if match:
@@ -670,7 +670,7 @@ def __init__(self, pattern: Union[str, Pattern]):
             self.name = f'Filters.caption_regex({self.pattern})'
 
         def filter(self, message: Message) -> Optional[Dict[str, List[Match]]]:
-            """"""  # remove method from docs
+            """ """  # remove method from docs
             if message.caption:
                 match = self.pattern.search(message.caption)
                 if match:
@@ -727,7 +727,7 @@ def __init__(self, category: Optional[str]):
                 self.name = f"Filters.document.category('{self._category}')"
 
             def filter(self, message: Message) -> bool:
-                """"""  # remove method from docs
+                """ """  # remove method from docs
                 if message.document:
                     return message.document.mime_type.startswith(self._category)
                 return False
@@ -758,7 +758,7 @@ def __init__(self, mimetype: Optional[str]):
                 self.name = f"Filters.document.mime_type('{self.mimetype}')"
 
             def filter(self, message: Message) -> bool:
-                """"""  # remove method from docs
+                """ """  # remove method from docs
                 if message.document:
                     return message.document.mime_type == self.mimetype
                 return False
@@ -832,7 +832,7 @@ def __init__(self, file_extension: Optional[str], case_sensitive: bool = False):
                     self.name = f"Filters.document.file_extension({file_extension.lower()!r})"
 
             def filter(self, message: Message) -> bool:
-                """"""  # remove method from docs
+                """ """  # remove method from docs
                 if message.document is None:
                     return False
                 if self._file_extension is None:
@@ -1298,7 +1298,7 @@ def __init__(self, entity_type: str):
             self.name = f'Filters.entity({self.entity_type})'
 
         def filter(self, message: Message) -> bool:
-            """"""  # remove method from docs
+            """ """  # remove method from docs
             return any(entity.type == self.entity_type for entity in message.entities)
 
     class caption_entity(MessageFilter):
@@ -1322,7 +1322,7 @@ def __init__(self, entity_type: str):
             self.name = f'Filters.caption_entity({self.entity_type})'
 
         def filter(self, message: Message) -> bool:
-            """"""  # remove method from docs
+            """ """  # remove method from docs
             return any(entity.type == self.entity_type for entity in message.caption_entities)
 
     class _Private(MessageFilter):
@@ -1563,7 +1563,7 @@ def remove_chat_ids(self, chat_id: SLT[int]) -> None:
                 self._chat_ids -= parsed_chat_id
 
         def filter(self, message: Message) -> bool:
-            """"""  # remove method from docs
+            """ """  # remove method from docs
             chat_or_user = self.get_chat_or_user(message)
             if chat_or_user:
                 if self.chat_ids:
@@ -2206,7 +2206,7 @@ def __init__(self, lang: SLT[str]):
             self.name = f'Filters.language({self.lang})'
 
         def filter(self, message: Message) -> bool:
-            """"""  # remove method from docs
+            """ """  # remove method from docs
             return bool(
                 message.from_user.language_code
                 and any(message.from_user.language_code.startswith(x) for x in self.lang)
diff --git a/telegram/ext/utils/promise.py b/telegram/ext/utils/promise.py
index ff74bd56f82..6f468201754 100644
--- a/telegram/ext/utils/promise.py
+++ b/telegram/ext/utils/promise.py
@@ -100,7 +100,7 @@ def run(self) -> None:
 
         finally:
             self.done.set()
-            if self._done_callback:
+            if self._exception is None and self._done_callback:
                 try:
                     self._done_callback(self.result())
                 except Exception as exc:
@@ -136,6 +136,10 @@ def add_done_callback(self, callback: Callable) -> None:
         """
         Callback to be run when :class:`telegram.ext.utils.promise.Promise` becomes done.
 
+        Note:
+            Callback won't be called if `:class:`telegram.ext.utils.promise.Promise`
+            raises an exception.
+
         Args:
             callback (:obj:`callable`): The callable that will be called when promise is done.
             callback will be called by passing ``Promise.result()`` as only positional argument.
diff --git a/tests/test_promise.py b/tests/test_promise.py
index ace0a358d65..ceb9f196e7d 100644
--- a/tests/test_promise.py
+++ b/tests/test_promise.py
@@ -136,3 +136,17 @@ def done_callback(_):
         )
         assert caplog.records[1].message.startswith("Full traceback:")
         assert promise.result() == "done!"
+
+    def test_done_cb_not_run_on_excp(self):
+        def callback():
+            raise TelegramError('Error')
+
+        def done_callback(_):
+            self.test_flag = True
+
+        promise = Promise(callback, [], {})
+        promise.add_done_callback(done_callback)
+        promise.run()
+        assert isinstance(promise.exception, TelegramError)
+        assert promise.done
+        assert self.test_flag is False


Code review comments:
diff_hunk: @@ -136,6 +136,10 @@ def add_done_callback(self, callback: Callable) -> None:
         """
         Callback to be run when :class:`telegram.ext.utils.promise.Promise` becomes done.
 
+        Note:
+            Callback won't be called if `:class:`telegram.ext.utils.promise.Promise`
path: telegram/ext/utils/promise.py
text: @user1:
```suggestion
            Callback won't be called if :attr:`pooled_function`
```
?