8000 Ensure Safe Handler Looping in `Application.process_update/error` by Bibo-Joshi · Pull Request #4802 · python-telegram-bot/python-telegram-bot · GitHub
[go: up one dir, main page]
Skip to content

Ensure Safe Handler Looping in Application.process_update/error #4802

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
Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Ensure Safe Handler Looping in Application.process_update/error #4802

wants to merge 10 commits into from

Uh oh!

There was an error while loading. Please reload this page.

Conversation

Bibo-Joshi
Copy link
Member
@Bibo-Joshi Bibo-Joshi commented May 25, 2025 •
edited by aelkheir
Loading

Based on user input https://t.me/pythontelegrambotgroup/779167

Closes #4803

No error handlers are registered, logging exception.

Traceback (most recent call last):

  File "/home/kuzunohalit/.local/lib/python3.12/site-packages/telegram/ext/_application.py", line 1170, in _create_task_callback
    return await coroutine  # type: ignore[misc]

  File "/home/kuzunohalit/.local/lib/python3.12/site-packages/telegram/ext/_application.py", line 1232, in _process_update_wrapper
    await self._update_processor.process_update(update, self.process_update(update))

  File "/home/kuzunohalit/.local/lib/python3.12/site-packages/telegram/ext/_baseupdateprocessor.py", line 170, in process_update
    await self.do_process_update(update, coroutine)

  File "/home/kuzunohalit/.local/lib/python3.12/site-packages/telegram/ext/_baseupdateprocessor.py", line 195, in do_process_update
    await coroutine

  File "/home/kuzunohalit/.local/lib/python3.12/site-packages/telegram/ext/_application.py", line 1259, in process_update
    for handlers in self.handlers.values():
RuntimeError: dictionary changed size during iteration

@Bibo-Joshi Bibo-Joshi requested review from aelkheir and harshil21 May 25, 2025 18:13
@Bibo-Joshi Bibo-Joshi added the 🔌 bug pr description: bug label May 25, 2025
aelkheir
aelkheir reviewed May 25, 2025
View reviewed changes
aelkheir
aelkheir approved these changes May 25, 2025
View reviewed changes
Copy link
Member
@aelkheir aelkheir left a comment

Choose a reason for hiding this comment

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

LGTM

At first I thought the exception being raised is reasonable, because i'm not sure of a use case where one would need to add handlers dynamically.

And the fact that it was not working, means that PTB users are also just discovering these use cases.

Maybe bot builders (?) where users of the bot add custom command/messages handers base on their input, also then how can the callback part be dynamic.

But that's for users to answer. We can have it.

Just one thought considering how this should work, for now if one dynamically adds a handler in a different group than the one currently selected, it will not be considered for the same update currently being processed, as the list of handlers was copied long before.

would that be a normal/abnormal behavior?

@aelkheir aelkheir mentioned this pull request May 26, 2025
Open
@Bibo-Joshi
Copy link
Member Author

LGTM

At first I thought the exception being raised is reasonable, because i'm not sure of a use case where one would need to add handlers dynamically.

And the fact that it was not working, means that PTB users are also just discovering these use cases.

Maybe bot builders (?) where users of the bot add custom command/messages handers base on their input, also then how can the callback part be dynamic.

But that's for users to answer. We can have it.

At least I'd say since we're offering add/remove_handler at runtime, it should not throw an exception, independently on how exactly that's achieved :D

Just one thought considering how this should work, for now if one dynamically adds a handler in a different group than the one currently selected, it will not be considered for the same update currently being processed, as the list of handlers was copied long before.

would that be a normal/abnormal behavior?

Yes, I'd say that's expected. I think there are a few different options one could choosev

  • No adding/removing handlers at runtime -> would be breaking
  • Apply a lock around the handlers -> would maybe be an option if we starting from scratch. The lock would be an asyncio.Lock making add/remove_handler coroutine functions. That would be a breaking change. Also it's unclear to me if that would be a desirable interface at all
  • Simply ensure safe looping as here. I think it's reasonable to argue that once an update has started being processed, changes to the logic of how updates are processed should not affect it. And as you've pointed out above, this appwars to be an edge case. I'll still add some hints in the docs

Also I've realized that we need the same logic in

https://github.com/python-telegram-bot/python-telegram-bot/blob/master/src%2Ftelegram%2Fext%2F_application.py#L1261

where we loop over the handler list within the group ..

Bibo-Joshi added 2 commits May 26, 2025 18:31
@Bibo-Joshi
Link closed issue in chango fragment
8f349df
@Bibo-Joshi
Extend scope of PR
9161e9a 
* cover also process-error
* cover non-critical list-loops
* document behavior as implementation detail
@Bibo-Joshi Bibo-Joshi changed the title Ensure Safe Handler Looping in Application.process_update Ensure Safe Handler Looping in Application.process_update/error May 26, 2025
@Bibo-Joshi
Copy link
Member Author

Explanation of additional changes:

  • For consistency, I also ensured that changing handlers within a group is discarded. Note that this currently does not raise an exception as changing a list while iterating over it is safe. However, we don't actually document how the effect on process_update is, so that IMO it's okay to see this as non-breaking.
  • changing error_handlers is now safe similar to the handlers change
  • I've added 10000 documentation hints stating that this is the current behavior but should be considered an implementation detail. This is mostly based on a gut feeling that making this documented behavior may restrict us from refactoring the Application internals in the future. If you think it's too cautios I'm open to removing that hint.

@aelkheir
Copy link
Member

it's reasonable to argue that once an update has started being processed, changes to the logic of how updates are processed should not affect it.

👍

If you think it's too cautios I'm open to removing that hint.

the hint+warning are sensible imo

@Bibo-Joshi
Copy link
Member Author

mh, apparently some test case is in a deadlock now 😵‍💫 will have to investigate. not today, though

@Bibo-Joshi
Copy link
Member Author

hanging tests apparently were already fixed by 2373596 🕺

@Bibo-Joshi Bibo-Joshi requested a review from aelkheir May 28, 2025 20:02
aelkheir
aelkheir reviewed May 28, 2025
View reviewed changes
Copy link
Member
@aelkheir aelkheir left a comment

Choose a reason for hiding this comment

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

Just one minor comment.

hanging tests apparently were already fixed by 2373596

that's weird cause checks were hanging for me too in #4750. maybe #4801 fixed it (?) i'll merge and see.

Bibo-Joshi and others added 2 commits May 29, 2025 00:13
@Bibo-Joshi @aelkheir
update comment
19903e0 
Co-authored-by: Abdelrahman Elkheir <90580077+aelkheir@users.noreply.github.com>
@Bibo-Joshi
Elaborate chango fragment and fix docstring rendering
1dffd82
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aelkheir aelkheir aelkheir approved these changes

@harshil21 harshil21 Awaiting requested review from harshil21

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

Successfully merging this pull request may close these issues.

Error With Adding Handler
2 participants
@Bibo-Joshi @aelkheir
0