Skip to content
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

Refactor @run_async #2051

Merged
merged 18 commits into from
Oct 4, 2020
Merged

Refactor @run_async #2051

merged 18 commits into from
Oct 4, 2020

Conversation

Bibo-Joshi
Copy link
Member

  • Deprecates the @run_async decorator
  • Instead adds kwargs update and error_handler to DP.run_async() to allow proper error handling
  • Allows to specify that a callback should be run asynchronously via DP.add_handler/DP.add_error_handler

One problem: In the new setting it's currently not possible to make the handlers of a ConversationHandler be run asynchronously. Possible solutions:

  • When adding a CH via DP.add_handler(CH, run_asyn=True) make all handlers of the Conversation async
  • Set the run_async flag as parameter for Handler instead of as parameter for DP.add_handler
  • Somehow build an option for that into CH

Wanted to have that clarified before going for the tests, so WIP.

When ready, closes #682 and superseeds/closes #785

@Bibo-Joshi Bibo-Joshi added this to the 13.0 milestone Aug 18, 2020
@Bibo-Joshi Bibo-Joshi marked this pull request as ready for review August 22, 2020 21:58
# Conflicts:
#	telegram/ext/dispatcher.py
#	telegram/ext/messagehandler.py
#	telegram/ext/regexhandler.py
@Bibo-Joshi
Copy link
Member Author

Bibo-Joshi commented Aug 26, 2020

I made two additional changes to Dispatcher.update_persistence:

  1. Use a lock to ensure that the method is not called twice simultanuously
  2. convert dispatcher.user/chat_data.keys() to lists to prevent errors like this. That particular one was probably caused by JobQueue calling Dispatcher.update_persistence while an entry was added to dispatcher.user_data, but similar situations can probably also arise when calling Dispatcher.update_persistence from the async threads. We don't acutally have to worry about the new entry not being persisted, as update_persistence() will be called for the update that lead to the new entry anyway.

@GauthamramRavichandran
Copy link
Contributor

Could this async be added to job callback as well? I'm using @run_async decorator to run jobs in parallel for now

Copy link
Member Author

@GauthamramRavichandran Please have a look at this comment :)

telegram/ext/callbackqueryhandler.py Outdated Show resolved Hide resolved
telegram/ext/callbackqueryhandler.py Outdated Show resolved Hide resolved
telegram/utils/promise.py Outdated Show resolved Hide resolved
telegram/utils/promise.py Show resolved Hide resolved
telegram/ext/dispatcher.py Outdated Show resolved Hide resolved
telegram/ext/dispatcher.py Outdated Show resolved Hide resolved
telegram/ext/dispatcher.py Outdated Show resolved Hide resolved
telegram/ext/dispatcher.py Show resolved Hide resolved
telegram/ext/dispatcher.py Outdated Show resolved Hide resolved
telegram/ext/dispatcher.py Show resolved Hide resolved
telegram/ext/dispatcher.py Outdated Show resolved Hide resolved
telegram/ext/dispatcher.py Outdated Show resolved Hide resolved
telegram/ext/dispatcher.py Outdated Show resolved Hide resolved
telegram/ext/dispatcher.py Show resolved Hide resolved
telegram/ext/commandhandler.py Show resolved Hide resolved
telegram/ext/dispatcher.py Show resolved Hide resolved
@Bibo-Joshi
Copy link
Member Author

Bibo-Joshi commented Sep 21, 2020

As discussed, I restructured a bit. Let me try to summarize:

  • The error_handler parameter for run_async was basically there to avoid infinite loops coming from asynchronously run functions error handlers
  • instead, now there is just a parameter async_error_handling. When False, no async error_handler will be called*
  • but we don't even handle sync errors raised by error handlers. So now there is just a parameter error_handling for run_async. When False, the error will just be logged.
  • We only have one set of error handlers, namely those registered with add_error_handler
  • new parameter async_params for CallbackContext so that the params for custom async functions can be accessed from the error handler

telegram/ext/callbackcontext.py Outdated Show resolved Hide resolved
telegram/ext/callbackcontext.py Outdated Show resolved Hide resolved
telegram/ext/callbackcontext.py Outdated Show resolved Hide resolved
telegram/ext/callbackcontext.py Outdated Show resolved Hide resolved
telegram/ext/dispatcher.py Outdated Show resolved Hide resolved
telegram/ext/dispatcher.py Outdated Show resolved Hide resolved
telegram/ext/dispatcher.py Outdated Show resolved Hide resolved
telegram/ext/dispatcher.py Outdated Show resolved Hide resolved
telegram/ext/dispatcher.py Outdated Show resolved Hide resolved
telegram/ext/dispatcher.py Outdated Show resolved Hide resolved
telegram/ext/dispatcher.py Show resolved Hide resolved
telegram/ext/dispatcher.py Outdated Show resolved Hide resolved
telegram/ext/dispatcher.py Outdated Show resolved Hide resolved
telegram/ext/dispatcher.py Outdated Show resolved Hide resolved
telegram/ext/dispatcher.py Outdated Show resolved Hide resolved
Copy link
Member

@tsnoam tsnoam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work

@Bibo-Joshi Bibo-Joshi merged commit 0d419ed into master Oct 4, 2020
@Bibo-Joshi Bibo-Joshi deleted the refactor-run-async branch October 4, 2020 15:20
@github-actions github-actions bot locked and limited conversation to collaborators Oct 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error handlers not working with @run_async
3 participants