8000 Add `Application.stop_running` and Improve Marking Updates as Read on `Updater.stop` by Bibo-Joshi · Pull Request #3804 · python-telegram-bot/python-telegram-bot · GitHub
[go: up one dir, main page]
Skip to content

Add Application.stop_running and Improve Marking Updates as Read on Updater.stop #3804

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
Merged
merged 19 commits into from
Aug 17, 2023
Merged

Add Application.stop_running and Improve Marking Updates as Read on Updater.stop #3804

merged 19 commits into from
Aug 17, 2023

Conversation

Bibo-Joshi
Copy link
Member
@Bibo-Joshi Bibo-Joshi commented Jul 16, 2023 •
edited
Loading

WIP. Addresses #3718. Doesn't include a native way to restart the whole script, though, so this doesn't really close #3718.

The naming stop_running is up for discussion. May be too similar to Application.stop.

  • Added .. versionadded:: NEXT.VERSION, .. versioncha 8000 nged:: NEXT.VERSION or .. deprecated:: NEXT.VERSION to the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)
  • Created new or adapted existing unit tests
  • Documented code changes according to the CSI standard <https://standards.mousepawmedia.com/en/stable/csi.html>__
  • [ ] Added myself alphabetically to AUTHORS.rst (optional)
  • [ ] Added new classes & modules to the docs and all suitable __all__ s
  • Checked the Stability Policy <https://docs.python-telegram-bot.org/stability_policy.html>_ in case of deprecations or changes to documented behavior

@Bibo-Joshi Bibo-Joshi mentioned this pull request Jul 16, 2023
Closed
@Bibo-Joshi Bibo-Joshi marked this pull request as ready for review July 24, 2023 10:24
harshil21
harshil21 reviewed Aug 1, 2023
View reviewed changes
Copy link
Member
@harshil21 harshil21 left a comment

Choose a reason for hiding this comment

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

Just tested this solution. It works by shutting it down but there's something wrong, it doesn't clear the update queue? It processes the same Update everytime I manually restart the bot (and that Update triggers a handler which calls stop_running, so it basically stops the bot whenever I restart it). Here are the debug logs:

2023-08-01 17:47:24,223 - telegram.ext.ExtBot - DEBUG - Entering: get_me
2023-08-01 17:47:24,704 - telegram.ext.ExtBot - DEBUG - User(can_join_groups=True, can_read_all_group_messages=True, first_name='Testbed', id=1028236561, is_bot=True, supports_inline_queries=True, username='Ttessttingbot')
2023-08-01 17:47:24,705 - telegram.ext.ExtBot - DEBUG - Exiting: get_me
2023-08-01 17:47:24,705 - telegram.ext.ExtBot - DEBUG - This Bot is already initialized.
2023-08-01 17:47:24,706 - telegram.ext.Updater - DEBUG - Updater started (polling)
2023-08-01 17:47:24,706 - telegram.ext.Updater - DEBUG - Start network loop retry bootstrap del webhook
2023-08-01 17:47:24,706 - telegram.ext.Updater - DEBUG - Deleting webhook
2023-08-01 17:47:24,706 - telegram.ext.ExtBot - DEBUG - Entering: delete_webhook
2023-08-01 17:47:24,878 - telegram.ext.ExtBot - DEBUG - True
2023-08-01 17:47:24,878 - telegram.ext.ExtBot - DEBUG - Exiting: delete_webhook
2023-08-01 17:47:24,878 - telegram.ext.Updater - DEBUG - Bootstrap done
2023-08-01 17:47:24,878 - telegram.ext.Updater - DEBUG - Waiting for polling to start
2023-08-01 17:47:24,878 - telegram.ext.Updater - DEBUG - Polling updates from Telegram started
2023-08-01 17:47:24,879 - telegram.ext.Updater - DEBUG - Start network loop retry getting Updates
2023-08-01 17:47:24,879 - telegram.ext.ExtBot - DEBUG - Entering: get_updates
2023-08-01 17:47:24,881 - apscheduler.scheduler - INFO - Scheduler started
2023-08-01 17:47:24,881 - telegram.ext.Application - DEBUG - JobQueue started
2023-08-01 17:47:24,881 - telegram.ext.Application - INFO - Application started
2023-08-01 17:47:25,336 - telegram.ext.ExtBot - DEBUG - Getting updates: [967073272]
2023-08-01 17:47:25,337 - telegram.ext.ExtBot - DEBUG - (Update(message=Message(channel_chat_created=False, chat=Chat(first_name='Harshil', id=476269395, type=<ChatType.PRIVATE>, username='Hoppingturtles'), date=datetime.datetime(2023, 8, 1, 13, 47, 13, tzinfo=<UTC>), delete_chat_photo=False, from_user=User(first_name='Harshil', id=476269395, is_bot=False, is_premium=True, language_code='en', username='Hoppingturtles'), group_chat_created=False, message_id=5681, supergroup_chat_created=False, text='test1'), update_id=967073272),)
2023-08-01 17:47:25,340 - telegram.ext.ExtBot - DEBUG - Exiting: get_updates
2023-08-01 17:47:25,340 - telegram.ext.ExtBot - DEBUG - Entering: get_updates
2023-08-01 17:47:25,341 - telegram.ext.Application - DEBUG - Processing update Update(message=Message(channel_chat_created=False, chat=Chat(first_name='Harshil', id=476269395, type=<ChatType.PRIVATE>, username='Hoppingturtles'), date=datetime.datetime(2023, 8, 1, 13, 47, 13, tzinfo=<UTC>), delete_chat_photo=False, from_user=User(first_name='Harshil', id=476269395, is_bot=False, is_premium=True, language_code='en', username='Hoppingturtles'), group_chat_created=False, message_id=5681, supergroup_chat_created=False, text='test1'), update_id=967073272)
shutting down, test1
2023-08-01 17:47:25,343 - telegram.ext.Updater - DEBUG - Stopping Updater
2023-08-01 17:47:25,343 - telegram.ext.Updater - DEBUG - Waiting background polling task to finish up.
2023-08-01 17:47:25,344 - telegram.ext.Updater - DEBUG - Network loop retry getting Updates was cancelled
2023-08-01 17:47:25,344 - telegram.ext.Updater - DEBUG - Updater.stop() is complete
2023-08-01 17:47:25,344 - telegram.ext.Application - INFO - Application is stopping. This might take a moment.
2023-08-01 17:47:25,344 - telegram.ext.Application - DEBUG - Waiting for update_queue to join
2023-08-01 17:47:25,344 - telegram.ext.Application - DEBUG - Dropping pending updates
2023-08-01 17:47:25,345 - telegram.ext.Application - DEBUG - Application stopped fetching of updates.
2023-08-01 17:47:25,345 - telegram.ext.Application - DEBUG - Waiting for running jobs to finish
2023-08-01 17:47:25,345 - apscheduler.scheduler - INFO - Scheduler has been shut down
2023-08-01 17:47:25,356 - telegram.ext.Application - DEBUG - JobQueue stopped
2023-08-01 17:47:25,356 - telegram.ext.Application - DEBUG - Waiting for `create_task` calls to be processed
2023-08-01 17:47:25,356 - telegram.ext.Application - INFO - Application.stop() complete
2023-08-01 17:47:25,357 - telegram.ext.ExtBot - DEBUG - This Bot is already shut down. Returning.
2023-08-01 17:47:25,358 - telegram.ext.Updater - DEBUG - Shut down of Updater complete
shut down

@Bibo-Joshi
Copy link
Member Author

My guess would be that the Updater is closed "too fast" such that get_updates is not called one more time with that updates id as offset. That way TG doesn't know that we already processed it and on the next call to get_updates (after startup), it will again provide the update. Can you try what happens if you add await self.bot.get_updates(offset=self._last_update_id) at the very end of Updater._stop_polling?

@harshil21
Copy link
Member

Can you try what happens if you add await self.bot.get_updates(offset=self._last_update_id) at the very end of Updater._stop_polling?

Yeah, doing this fixes the problem. It returns an empty tuple.

@Bibo-Joshi
Copy link
Member Author

Nice. Then I'll try to incoporate that. I guess that's even a more general problem and here it just shows more prominentely 🤔

@Bibo-Joshi Bibo-Joshi changed the title stop-from-callback Add Application.stop_running and Improve Marking Updates as Read on Updater.stop Aug 2, 2023
@Bibo-Joshi Bibo-Joshi requested a review from harshil21 August 2, 2023 09:52
harshil21
harshil21 approved these changes Aug 4, 2023
View reviewed changes
Copy link
Member
@harshil21 harshil21 left a comment

Choose a reason for hiding this comment

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

alright, changes look good, it works great now. Did we not "mark updates as read" prior to this at all? Shutdown now takes twice as long for me.

Also one more thing - when I press ctrl + C on the console, I expect the "Application is stopping. This might take a moment." log msg to pop up instantly, however that is only printed after shutdown.

@Bibo-Joshi
Copy link
Member Author

Did we not "mark updates as read" prior to this at all?

Apparently not on shutdown. The continuous calling of get_updates(…, offset=self._last_update_id) marks the updates as read and apparently the usual shutdown so rarely causes updates going unmarked that it didn't surface so far.

Shutdown now takes twice as long for me.

Fixed with the latest commit

Also one more thing - when I press ctrl + C on the console, I expect the "Application is stopping. This might take a moment." log msg to pop up instantly, however that is only printed after shutdown.

This is the log output for me:

2023-08-05 22:10:59,008 - telegram.ext.Updater - DEBUG - Stopping Updater
2023-08-05 22:10:59,008 - telegram.ext.Updater - DEBUG - Waiting background polling task to finish up.
2023-08-05 22:10:59,018 - telegram.ext.Updater - DEBUG - Network loop retry getting Updates was cancelled
2023-08-05 22:10:59,018 - telegram.ext.Updater - DEBUG - Calling `get_updates` one more time to mark all fetched updates as read.
2023-08-05 22:10:59,018 - telegram.ext.ExtBot - DEBUG - Entering: get_updates
2023-08-05 22:10:59,084 - telegram.ext.ExtBot - DEBUG - No new updates found.
2023-08-05 22:10:59,100 - telegram.ext.ExtBot - DEBUG - ()
2023-08-05 22:10:59,100 - telegram.ext.ExtBot - DEBUG - Exiting: get_updates
2023-08-05 22:10:59,100 - telegram.ext.Updater - DEBUG - Updater.stop() is complete
2023-08-05 22:10:59,101 - telegram.ext.Application - INFO - Application is stopping. This might take a moment.
2023-08-05 22:10:59,101 - telegram.ext.Application - DEBUG - Waiting for update_queue to join
2023-08-05 22:10:59,101 - telegram.ext.Application - DEBUG - Dropping pending updates
2023-08-05 22:10:59,101 - telegram.ext.Application - DEBUG - Application stopped fetching of updates.
2023-08-05 22:10:59,102 - telegram.ext.Application - DEBUG - Waiting for running jobs to finish
2023-08-05 22:10:59,102 - apscheduler.scheduler - INFO - Scheduler has been shut down
2023-08-05 22:10:59,102 - telegram.ext.Application - DEBUG - JobQueue stopped
2023-08-05 22:10:59,102 - telegram.ext.Application - DEBUG - Waiting for `create_task` calls to be processed
2023-08-05 22:10:59,102 - telegram.ext.Application - INFO - Application.stop() complete
2023-08-05 22:10:59,105 - telegram.ext.ExtBot - DEBUG - This Bot is already shut down. Returning.
2023-08-05 22:10:59,105 - telegram.ext.Updater - DEBUG - Shut down of Updater complete

i.e. fist Update & App stop (in that order) and then Bot, Updater & app shut down. looks fine to me …

@Bibo-Joshi Bibo-Joshi requested a review from harshil21 August 9, 2023 19:41
@harshil21
Copy link
Member

Fixed with the latest commit

Yep, fine now.

i.e. fist Update & App stop (in that order) and then Bot, Updater & app shut down. looks fine to me …

I still notice a slight delay, which is happening because the log message is in application.stop(), but updater.stop() needs to complete first, and by calling get_updates(), the delay to run app.stop() increases. Can we move that logger message or maybe add another message "Stop signal received, shutting down Updater"?

harshil21
harshil21 approved these changes Aug 17, 2023
View reviewed changes
Copy link
Member
@harshil21 harshil21 left a comment

Choose a reason for hiding this comment

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

ok, lgtm
A05D

@Bibo-Joshi Bibo-Joshi merged commit 5128748 into master Aug 17, 2023
@Bibo-Joshi Bibo-Joshi deleted the stop-from-callback branch August 17, 2023 09:50
@github-actions github-actions bot locked and limited conversation to collaborators Aug 25, 2023
@Bibo-Joshi Bibo-Joshi added 🔌 enhancement pr description: enhancement and removed enhancement labels Nov 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@harshil21 harshil21 harshil21 approved these changes

@Poolitzer Poolitzer Awaiting requested review from Poolitzer

Assignees
No one assigned
Labels
🔌 enhancement pr description: enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Bibo-Joshi @harshil21
0