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 ..

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 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.

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

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

hanging tests apparently were already fixed by 2373596 🕺