Skip to content
This repository was archived by the owner on Nov 23, 2017. It is now read-only.

Add more highlights in docs and examples that coroutines definitions always depend on loop context #332

Open
rudyryk opened this issue Apr 20, 2016 · 5 comments

Comments

@rudyryk
Copy link

rudyryk commented Apr 20, 2016

Hello, everyone! This is more discussion issue than bug report or kind of.

For my opinion code examples in official asyncio docs are broken. Well, not totally broken of course, but they give a wrong idea that coroutines definition are independent of loop context!

I'd stress the word definition. Because I think it's clear, that coroutines execution depends on loop. But it's totally unclear, that definition also depends on it.

Here's an example from docs:
https://docs.python.org/3/library/asyncio-task.html#example-coroutine-displaying-the-current-date

import asyncio
import datetime

async def display_date(loop):
    end_time = loop.time() + 5.0
    while True:
        print(datetime.datetime.now())
        if (loop.time() + 1.0) >= end_time:
            break
        await asyncio.sleep(1)

loop = asyncio.get_event_loop()
# Blocking call which returns when the display_date() coroutine is done
loop.run_until_complete(display_date(loop))
loop.close()

It works.

Now let's change a single line:

loop = asyncio.new_event_loop()

It doesn't.

But the fix isn't really hard:

await asyncio.sleep(1, loop=loop)

Well, the next example is even worse:

import asyncio

async def compute(x, y):
    print("Compute %s + %s ..." % (x, y))
    await asyncio.sleep(1.0)
    return x + y

async def print_sum(x, y):
    result = await compute(x, y)
    print("%s + %s = %s" % (x, y, result))

loop = asyncio.get_event_loop()
loop.run_until_complete(print_sum(1, 2))
loop.close()

It can't run on a custom loop. Because compute() and print_sum() coroutines have no idea of event loop and thus can't be fixed without changing interface.

Well, I think we should handle that really carefully :)

Either rewrite examples in docs to provide correct patterns or handle custom loops automatically. About second option, don't know if that's possible or really good idea :)

@rudyryk rudyryk changed the title Add more highlights in docs and examples that coroutines definitions are always depend on loop context Add more highlights in docs and examples that coroutines definitions always depend on loop context Apr 20, 2016
@Martiusweb
Copy link
Member

Martiusweb commented Apr 20, 2016

Hi,

On the line :

    await asyncio.sleep(1)

The loop is implicitly retrieved from asyncio.get_event_loop().

However, when calling asyncio.new_event_loop(), it doesn't set the default
loop, returned by get_event_loop(). Thus, asyncio.sleep() is bound to a
different loop than the one executing the coroutine and will never
terminate.

If you want to use several loops, one can be the default loop returned by
get_event_loop(), the other one must be passed as an argument.

Also, when one awaits on a future/task which is not bound to the loop
executing the coroutine, an exception is raised. We may want
asyncio.sleep() to also raise an exception in that case.

@rudyryk
Copy link
Author

rudyryk commented Apr 20, 2016

Hi, @Martiusweb! Yes, I totally understand now, what happens and usually write my code against explicitly specified event loop.

But I came to that after one year of development with asyncio :) And now I can say, it's one of the most unclear and tricky parts. And so I find introductory docs confusing about that.

And regarding code examples:

The loop is implicitly retrieved from asyncio.get_event_loop().

But we have also defined loop as argument:

async def display_date(loop):

Why we don't pass it as an argument to sleep()?

We may want asyncio.sleep() to also raise an exception in that case.

Yes, I didn't think about that, but sounds reasonable. Now it's just silently pending forever.

While writing coroutine we should pick one of two options:

  1. Enable support of arbitrary (not set as current) event loop
  2. Always run on current event loop

The first option is more verbose and more explicit. The second one is compact and implicit. And that's not really simple question, which option if preferable in a particular case.

I think that reusable library should always pick the first option. And users (application) code may want to pick the second one.

And, probably the most common misuse is forgetting to set current event loop. For example, as far as I remember test suit in Tornado framework creates new event loop for every test (its own, not asyncio one), and it's user's responsibility to set up corresponding current asyncio loop.

@Martiusweb
Copy link
Member

asyncio.sleep() accepts an optional explicit loop argument, as any function or object constructor in asyncio. When the optional loop argument is None, the loop is retrieved from asyncio.get_event_loop().

Indeed, reusable libraries should follow the convention of an optional loop argument and defaulting to asyncio.get_event_loop().

In the application code, I can't see cases where you want to use several loops in one thread. I chose explicit loop passing most of the time, but one could use a custom EventLoopPolicy which sets a loop per thread, stored as a thread-local object.

@rudyryk
Copy link
Author

rudyryk commented Apr 20, 2016

I think it would be good to have such explanation as introduction to coroutines / tasks with code samples for both approaches :)

Currently, "Tasks and coroutines" documentation tells how to do things, but doesn't cover motivation and proper design practices. And I would insist that the first example is broken :) If we pass loop as argument, but do not use it, it's probably a bug. Just imagine, that display_date() would be a 3rd party library.

@Martiusweb
Copy link
Member

I agree: those examples are a bit misleading, and the documentation misses a general discussion about how and when asyncio should be used. I remember that the PEP (https://www.python.org/dev/peps/pep-3156/) helped me a bit.

mbuferli added a commit to mbuferli/asyncio-labs that referenced this issue Jul 8, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants