Skip to content
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.

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

assertion failure in add_signal_handler with prompt-toolkit #50

Open
faassen opened this issue Jan 19, 2019 · 4 comments

Comments

@faassen
Copy link

commented Jan 19, 2019

I'm trying to make prompt-toolkit (which has asyncio support) work with trio through this library. I don't actually know what I'm doing. :)

I had a persistent issue due to an assertion in trio-asyncio

assert sig not in self._signal_handlers, \ "Signal %d is already being caught" % (sig,)

My hypothesis is that prompt-toolkit in async mode assumes it can call this multiple times, but that trio-asyncio assumes you can't. Which one is right I cannot judge. If it's prompt-toolkit, I can try to communicate that to them.

I have an evil workaround where I write this in my application:

del signal.SIGWINCH

Because I noticed it was going wrong when prompt-toolkit detected SIGWINCH exists. But of course that's not a proper solution (and I just now lack resize support).

@faassen

This comment has been minimized.

Copy link
Author

commented Jan 19, 2019

Here's some code that reproduces the issue. You need prompt_toolkit installed of course. If you enable the signal hack the code appears to work.

import asyncio
import trio_asyncio
from trio_asyncio import trio2aio
from prompt_toolkit import print_formatted_text, prompt as _prompt
from prompt_toolkit.eventloop.defaults import use_asyncio_event_loop

# workaround as otherwise prompt toolkit wants to re-add signal handler
# which trio_asyncio doesn't like because of an assertion
# this means the application can't respond to resizes anymore...

# import signal
# del signal.SIGWINCH


@trio2aio
async def prompt(*args, **kw):
    asyncio.get_event_loop().remove_signal_handler(28)
    return await _prompt(*args, async_=True, **kw)


async def async_main():
    use_asyncio_event_loop()
    text = await prompt("Give me input please: ")
    print_formatted_text(f"Hello world, your input was {text}")


trio_asyncio.run(async_main)
@njsmith

This comment has been minimized.

Copy link
Member

commented Jan 21, 2019

As a rule of thumb, if it works on asyncio, but doesn't work on trio-asyncio, then it's probably a bug in trio-asyncio. (No matter what asyncio is "supposed" to do.) So, this is probably a bug in trio-asyncio!

It looks like it should be pretty straightforward to fix... right now trio-asyncio's add_signal_handler does:

        assert sig not in self._signal_handlers, \
            "Signal %d is already being caught" % (sig,)
        self._orig_signals[sig] = signal.signal(sig, self._handle_sig)
        self._signal_handlers[sig] = h

I think it should work to replace those lines of code with:

        if sig not in self._signal_handlers:
            self._orig_signals[sig] = signal.signal(sig, self._handle_sig)
        self._signal_handlers[sig] = h

Any interest in putting together a PR?

(@smurfix Do we need to add a call to self._signal_handlers[sig].cancel() as well? AFAICT this shouldn't do anything, but remove_signal_handler does call it...)

@faassen

This comment has been minimized.

Copy link
Author

commented Jan 21, 2019

I can take a look at it later, but I don't guarantee I won't get seriously lost. :)

@njsmith

This comment has been minimized.

Copy link
Member

commented Jan 21, 2019

That we can help with! :-) Feel free to ask in the issue or here...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.