Use native coroutines instead of tornado coroutines #632
Conversation
SylvainCorlay
force-pushed
the
use-native-coroutines
branch
6 times, most recently
from
4c9ae9c to
3604092
Compare
SylvainCorlay
force-pushed
the
use-native-coroutines
branch
6 times, most recently
from
db887a1 to
1e7272b
Compare
minrk
left a comment
minrk
left a comment
There was a problem hiding this comment.
Makes sense. One minor suggestion that we preserve the maybe_future functionality to allow non-async methods, since most are in fact not async. That's probably the biggest breaking change here.
Since asyncio doesn't have its own maybe_future we can be more simplistic and just use:
result = maybe_async_api()
if inspect.isawaitable(result):
result = await resultThe main difference this has is that concurrent.futures.Future is not awaitable. Everything tornado returns is, though (>=5.0, at lest). I think that's okay, though.
SylvainCorlay
force-pushed
the
use-native-coroutines
branch
3 times, most recently
from
11f8217 to
971383d
Compare
|
I could fix the in-process kernel tests. |
|
Also, should I remove other uses of tornado in the code base in this PR? |
There was a problem hiding this comment.
should I remove other uses of tornado in the code base in this PR?
I think it's sensible to do it in stages, since this is a concrete discrete change.
It looks like the main thing left is our use of queues. However, asyncio's queue is not threadsafe, unlike tornado's, and the whole point of our use of the queues is inter-thread communication. So I think it makes sense to deal with that in its own PR, and keep this one to just Edit: tornado's queues are also not threadsafe. They are only called via async def.add_callback. But I can imagine there being subtle threadsafety issues, since we do use them across threads.
SylvainCorlay
force-pushed
the
use-native-coroutines
branch
from
971383d to
93a61cd
Compare
|
That looks great; with this, capturing of IO and some of the cleanup that has been done recently it would make a compelling release. We may even use the occasion to drop a couple deprecated things. |
👍 And debugger support! |
SylvainCorlay
force-pushed
the
use-native-coroutines
branch
3 times, most recently
from
a2bfc64 to
e6ff9ff
Compare
Tornado queues are not thread safe, but it does not mean you can't use them with threads. It only means you can't use their APIs from a different thread where the ioloop runs - except for add_callback which is documented as being threadsafe. |
minrk
left a comment
minrk
left a comment
There was a problem hiding this comment.
👍 in general. I'd only add that I think we shouldn't deprecate synchronous handlers, so I think we should skip the deprecation message, and shouldn't convert existing do_ methods and message handlers to async if they aren't already. There isn't really a benefit to that that I can see, and it does have a (tiny) performance cost.
SylvainCorlay
force-pushed
the
use-native-coroutines
branch
2 times, most recently
from
fb46301 to
583dc40
Compare
|
Kicking CI |
SylvainCorlay
force-pushed
the
use-native-coroutines
branch
from
7b01b09 to
4965355
Compare
|
Nice! |
|
Many thanks to @minrk for the careful review. |
|
Wow, good work y'all. 🎉 |
|
I just upgraded ipykernel to 5.5.5 for this fix but it doesn't seem to be working for me: EDIT: it turns out this PR is not backported to 5.x releases; it works fine after |
|
Yep, will be in 6.0. |
5 participants
contextvarsdon't persist across cells in notebook ipython#11565 because tornado's wrapping of asyncio is a bit aggressive with creating Task objects which causes them to have a differentcontextvars.Context.cc @afshin @minrk @Carreau.
Note: tests are passing except for the in-process kernel tests, for which we will need to make more functions async.