Skip to content

proposed fix for #2542: proper treatment of pooled_function exception… #2543

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.

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

Closed
wants to merge 1 commit into from

Conversation

angadsingh
Copy link

@angadsingh angadsingh commented Jun 4, 2021

send the pooled_function exception back through done_callback instead of eating it up, mistreating it as a done_callback exception

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

hey Glad to see that you are eager to fix this. However, your change is immediately breaking (especially for ConversationHandler), because you pass an unexpected argument to the done_callbacks.

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

You can also just add proper error handling directly in your pooled_function. Promise is used two places:

  • MessageQueue - deprecated anyway, so no use to add new features
  • run_async, including ConversationHandler - here callbacks get error handling via dispatchers error handlers

Moreover, done_callback is only used in ConversationHandler and we don't need additional error handling there.

If you want to, you are very welcome to update your PR to the approach detailed in #2542 (comment) :)

@Bibo-Joshi
Copy link
Member

superseded by #2544. Nevertheless, thanks for taking the time to PR :)

@Bibo-Joshi Bibo-Joshi closed this Jun 5, 2021
@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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants