Wrap concurrent.futures Future in polling function

[ondave]

Cannot await a concurrent.futures Future, so easiest fix is to wrap with asyncio.wrap_future

Closes #466, a bug influencing 0.14.0 and 0.14.1 I think - Edit by @consideRatio.

[consideRatio]

@minrk could you help review this one liner? I think could be a quite severe bug with a simple fix.

I'm not so confident about these things even though I learned a lot working with the async in some PRs before.

[welcome]

Congrats on your first merged pull request in this project! 🎉

Thank you for contributing, we are very proud of you! ❤️

[minrk]

Yup, this is right!

Background: tornado @gen.coroutine coroutines have a lot more 'awaitables' than stdlib asyncio (concurrent.futures.Futures, tornado's own concurrent.Future which is now an alias for asyncio.Future, internal objects like YieldPoints). A lot of this variety has fallen away with recent convergence of asyncio/tornado, but one distinction remains: asyncio doesn't support concurrent.futures.Future without this explicit wrapper step, but tornado still does. So our recent switch to async def coroutines means we had to make sure that we always ensure our tornado awaitables are cast to asyncio awaitables where necessary, and this one was missed.

A bit about why we have this: concurrent.futures.Future objects are threadsafe, so can be used to communicate a via a standard Future API across threads. asyncio Futures must not be used across threads. So one good way to signal from one thread to another is to use a concurrent.futures.Future. This wrap_future is the adaptation to make the threadsafe Future awaitable in asyncio.

It bothers me perhaps more than it should that:

1. asyncio thinks this is enough of an issue to provide the asyncio.wrap_future utility, but
2. asyncio does not bother to call this itself when it's clear and unambiguous that it's always the right thing to do (and/or concurrent.futures.Future doesn't implement this within its own __await__ methods to make them awaitable by default)

[consideRatio]

Thank you @ondave for reporting and fixing this issue!!! 🎉 ❤️!

@minrk is this a consequence of jupyterhub/jupyterhub#3242, which also mean it would only influence jupyterhub>=1.3.0 ? Should something be patched in JupyterHub itself following this?

[minrk]

is this a consequence of jupyterhub/jupyterhub#3242,

Close. It should only be relevant within the given function, so I'm pretty sure this was caused by #444, which made the same switch on this particular method. Take for example:

from concurrent.futures import ThreadPoolExecutor
from tornado import gen, ioloop


pool = ThreadPoolExecutor(1)


@gen.coroutine
def gen_coroutine():
    return (yield pool.submit(lambda: "tornado ok"))


async def async_def():
    return (await pool.submit(lambda: "asyncio ok"))


async def main():
    print("tornado?")
    print(await gen_coroutine())
    print("asyncio?")
    print(await async_def())

if __name__ == "__main__":
    ioloop.IOLoop.current().run_sync(main)

In this script, the tornado implementation can yield a c.f.Future and the async def can't, but the outer async def main() doesn't need to know about it, so it's not a far-reaching compatibility issue.

JupyterHub has its own compatibility layer in utils.maybe_future that already calls wrap_future where necessary. I did review jupyterhub/jupyterhub#3242 with that in mind, i.e. making sure we have utils.maybe_future where we have user-overrideable APIs that could be allowed to return these Futures (never officially supported in the docs, but technically would have worked prior to 1.3). That doesn't mean I caught everything, of course :)