Skip to content

[BUG] promise mistreats pooled_function exceptions as done_callback exceptions #2542

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
angadsingh opened this issue Jun 4, 2021 · 3 comments · Fixed by #2544
Closed

[BUG] promise mistreats pooled_function exceptions as done_callback exceptions #2542

angadsingh opened this issue Jun 4, 2021 · 3 comments · Fixed by #2544
Milestone
v13.6

Comments

@angadsingh
Copy link

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:

            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:

      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():

    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
@Bibo-Joshi
Copy link
Member

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.

@Bibo-Joshi Bibo-Joshi added this to the v13.6 milestone Jun 4, 2021
@starry-shivam
Copy link
Member

starry-shivam commented Jun 4, 2021 •
edited
Loading

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.

angadsingh added a commit to angadsingh/python-telegram-bot that referenced this issue Jun 4, 2021
@angadsingh
proposed fix for python-telegram-bot#2542: proper treatment of pooled…
0034a5f 
…_function exceptions in Promise

send the pooled_function exception back through done_callback instead of eating it up, mistreating it as a done_callback exception
@angadsingh
Copy link
Author

IMO the fix is simple: #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

@Bibo-Joshi Bibo-Joshi mentioned this issue Jun 4, 2021
Closed
@starry-shivam starry-shivam mentioned this issue Jun 5, 2021
Merged
@github-actions github-actions bot locked and limited conversation to collaborators Jun 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v13.6
Development

Successfully merging a pull request may close this issue.

Don't call done_callback if Promise raises an exception. python-telegram-bot/python-telegram-bot
3 participants
@angadsingh @Bibo-Joshi @starry-shivam