python / cpython

The Python programming language
https://www.python.org
Other
62.59k stars 30.04k forks source link

asyncio.wait loses coroutine return value #70545

Closed 9b6fbe46-05dc-4bf0-a649-5a545d776750 closed 6 years ago

9b6fbe46-05dc-4bf0-a649-5a545d776750 commented 8 years ago
BPO 26357
Nosy @gvanrossum, @vstinner, @asvetlov, @1st1
Files
  • asyncio-wait-coroutine.py: Demonstration of problem.
  • 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: ```python assignee = None closed_at = created_at = labels = ['expert-asyncio'] title = 'asyncio.wait loses coroutine return value' updated_at = user = 'https://bugs.python.org/AndrCaron' ``` bugs.python.org fields: ```python activity = actor = 'asvetlov' assignee = 'none' closed = True closed_date = closer = 'asvetlov' components = ['asyncio'] creation = creator = 'Andr\xc3\xa9 Caron' dependencies = [] files = ['41914'] hgrepos = [] issue_num = 26357 keywords = [] message_count = 5.0 messages = ['260250', '260257', '260325', '260327', '308861'] nosy_count = 5.0 nosy_names = ['gvanrossum', 'vstinner', 'asvetlov', 'yselivanov', 'Andr\xc3\xa9 Caron'] pr_nums = [] priority = 'normal' resolution = 'wont fix' stage = 'resolved' status = 'closed' superseder = None type = None url = 'https://bugs.python.org/issue26357' versions = ['Python 3.5'] ```

    9b6fbe46-05dc-4bf0-a649-5a545d776750 commented 8 years ago

    When the asyncio.wait() function with coroutine objects as inputs, it is impossbible to extract the results reliably.

    This issue arises because asyncio.wait() returns an unordered set of futures against which membership tests of coroutine objects always fail.

    The issue is made even worse by the fact that coroutine objects cannot be awaited multiple times (see https://bugs.python.org/issue25887). Any await expression on the coroutine object after the call to asyncio.wait() returns None, regardless of the coroutine's return value.

    (See attached asyncio-wait-coroutine.py for an example of both these issues.)

    In the worst case, multiple inputs are coroutine objects and the set of returned futures contains return values but there is no way to determine which result corresponds to which coroutine call.

    To work around this issue, callers need to explicitly use asyncio.ensure_future() on coroutine objects before calling asyncio.wait().

    (See comment in asyncio-wait-coroutine.py for an example "fix").

    Note that, in general, it is not possible to know ahead of time whether all inputs to asyncio.wait() are coroutines or futures. Furthermore, the fact that a given third-party library function is implemented as a regular function that returns a Future or a proper coroutine is an implementation decision which may not be part of the public interface. Even if it is, the inputs to asyncio.wait() may come from complex code paths and it may be difficult to verify that all of them end up producing a Future.

    As a consequence, the only reliable way to recover all results from asyncio.wait() is to explicitly call asyncio.ensure_future() on each of the inputs.

    When doing so, both the membership test against the done set and the await expressions work as expected.

    Quickly, there are several possible solutions:

    Related issue: https://bugs.python.org/issue25887 proposes a patch to change the behaviour of awaiting a coroutine object multiple times in order to produce a RuntimeError on all awaits after the 1st. While that change in behaviour would make it easier to diagnose the loss of the return value, it does not fix this issue.

    gvanrossum commented 8 years ago

    I wonder if the right solution isn't to insist that the arguments to await() are Futures, not coroutines. While in many cases (e.g. gather(), wait_for()) it's a useful convention to support either coroutines or Futures, for wait() it does seem pretty useless. Mapping the Futures back to coroutines isn't a good solution either, since you can't get the result out of just the coroutine.

    The question is whether we can just change this or whether we should issue some kind of deprecation warning, since it's at least conceivable that someone relies on the current behavior and doesn't care about mapping results back to coroutines (e.g. if the results are self-identifying).

    The same issue applies to as_completed(), I think.

    9b6fbe46-05dc-4bf0-a649-5a545d776750 commented 8 years ago

    Hi Guido,

    Thanks for the quick reply :-)

    AFAICT, there seem to be three possible directions regarding this issue -- for both wait() and as_completed():

    1) remove the need for ensure_future(): make the membership test succeed and allow multiple await expressions on the same coroutine;

    2) fail fast: reject coroutine objects as inputs to wait() and reject multiple await expressions on coroutine objects (see issue bpo-25887);

    3) clarify API usage: deprecate couroutine objects and enhance docs for wait(), as_completed() and ensure_future().

    From a pure API standpoint, #1 makes the API more uniform and less surprising, #2 makes it easier to detect and fix incorrect usage and #3 accelerates troubleshooting when people get bitten by this quirk in the API.

    You're right that technically, the case of side-effect-only coroutine invocations is a use case that's currently supported by wait() and that removing this (e.g. by adopting #2) would be a compatibility break. I personally doubt that this is a common use case as both wait() and as_completed() seem specifically designed to recover the results, but I'm not an asyncio expert either.

    Asyncio is still young and is undergoing adoption, so I would like to see this issue resolved without resorting to #3.

    Any chance #1 or #2 can be considered?

    1st1 commented 8 years ago

    TBH I never ever needed to do membership tests on (done, failed) result of asyncio.wait. If you need to do such tests - just wrap your coroutines into tasks manually. I honestly don't understand what's the problem and why we need to change anything in asyncio or in Python. There're tons of code on asyncio now, and this is only a second time someone wants to "fix" await.

    Fixing bpo-25887 allows us to enable multiple awaits on coroutines later, but I wouldn't rush that in 3.5 or 3.6.

    Restricting asyncio.wait to accept only futures will also cause a lot of pain. I'd just fix the docs with an explanation of this problem and with a snippet of code showing how to do membership tests if needed. Alternatively, we can add a wrapper for asyncio.wait (wait_futures?), that will only accept futures/tasks.

    asvetlov commented 6 years ago

    Let's close as "wont fix". If user need an identity for awaited coroutine -- he/she should return it as part of coroutine result.