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

tornado 5 fixes in ThreadedClient #352

Merged
merged 2 commits into from Mar 9, 2018

Conversation

@minrk
Copy link
Member

minrk commented Mar 8, 2018

The main issue was failing to schedule ioloop.stop in the ioloop thread, which is required when tornado is running on asyncio in order to wake the thread. The result of not doing this is hanging forever when trying to exit, e.g. in QtConsole.

There is further cleanup of threadsafety issues with respect to asyncio and tornado objects.

closes jupyter/qtconsole#275

cc @ccordoba12

minrk added 2 commits Mar 8, 2018
calling stop doesn’t wake the IOLoop with asyncio (tornado 5)
- avoid instantiating an IOLoop outside the thread in which it will be used,
  which sometimes causes problems.
- ensure asyncio eventloop is defined in the thread, if asyncio might be in use
# tornado may be using asyncio,
# ensure an eventloop exists for this thread
import asyncio
asyncio.set_event_loop(asyncio.new_event_loop())

This comment has been minimized.

Copy link
@takluyver

takluyver Mar 8, 2018

Member

It seems odd that we have to do this. Is there a neater way that we can use in the future when we can assume that tornado is using asyncio?

This comment has been minimized.

Copy link
@minrk

minrk Mar 9, 2018

Author Member

The situation: asyncio will not create an eventloop in a thread, you have to tell it to, at least in certain circumstances. Tornado is running on top of asyncio, but won't create the required asyncio eventloop if there isn't one (asyncio.get_event_loop will fail in threads that haven't initialized eventloops explicitly). I'm not sure if this is a tornado bug or not.

We can ask if the tornado configured IOLoop class is a subclass of AsyncIOLoop, which is the tornado asyncio wrapper implementation. If we assume that's going to be the case and not some other weird requires-asyncio implementation that doesn't subclass it.

Note that this is just instantiating an object, not starting it or anything, so it's a pretty minimal operation and harmless if the thread-local eventloop goes unused. I'm not quite sure what the cleanest solution is for "create the asyncio eventloop that tornado may need in this thread only if tornado is actually going to need it", because we would need more detailed try-except for importing tornado.platform.asyncio.AsyncIOLoop which may not be defined (e.g. python 2 or older tornado) in order to call issubclass().

This comment has been minimized.

Copy link
@vidartf

vidartf Mar 9, 2018

You could consider setting our own policy, so that we handle the calls to get_event_loop.

This comment has been minimized.

Copy link
@minrk

minrk Mar 11, 2018

Author Member

We shouldn't set our own policy in jupyter_client because that would preclude applications setting their own policies. We could set our own in qtconsoleapp, but can't rely on that in the qtconsole kernelmanager, which can be used in other applications like Spyder.

@ccordoba12

This comment has been minimized.

Copy link
Contributor

ccordoba12 commented Mar 8, 2018

I tested this against qtconsole and Spyder and it works as expected. Thanks @minrk!

@jakirkham jakirkham referenced this pull request Mar 8, 2018
Copy link
Member

willingc left a comment

I'm going to go ahead and merge this. We can refactor the eventloop with Tornado in a future PR if a cleaner approach surfaces. In the meanwhile, let's at least get the fix into master 👍

@willingc willingc merged commit 4bfb5c4 into jupyter:master Mar 9, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@stonebig

This comment has been minimized.

Copy link
Contributor

stonebig commented Mar 10, 2018

will there be a patch release soon so everybody can enjoy Tornado5 ?

@minrk minrk deleted the minrk:threadsafe-shutdown branch Mar 11, 2018
@minrk minrk added this to the 5.2.3 milestone Mar 11, 2018
minrk added a commit that referenced this pull request Mar 11, 2018
The main issue was failing to schedule `ioloop.stop` in the ioloop thread, which is required when tornado is running on asyncio in order to wake the thread. The result of not doing this is hanging forever when trying to exit, e.g. in QtConsole.

There is further cleanup of threadsafety issues with respect to asyncio and tornado objects.

closes jupyter/qtconsole#275

cc  ccordoba12

Signed-off-by: Min RK <benjaminrk@gmail.com>
@minrk

This comment has been minimized.

Copy link
Member Author

minrk commented Mar 11, 2018

@stonebig yes, hopefully tomorrow. I'll open a PR with a changelog for 5.2.3 and it should be out shortly.

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