Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

awaiting on coroutine more than once should be an error #70075

Closed
1st1 opened this issue Dec 16, 2015 · 29 comments
Closed

awaiting on coroutine more than once should be an error #70075

1st1 opened this issue Dec 16, 2015 · 29 comments
Assignees
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@1st1
Copy link
Member

1st1 commented Dec 16, 2015

BPO 25887
Nosy @gvanrossum, @brettcannon, @ncoghlan, @vstinner, @asvetlov, @vadmium, @1st1
Files
  • Issue25887.patch
  • Issue25887_2.patch
  • Issue25887_3.patch
  • Issue25887_4.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/1st1'
    closed_at = <Date 2016-02-13.23:01:11.496>
    created_at = <Date 2015-12-16.21:46:57.616>
    labels = ['interpreter-core', 'type-feature']
    title = 'awaiting on coroutine more than once should be an error'
    updated_at = <Date 2016-02-13.23:01:11.496>
    user = 'https://github.com/1st1'

    bugs.python.org fields:

    activity = <Date 2016-02-13.23:01:11.496>
    actor = 'yselivanov'
    assignee = 'yselivanov'
    closed = True
    closed_date = <Date 2016-02-13.23:01:11.496>
    closer = 'yselivanov'
    components = ['Interpreter Core']
    creation = <Date 2015-12-16.21:46:57.616>
    creator = 'yselivanov'
    dependencies = []
    files = ['41336', '41337', '41587', '41588']
    hgrepos = []
    issue_num = 25887
    keywords = ['patch']
    message_count = 29.0
    messages = ['256535', '256564', '256566', '256567', '256568', '256610', '256611', '258039', '258041', '258042', '258044', '258048', '258050', '258054', '258058', '258059', '258061', '258064', '258065', '258066', '258067', '258068', '258069', '260200', '260203', '260248', '260251', '260253', '260254']
    nosy_count = 9.0
    nosy_names = ['gvanrossum', 'brett.cannon', 'ncoghlan', 'vstinner', 'asvetlov', 'python-dev', 'martin.panter', 'yselivanov', 'Andr\xc3\xa9 Caron']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue25887'
    versions = ['Python 3.5', 'Python 3.6']

    @1st1
    Copy link
    Member Author

    1st1 commented Dec 16, 2015

    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

    @1st1 1st1 self-assigned this Dec 16, 2015
    @1st1 1st1 added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement labels Dec 16, 2015
    @gvanrossum
    Copy link
    Member

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

    @vadmium
    Copy link
    Member

    vadmium commented Dec 16, 2015

    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().

    @1st1
    Copy link
    Member Author

    1st1 commented Dec 16, 2015

    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

    @gvanrossum
    Copy link
    Member

    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\>


    @1st1
    Copy link
    Member Author

    1st1 commented Dec 17, 2015

    Please review the attached patch.

    @1st1
    Copy link
    Member Author

    1st1 commented Dec 17, 2015

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

    @1st1
    Copy link
    Member Author

    1st1 commented Jan 11, 2016

    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.

    @brettcannon
    Copy link
    Member

    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.

    @1st1
    Copy link
    Member Author

    1st1 commented Jan 12, 2016

    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".

    @ncoghlan
    Copy link
    Contributor

    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().

    @vadmium
    Copy link
    Member

    vadmium commented Jan 12, 2016

    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.

    @1st1
    Copy link
    Member Author

    1st1 commented Jan 12, 2016

    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!

    @1st1
    Copy link
    Member Author

    1st1 commented Jan 12, 2016

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

    @vadmium
    Copy link
    Member

    vadmium commented Jan 12, 2016

    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.

    @1st1
    Copy link
    Member Author

    1st1 commented Jan 12, 2016

    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.

    @ncoghlan
    Copy link
    Contributor

    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" :)

    @1st1
    Copy link
    Member Author

    1st1 commented Jan 12, 2016

    "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.

    @ncoghlan
    Copy link
    Contributor

    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)

    @vadmium
    Copy link
    Member

    vadmium commented Jan 12, 2016

    Isn’t the combined point of this issue and bpo-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.

    @1st1
    Copy link
    Member Author

    1st1 commented Jan 12, 2016

    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.

    @ncoghlan
    Copy link
    Contributor

    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.

    @1st1
    Copy link
    Member Author

    1st1 commented Jan 12, 2016

    "Cannot reuse already awaited coroutine"

    Great, I like it! Thanks Martin and Nick.

    Please check out issue bpo-25888 just in case.

    @AndrCaron
    Copy link
    Mannequin

    AndrCaron mannequin commented Feb 12, 2016

    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 PEP-492) 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?

    @1st1
    Copy link
    Member Author

    1st1 commented Feb 12, 2016

    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 PEP-492)
      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).

    @AndrCaron
    Copy link
    Mannequin

    AndrCaron mannequin commented Feb 13, 2016

    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().

    @AndrCaron
    Copy link
    Mannequin

    AndrCaron mannequin commented Feb 13, 2016

    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 bpo-26357.

    http://bugs.python.org/issue26357

    @1st1
    Copy link
    Member Author

    1st1 commented Feb 13, 2016

    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 bpo-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 bpo-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.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 13, 2016

    New changeset c2a2685ab89b by Yury Selivanov in branch '3.5':
    Issue bpo-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 bpo-25887)
    https://hg.python.org/cpython/rev/23297d5bbd29

    @1st1 1st1 closed this as completed Feb 13, 2016
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants