prompt-toolkit / python-prompt-toolkit

Library for building powerful interactive command line applications in Python
https://python-prompt-toolkit.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
9.11k stars 717 forks source link

assertion in Completion constructor doesn't propagate to prompt() #797

Open meeuw opened 5 years ago

meeuw commented 5 years ago

It seems that the assertion at https://github.com/prompt-toolkit/python-prompt-toolkit/blob/7db93560cb1f10c96f4a22816774da63e67e467e/prompt_toolkit/completion/base.py#L37 is handled at https://github.com/prompt-toolkit/python-prompt-toolkit/blob/7db93560cb1f10c96f4a22816774da63e67e467e/prompt_toolkit/eventloop/coroutine.py#L103 but that assertion doesn't propagate back to prompt() and seems to be silently discarded (at least in mycli and pgcli) ...

This can be reproduced by adding assert False at the first line of the Completion constructor and running prompt(), this seems to do nothing and I'd expect prompt() to raise an exception.

jonathanslenders commented 5 years ago

Thanks for pointing this out, @meeuw. This could be a bug indeed, but it's tricky.

In a coroutine, the exceptions are propagated using futures, but if there's no handle for the future, it should propagate further. (I guess this is one of the reasons that the Trio library avoids using futures).

The problem with futures is that the exception handler can be attached after the exception is raised, and that's completely valid. In that case it's a handled exception. Asyncio uses a custom __del__ method on the future object to handle uncaught exceptions. Prompt_toolkit copies this behavior.

See: https://github.com/prompt-toolkit/python-prompt-toolkit/blob/7db93560cb1f10c96f4a22816774da63e67e467e/prompt_toolkit/eventloop/future.py#L47

For debugging, it can also help to uncomment the last few lines in here: https://github.com/prompt-toolkit/python-prompt-toolkit/blob/7db93560cb1f10c96f4a22816774da63e67e467e/prompt_toolkit/eventloop/future.py#L103

So, the fix is probably not in the coroutine implementation, but probably we're keeping somewhere a reference around to a future where we shouldn't.