Skip to content

Refactor MessageQueue #2139

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
Bibo-Joshi opened this issue Oct 12, 2020 · 5 comments
Closed

Refactor MessageQueue #2139

Bibo-Joshi opened this issue Oct 12, 2020 · 5 comments
Labels
🛠 refactor change type: refactor
Milestone

Comments

@Bibo-Joshi
Copy link
Member

Bibo-Joshi commented Oct 12, 2020

TL;DR:

Known bugs are

Long version:

Using MessageQueue currently requires quite some manual work and there also the aformentioned bugs. Time to live up to the wikis statement

We plan to tightly couple it with telegram.Bot so that it could be used more conveniently (and even implicitly unless other specified).

So here is what my idea for a refactoring is. I'd be happy for feedback before getting started :)

Refactoring of MessageQueue and DelayQueue

  • DQ.exc_route: I'd like to rename to error_handler in accordance with Dispatcher.add_error_handler.
  • If a Dispatcher is used, it would make sense to have the execution of the promises to behave like the refactored Dipsatcher._pooled, which also includes proper error handling. That can be easily achieved by passing the Dispatcher to either the Bot or the MessageQueue. Not sure yet, what makes more sense …
  • I think I'd like to repace the __call__ methods with process or whatever. That's more explicit and easier to read in the code.
  • Add MQ.add/remove_delay_queue so that users can add custom delay queues in addition to the default & the group one. One should be able to specify a parent (defaulting to the default DQ) for new_dq, such that everything going throguh new_dq will also go through the parent. MQ.add_remove_delay_queue sohuld also start/stop the DQ in case we're adding them at runtime. Edit: This will in fact close Multiple message queues for a bot #1369 , as I just notice
  • Add constants for the default & the group DQ
  • Handle the stopping of MQ correctly, some hints are already given in [BUG] Bot hangs when using ctrl + c and MessageQueue #2012

Integration into Bot

Add a MessageQueue to Bot by default and rebuild all methods in the fashion

def badly_sketched_messagequeue_wrapper(func, *args, **kwargs):
    delay_queue = kwargs.pop('delay_queue')
    if delay_queue == self.message_queue.DEFAULT_DQ and int(kwargs.get('chat_id')) < 0: 
        return self.message_queue.process(func, self.message_queue.GROUP_DQ)
    elif delay_queue:
        return self.message_queue.process(func, delay_queue)
    return func(*args, **kwargs)

@badly_sketched_messagequeue_wrapper
def send_message(…, delay_queue=None):
    ...

such that delay_queue allows to select the delay queue the method is passed through, replacing the current queue and isgroup kwargs

def badly_sketched_messagequeue_wrapper(func, *args, **kwargs):
    if self.message_queue.is_running:
        # see above
    return func(*args, **kwargs)

so users can easily opt-into MessageQueue with bot.mq.start/stop()

Integration with Defaults

Add Defaults.delay_queue defaulting to None (I'm not in favor of implicitly always using the MessageQueue)

Handling context managers

I think the approach detailed in #2035 is feasible (build InputFiles already in Promise.__init__). I can't think of any use case, where a context manager would be used for anything else that we'd to handle …

Edit: Now I can. See #2160

Backwards compatibility

All of above changes should be doable without breaking changes.
Hower, given that MessageQueue is essentially still in the same quite rough state as when introduced in #537 3.5 years ago, IMHO it would be okay to removethe current usage pattern (i.e. the @queuedmessage decorator) in the next major release (which probably will be mostly a "delete all the stuff" release anyway …)

@Bibo-Joshi Bibo-Joshi mentioned this issue Oct 25, 2020
8 tasks
@Bibo-Joshi Bibo-Joshi added the 🛠 refactor change type: refactor label Nov 26, 2020
@Bibo-Joshi
Copy link
Member Author

Bibo-Joshi commented Jan 2, 2021

After some internal discussion, We'll postpone this to v14 (or later minor releases) when we switch to asyncio. This will allow us to make the bot methods use something like return Message.de_json(await self._post(…)) instead of returning a future/promise. There is also the idea of moving the delay logic to utils.Request which would allow for a higher level of customization. Still many ideas above still apply.

PS: I thinks it's important to allow users to customize the delay logic both on a global level (see #1369) and on a per-call level (see #2265)

@jagub2
Copy link

jagub2 commented Feb 23, 2021

I updated this Python module and got deprecation warning. Sure, I know I should fix this on my own. However, looks like examples on wiki are out of date: https://github.com/python-telegram-bot/python-telegram-bot/wiki/Avoiding-flood-limits#using-mq-with-queuedmessage-decorator.

@Bibo-Joshi
Copy link
Member Author

Bibo-Joshi commented Feb 23, 2021

The wiki also comes with two big notes about the deprecation ;)

Also you are not expected to fix anything an your own - the deprecation notes simply state that this very issue here lists known bugs (see the very first line) and you should be aware of them if you use MessageQueue ;)

@jagub2
Copy link

jagub2 commented Feb 23, 2021

Whoops, I went "tl;dr" too much. I just checked if the example changed. Got it, thank you.

@Bibo-Joshi
Copy link
Member Author

closing in favor of #3018

@github-actions github-actions bot locked and limited conversation to collaborators May 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🛠 refactor change type: refactor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants