asyncio icon indicating copy to clipboard operation
asyncio copied to clipboard

Resurrect asyncio.timeout() context manager

Open asvetlov opened this issue 9 years ago • 5 comments

In https://mail.python.org/pipermail/async-sig/2016-June/000033.html Ben Darnell asked for asyncio.timeout() removing.

I understand the problem (hopefully).

But the timeout context manager is very important. It's really user friendly.

Also it produces very small overhead. In opposite calling wait_for(f(), timeout=1) is expensive thanks to new task creation. @1st1 can confirm it. He had significant performance degradation when worked on https://github.com/MagicStack/asyncpg by using naive wait_for calls for adding timeouts.

From my understanding the main @bdarnell objection was using with asyncio.timeout(...) but async with asyncio.timeout(...) solves his problem.

If it's the only objection I can rewrite the code to use async context manager, if the change make code more portable. It's easy task. At very early version of aiohttp.Timeout() we used async with but figured out than just with was enough for our purposes.

@bdarnell, does switching to async context manager solve compatibility problem with Tornado?

asvetlov avatar Aug 02 '16 19:08 asvetlov

Switching to async with doesn't solve the problem on its own; you need to use the asynchronous capabilities in a to-be-determined protocol to communicate with the coroutine runner (instead of assuming that it can find an asyncio.Task in a thread-specific global). There are two options that I would consider acceptable, although I don't really like either of them:

  • Keep the thread-specific global, but remove the assumptions about asyncio.Task: Add methods {push,peek,pop}_coroutine_runner to the asyncio module and define the methods that a coroutine runner must implement (in this case, set_timeout). tornado.gen.Runner could then implement set_timeout and push/pop itself onto the stack whenever it runs.
  • Define a new type of objects that may be yielded in coroutines (in addition to Futures), use async with asyncio.timeout, and use these objects to communicate directly with the current coroutine runner. Untested:
class timeout(object):
    def __init__(self, timeout):
        self.timeout = timeout

    def __aenter__(self):
        yield SetTimeout(self.timeout)

    def __aexit__(self, *args):
        yield ClearTimeout()
        return False

Either way, the problem is one of defining a protocol that all coroutine runners will have to implement, so it probably needs a PEP. (Or, as I said in the linked message, we could abandon the idea of supporting multiple coroutine runners in the ecosystem and make asyncio.Task the one true coroutine runner. However, it would need some enhancements before I would consider that a good outcome)

bdarnell avatar Aug 03 '16 02:08 bdarnell

I think first option ({push,peek,pop}_coroutine_runner) is better than allowing to yield non-futures from async methods.

But anyway I would to stress that async PEP mention Task class many times (but doesn't specify a Task interface as the pep does it for Future and other classes). De-facto every coroutine in asyncio world is executed inside a task, so Task.current_task() always work correctly. Only callbacks like protocol methods and delayed calls have no task context.

About multiple coroutine runners idea -- I have no personal opinion. I don't know how Tornado and Twisted supports async def coroutines to make decision about the need for multiple runners.

asvetlov avatar Aug 04 '16 15:08 asvetlov

The asyncio PEP defines two separate things: support for interoperability between asynchronous frameworks (mainly defined in terms of the event loop interface and Futures), and a specific asynchronous framework (using transports, protocols, etc). Tasks are part of the latter framework; the PEP specifically says that "For interoperability... the scheduler defines a Task class that behaves like a Future". That is, it is the Future-like parts of the Task interface that matter for other frameworks.

Prior to the introduction of asyncio.timeout, it was possible to interoperate with asyncio without knowing anything about Tasks. This is an important property since the Task interface is unspecified.

De-facto every coroutine in asyncio world is executed inside a task

That depends on how you define the "asyncio world". Tornado interoperates with asyncio so that coroutines that use asyncio functionality can be run under tornado.gen.coroutine instead of asyncio.Task (so it's possible to use asyncpg from a Tornado app).

bdarnell avatar Aug 05 '16 16:08 bdarnell

I've found this implementation on pypi : https://github.com/aio-libs/async-timeout/blob/master/async_timeout/init.py If I understand this thread correctly, this is a valid implementation but based on an assumption : that it can find an asyncio.Task in a thread-specific global ?

Lucas-C avatar May 03 '17 14:05 Lucas-C

Yes, that async_timeout package requires that it can find an asyncio.Task in a thread-specific global (I think it's the same as what was briefly in the standard library). It works in a pure-asyncio world but is incompatible with Tornado/asyncio integration (I'm trying to make this better in Tornado 5.0)

bdarnell avatar May 20 '17 21:05 bdarnell