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

fix %gui asyncio with tornado 5 #296

Merged
merged 3 commits into from
Jan 16, 2018
Merged

fix %gui asyncio with tornado 5 #296

merged 3 commits into from
Jan 16, 2018

Conversation

minrk
Copy link
Member

@minrk minrk commented Jan 16, 2018

Fixes test failures when tornado runs on asyncio (tornado 5 + Python ≥3.5).

rather than calling enter_eventloop again when the eventloop is disabled (eventloop = None)
ensures signal handlers are properly restored after exiting eventloop
@minrk
Copy link
Member Author

minrk commented Jan 16, 2018

The main issue appears to be related to the fact that asyncio is already running with tornado 5, so the eventloop integration with %gui asyncio is a no-op. There's something we aren't doing right in that case.

@minrk
Copy link
Member Author

minrk commented Jan 16, 2018

I think I got it. We were registering the default interrupt handler when we enter a custom eventloop, but not when we exit. The result was that our interrupt handler would be unregistered, so interrupting an idle kernel after exiting an eventloop. Running the same pre/post hooks around eventlloop that we use on regular handlers should ensure the right signal handlers are registered appears to fix the issue.

This has been a longstanding bug, but as a bit of a coincidence we just started testing this particular circumstance in #281.

@minrk minrk changed the title [WIP] tornado 5 fix %gui asyncio with tornado 5 Jan 16, 2018
@takluyver takluyver added this to the 4.8 milestone Jan 16, 2018
@takluyver takluyver merged commit 479932d into ipython:master Jan 16, 2018
@minrk minrk deleted the tornado-5 branch March 14, 2018 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants