async def foo():
    return 123

print(await foo())   # will print 123
print(await foo())   # prints None
print(await foo())   # prints None


The above code illustrates the current behaviour.  I propose to change it, so that second 'await' will trigger a RuntimeError, explaining the the coroutine was awaited more than once.

This would make coroutines more predictable, and would allow users to catch subtle bugs in their code.

This topic was originally brought up in this thread: https://mail.python.org/pipermail/python-dev/2015-December/142443.html

OK, but only for await (not for yield from).

It should always be valid to create a new coroutine instance. Perhaps you meant:

instance = foo()
print(await instance)  # Okay the first time
print(await instance)  # Second time should be an error

This seems sensible, at least for 3.6. Maybe it should also be an error to re-await if the coroutine raised an exception, and if it was cancelled via close().

> OK, but only for await (not for yield from).

Sure, generators will stay untouched (I'll even add a unittest for that, if we don't have it).


> It should always be valid to create a new coroutine instance. Perhaps you meant:

Correct, Martin, great that you've noticed that!

I'll duplicate the fixed code:

async def coroutine():
    return 123
coro = coroutine()

# In Python 3.5.1:
print(await coro)   # will print 123
print(await coro)   # prints None
print(await coro)   # prints None

# What we want in Python 3.5.2
print(await coro)   # will print 123
print(await coro)   # raises RuntimeError

PEP 492 is provisional, we can change things like this in 3.5.2.

On Wed, Dec 16, 2015 at 3:48 PM, Martin Panter <report@bugs.python.org>
wrote:

>
> Martin Panter added the comment:
>
> It should always be valid to create a new coroutine instance. Perhaps you
> meant:
>
> instance = foo()
> print(await instance)  # Okay the first time
> print(await instance)  # Second time should be an error
>
> This seems sensible, at least for 3.6. Maybe it should also be an error to
> re-await if the coroutine raised an exception, and if it was cancelled via
> close().
>
> ----------
> nosy: +martin.panter
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue25887>
> _______________________________________
>

Please review the attached patch.

New patch -- added more tests, made gen_send_ex() to always raise an error if the genobject is an exhausted coroutine.

Attaching another patch.  Please review (I plan to commit it tomorrow in 3.5 and 3.6 branches).

The patch affects generators machinery in the following way:

1. Generators behaviour isn't touched, the patch is only for 'async def' coroutines.

2. Calling 'send()' or 'throw()' on a coroutine object after it is exhausted or closed triggers a `RuntimeError("coroutine was already awaited")`

3. Calling 'close()' method on an exhausted or closed coroutines is a no-op.  'close()' can be called multiple times -- same as for generators.

I don't like "coroutine was already awaited". I feel like either "on" should be appended to that or another sentence like "coroutine had 'await' called on it" or something.

> I don't like "coroutine was already awaited". I feel like either "on" should be appended to that or another sentence like "coroutine had 'await' called on it" or something.

Fine with me to have "coroutine was already awaited on".

The patch looks good to me, but I'd like to see the error message convey two points:
- the coroutine has already terminated (regardless of how that happened)
- the calling code attempted to resume it anyway

That is, something like "Cannot resume terminated coroutine", rather than specifically referring to "await".

After all, waiting for the result with "await" is only one way to terminate a coroutine - you can also get there with direct calls to next(), send(), throw() and close().

The patch looks like it adds checks for the special generator-like send() etc methods, but I was expecting __await__() to also (or instead) do this check. That way you wouldn’t have to break the iterator protocol by the iterator not raising StopIteration a second time.

Also left some other review comments.

Nick,

> After all, waiting for the result with "await" is only one way to terminate a coroutine - you can also get there with direct calls to next(), send(), throw() and close().

Yes, but I expect that almost nobody will use 'send', 'throw' etc on coroutines, it's too low level.  In asyncio there is only one place where this magic happens.  Regular user will only understand the 'await' part -- that's why I wanted to mention it in the error message.

  async def something():
      await coro
      ^ RuntimeError('Cannot resume terminated coroutine')

The above use case might be a bit hard for users to actually understand, as opposed to

  async def something():
      await coro
      ^ RuntimeError('coroutine was already awaited on')

What do you think?

Martin, thanks for the review!

Attaching an updated patch.  The RuntimeError says 'coroutine was already awaited on' for now.

If the coroutine-iterator is going to raise RuntimeError rather than StopIteration, do you think the __await__() documentation <https://docs.python.org/dev/reference/datamodel.html#object.__await__> should be clarified?

IMO “yield from coroutine_iterator” might be plausable for some strange combination of 3.4 code and a 3.5 coroutine, but I think it would be rare. And if you added a check in __await__() then the using “await” wouldn’t need to rely on next() raising the RuntimeError.

> IMO “yield from coroutine_iterator” might be plausable for some strange combination of 3.4 code and a 3.5 coroutine, but I think it would be rare. And if you added a check in __await__() then the using “await” wouldn’t need to rely on next() raising the RuntimeError.

Adding the check *only* to __await__ will allow you to wrap an exhausted coroutine in an 'asyncio.Task' and await on it (the await will do nothing, which this patch fixes).

I think it's important to fix all coroutines' APIs to throw an error if they're manipulated in any way after being exhausted/closed, that's why I decided to fix the lower level.

To be honest, I don't care too much about 'yield from coro.__await__()' raising a RuntimeError (if the coro is an 'async def' coroutine that *is* closed/exhausted).  To me it's a clear behaviour.  Again, coroutine-iterators (objects returned by native coroutines from their __await__() method) aren't classical iterators meant to produce a fibonacci sequence in a for..in loop.  They are a low level interface to their coroutine objects.

Good point about having "await" in the error message to make it more obvious what it's referring to in the "awaiting twice" case.

I'll tender "Cannot await previously awaited coroutine" as one final alternative, but if you don't like that, I can cope with "Coroutine already awaited on" :)

"Cannot await previously awaited coroutine" might be confusing if raised on "coro.send(..)"-like code...

So far "coroutine was already awaited on" (do you want to drop "was", btw?) is less of all evil IMHO.

Sorry, dropping the "was" was a typo (I should have copied & pasted it instead of rewriting).

From the point of view of the error message, the reason I changed my suggestion was because I liked your idea of optimising it for the "only using await" case and trusting that the folks delving into the lower level plumbing of calling methods manually can figure it out. Anything that mentions await *at all* will be wrong in some cases, since what we're actually reporting is an attempt to resume (by some means) a coroutine that was previously terminated (by some means).

That is, "Cannot resume terminated coroutine" is always accurate, but relies on the reader knowing that "await" both resumes a coroutine and waits for it to terminate.

"Coroutine was previously awaited on" may be wrong about how the coroutine was originally terminated, but at least hints that the error may be related to awaiting the coroutine.

"Cannot resume previously awaited coroutine" would be inaccurate under the same circumstances.

"Cannot await previously awaited coroutine" would only be entirely accurate for "double await" errors, but doesn't rely on the reader making any assumptions at all in that case.

(The bulk of the problem here is that my brain is happy to accept "awaited" as a novel adjective modifying "coroutine", but balks at "awaited" as a verb or "awaited on" as a verb phrase. I'm extrapolating from that to guess that other folks would find the verb form similarly jarring)

Isn’t the combined point of this issue and Issue 25888 to make it only legal to “await” a coroutine instance that has not yet been started? Perhaps “Cannot [re]start terminated coroutine” would be better.

Oh, choosing a good error message is hard :(

I've a few comments.  Sorry, this thread is growing rather rapidly, but please help me to pick one of them:


> From the point of view of the error message, the reason I changed my suggestion was because I liked your idea of optimising it for the "only using await" case and trusting that the folks delving into the lower level plumbing of calling methods manually can figure it out. [...]

We either have to use a sub-optimal error message tailored for 'await' expression users, or we can choose a longer error message.  I'm not very fond of this option, but maybe it's because I can't come up with something long and concise at the same time.  Maybe something along the lines of: "cannot resume terminated coroutine (was it awaited on before?)"


> That is, "Cannot resume terminated coroutine" is always accurate, but relies on the reader knowing that "await" both resumes a coroutine and waits for it to terminate.

Not sure I like the "terminated" word here.  The coroutine was either awaited before, which means that the coroutine object is now exhausted, or it was manually closed at some point.  To me "terminated" is closer to the latter.


> "Coroutine was previously awaited on" may be wrong about how the coroutine was originally terminated, but at least hints that the error may be related to awaiting the coroutine.

Agree.


> "Cannot resume previously awaited coroutine" would be inaccurate under the same circumstances.

Thinking about "resume"...  If a user sees this message pointing to an "await" expression, it might confuse them, since "await coro" does not just resume "coro".  It inits and consumes "coro" until it raises a StopIteration.


"Cannot await previously awaited coroutine" would only be entirely accurate for "double await" errors, but doesn't rely on the reader making any assumptions at all in that case.

Agree.  But I don't like that we have two "await" words in one short message.


> (The bulk of the problem here is that my brain is happy to accept "awaited" as a novel adjective modifying "coroutine", but balks at "awaited" as a verb or "awaited on" as a verb phrase. I'm extrapolating from that to guess that other folks would find the verb form similarly jarring)

I trust your brain here, you're a native speaker ;)  If you think that "Cannot await previously awaited coroutine" is the best option here, let's stick to it.

I expect whatever message we use will become a Stack Overflow question in fairly short order, so "good enough" is almost certainly good enough here, and I'm massively overthinking the problem :)

However, combining my last suggestion with a variant of Martin's would give us:

    "Cannot reuse already awaited coroutine"

The generic "reuse" both eliminated the repetition of "await", and also gets us away from worrying about exactly what the caller was doing.

>    "Cannot reuse already awaited coroutine"

Great, I like it!  Thanks Martin and Nick.

Please check out issue #25888 just in case.

Hi there!

I've just stumbled upon this behavior and I was also surprised by the fact that the second await simply returns None.

After fiddling around for a while, I noticed that if I wrap the coroutine object using asyncio.ensure_future() or asyncio.get_event_loop().create_task(), the same result/exception is returned by multiple await expressions.

I haven't looked at the patch, but the intent to make the 2nd await raise a RuntimeError seems strange for several reasons:
- it's inconsistent with the Future/Task interface;
- it's quite common to await a 2nd time to get the coroutine result after calling asyncio.wait(...) using ALL_COMPLETED or FIRST_EXCEPTION;
- as mentioned in the mailing list the await keyword in C#/Hack/JS which inspired the await keyword (as per PEP492) returns the result/exception multiple times.

I put up a Gist that shows the inconsistency: https://gist.github.com/AndreLouisCaron/db2965aae095f5c85dd5

Here's an example of asyncio.wait() I was referencing:

async def main()
    f1 = foo()
    f2 = bar()
    asyncio.wait([f1, f2], return_when=asyncio.FIRST_EXCEPTION)
    print('1:', await f1)
    print('2:', await f2)

I also noticed that there seems to be some intent to avoid making a distinction between a normal function returning a future and a coroutine function from the point of view of the caller.

If the patch is merged as is, I will always need to use asyncio.ensure_future() on all coroutine calls before asyncio.wait() because the result is inconsistent depending on the implementation of foo() and bar(): if they return futures, I'm OK, but if any of them is a proper coroutine function, I might get RuntimeError exceptions.

Any chance you can consider changing the patch to make awaiting a coroutine's result multiple times a valid pattern?

> I haven't looked at the patch, but the intent to make the 2nd 
> await raise a RuntimeError seems strange for several reasons:
> - it's inconsistent with the Future/Task interface;

Well, coroutines are much more lower level than Future/Tasks.


> - it's quite common to await a 2nd time to get the coroutine 
> result after calling asyncio.wait(...) using ALL_COMPLETED or 
> FIRST_EXCEPTION;

I believe you're not using the asyncio.task() function correctly.  
From the docs:

"""Wait for the Futures and coroutine objects given by the sequence 
futures to complete. Coroutines will be wrapped in Tasks. Returns 
two sets of Future: (done, pending)."""


> - as mentioned in the mailing list the await keyword in 
> C#/Hack/JS which inspired the await keyword (as per PEP492) 
> returns the result/exception multiple times.

Yes, this was discussed at length on the mailing list.  There are
many reasons as to why we don't want coroutines to be awaitable
many times.  One of them is that we don't want low-level coroutine
objects to hold references to return values.  Coroutines in Python
are exhaustible resources (like generators).

> I believe you're not using the asyncio.task() function correctly.

I assume you meant asyncio.wait().

I just updated my gist with a variant of my example that uses the (done, pending) pair returned by asyncio.wait() as you suggested.

The set of done futures that is returned does not help in this case because you need to test objects by set membership using something like "if f1 in done:", which will never be true because f1 is a coroutine object and the set contains a wrapper returned by asyncio.ensure_future().

So we're back to square 1.  Because you cannot await multiple times on a coroutine object (in contrast to futures), you will need to explicitly call asyncio.ensure_future() on everything you pass to asyncio.wait() if you care about the future's result -- which is more often than not in my experience.

> There are many reasons as to why we don't want coroutines to be awaitable
many times.  One of them is that we don't want low-level coroutine
objects to hold references to return values.  Coroutines in Python
are exhaustible resources (like generators).

I understand concerns for efficiency and management of scarce resources.

However, I don't understand your comment.  Maybe this has something to do with CPython/asyncio internals, which I know nothing about.  If my code keeps a reference to the coroutine object, it can't be released right?  I don't see how adding a reference to the return value or wrapping the coroutine object in a Task wrapper (in order to recover the return value) will affect the allocation of coroutine resources.

> Well, coroutines are much more lower level than Future/Tasks.

From an implementer's point of view, maybe.  From a user's point of vue, the value of having an "awaitable" concept is that I can use different objects in the same way.  If the semantics of await expressions vary depending on the awaitable's type, then the value of the abstraction is reduced.

If we go back to the core of the issue, the real problem here is the fact that the current behaviour of multiple awaits on a coroutine object is surprising: nobody expects it to return None the 2nd time around.

Raising a RuntimeError instead of returning None is still surprising behaviour IMO.

In addition to that, the error message that was agreed upon in this issue suggests a programming error (you're not _supposed_ to await twice), but what I'm getting to here with my example is that asyncio.wait() is a common/legitimate use case of this.

Anyways, if you insist on considering this a programming error, you should probably reject coroutine objects in calls to asyncio.wait() since there will be effectively no way of recovering the return value after the call to asyncio.wait().

After thinking about this some more, I think my problem with asyncio.wait() is a bit bigger than the simple fact that coroutine objects cannot be awaited multiple times.  It seems to me like asyncio.wait() is completely broken for coroutine objects as inputs and that multiple awaits on coroutine objects only make that problem worse.

While I still think there is a major benefit API-wise to have await expressions produce the same behaviour for both Futures and coroutine objects, I'm moving that discussion to issue #26357.

http://bugs.python.org/issue26357

> After thinking about this some more, I think my problem with asyncio.wait() is a bit bigger than the simple fact that coroutine objects cannot be awaited multiple times.  It seems to me like asyncio.wait() is completely broken for coroutine objects as inputs and that multiple awaits on coroutine objects only make that problem worse.

> While I still think there is a major benefit API-wise to have await expressions produce the same behaviour for both Futures and coroutine objects, I'm moving that discussion to issue #26357.

Looks like your problem can solved if you wrap coroutine objects in Tasks and then pass them to asyncio.wait().  In any case, it's good that you've opened #26357 to discuss that.  

I'm going to commit the patch for this issue now, since the current behaviour of coroutines returning None on second await is wrong.  Allowing multiple awaits for coroutine objects also seems very wrong to me, but please feel free to discuss that on python-ideas.

New changeset c2a2685ab89b by Yury Selivanov in branch '3.5':
Issue #25887: Raise a RuntimeError when a coroutine is awaited more than once.
https://hg.python.org/cpython/rev/c2a2685ab89b

New changeset 23297d5bbd29 by Yury Selivanov in branch 'default':
Merge 3.5 (issue #25887)
https://hg.python.org/cpython/rev/23297d5bbd29