Skip to content

Conversation

@Bibo-Joshi
Copy link
Member

See https://t.me/pythontelegrambotgroup/595029?thread=595002 for discussion. The problem is that _update_state doesn't do anything if the new state is None (because return None means "stay in the current state") so that _conversations.get(key) returns the PendingState.

Checklist for PRs

  • Added .. versionadded:: version, .. versionchanged:: version or .. deprecated:: 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
  • Added myself alphabetically to AUTHORS.rst (optional)
  • Added new classes & modules to the docs and all suitable __all__ s

@Bibo-Joshi Bibo-Joshi added this to the v20.0a1 milestone May 26, 2022
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.

good catch by the OP

@CedricFauth
Copy link

I tested out the current release 20.0a0. Every non-blocking callback inside state or fallback that returns None has that issue not only entry points I think. Maybe that is already fixed somewhere else. Currently, when I am returning None in any state I get the PendingState... error.

@Bibo-Joshi
Copy link
Member Author

@CedricFauth thanks for the info! I've pushed more changes that should address that. Could you check if those fix the problems for you? You can install from this branch via pip install git+https://github.com/python-telegram-bot/python-telegram-bot.git@ch-entry-point-bug.

@CedricFauth
Copy link

@Bibo-Joshi Yes but no :/ You fixed my previous problem. Returning None now works inside the conversation handler but a new problem occurred:
I am unable to leave a conversation (return ConversationHandler.END). The next time I send a message to the bot (after ending a conversation) it says: Selecting conversation (..., ...) with state -1 (which is the END state so the bot should have left the conversation before but does not). This ends in my fallback handler getting called every time I send a message. The user is not able to do anything because the conversation stays in state -1 forever.
This is only the case when using ConversationHandler(block=False) and the current branch ch-enrty-point-bug. In the pre-release it never happened.
Hope I could help you find the problem.

@Bibo-Joshi
Copy link
Member Author

Yeah, I should have properly run the tests before pushing 🤦‍♂️ hopefully it's now better :)

@CedricFauth
Copy link

Seems to work now, thanks!

@Bibo-Joshi Bibo-Joshi requested a review from harshil21 May 30, 2022 05:51
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.

So I was testing all combinations of block=False/True of ConversationHandler with the handlers in entry_points, states and fallbacks, and noticed that when ConvHandler(..., block=False), the exception thrown from one entry point isn't discarded correctly, and when another entry point is run, the same exception is printed again.

MRE: https://nekobin.com/modoxayexo

Steps to reproduce:

  1. Run bot, send /start. You'll see in start printed along with the exception.
  2. Now send /start2. You'll see the same exception printed again, and then in start2 is printed to console.
  3. State is correctly switched to GENDER though.

Expected behavior:
In step 2, the exception should not be printed again. This is indeed the behaviour when ConvHandler(..., block=True).

@Bibo-Joshi
Copy link
Member Author

State is correctly switched to GENDER though.

Just to double check: your observation is only about error prints & not about wrong behavior right?

The difference is the heading of the traceback: The first (triggered by /start) will say No error handlers are registered, logging exception.. If you have a custom error handler, this one won't show (unless your error handler explicitly logs it again).
The second one (striggered by the next update) will say Task function raised exception. Falling back to old state None, which is logged by PendingState.resolve(). IMO those two long entries serve different purposes: The first informing you about an error in the callback & the second one about what's going on state-wise in the ConversationHandler. the traceback being printed twice may surely be seen as a bit overly verbose in the default case, though I guess it's not really harmful either.

@harshil21
Copy link
Member

Just to double check: your observation is only about error prints & not about wrong behavior right?

Yes. It confused me, thinking it was running the start callback again somehow, and will confuse others too, eventually

But why should we re-print the error message when it's already printed/forwarded to error handler the first time? Maybe we should move the "Task function raised exception. Falling back to old state %s" to when the error actually happens the first time (is this possible to do?)

@Bibo-Joshi Bibo-Joshi requested a review from harshil21 June 3, 2022 20:33
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.

Did some more testing, and everything seems to run fine now

@Bibo-Joshi Bibo-Joshi merged commit 22419c0 into master Jun 7, 2022
@Bibo-Joshi Bibo-Joshi deleted the ch-enrty-point-bug branch June 7, 2022 15:48
@github-actions github-actions bot locked and limited conversation to collaborators Jun 14, 2022
@Bibo-Joshi Bibo-Joshi added 🔌 bug pr description: bug and removed bug 🐛 labels Nov 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

🔌 bug pr description: bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants