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

bpo-44176: Allow asyncio.as_completed()'s first parameter to be a generator yielding awaitables #26228

Closed
wants to merge 4 commits into from

Conversation

alexdelorenzo
Copy link

@alexdelorenzo alexdelorenzo commented May 19, 2021

Allow asyncio.as_completed()'s first parameter to be a generator yielding awaitables

bpo-44176: asyncio.as_completed() raises TypeError when the first supplied parameter is a generator that yields awaitables

https://bugs.python.org/issue44176

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

Recognized GitHub username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@alexdelorenzo

This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

@alexdelorenzo alexdelorenzo changed the title Use inspect.isawaitable() to detect if as_completed()'s first param is awaitable bpo-44176: Use inspect.isawaitable() to detect if as_completed()'s first param is awaitable May 19, 2021
@alexdelorenzo alexdelorenzo changed the title bpo-44176: Use inspect.isawaitable() to detect if as_completed()'s first param is awaitable bpo-44176: Use inspect.isawaitable() to detect if as_completed()'s first parameter is awaitable Jun 2, 2021
@alexdelorenzo alexdelorenzo changed the title bpo-44176: Use inspect.isawaitable() to detect if as_completed()'s first parameter is awaitable bpo-44176: Allow asyncio.as_completed() first parameter to be a generator yielding awaitables Jun 2, 2021
@alexdelorenzo alexdelorenzo changed the title bpo-44176: Allow asyncio.as_completed() first parameter to be a generator yielding awaitables bpo-44176: Allow asyncio.as_completed()'s first parameter to be a generator yielding awaitables Jun 2, 2021
@github-actions
Copy link

github-actions bot commented Jul 3, 2021

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Jul 3, 2021
Copy link
Member

@uriyyo uriyyo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Could you please also add tests to cover case when you pass generator to asyncio.as_completed and asyncio.wait function call?

@uriyyo
Copy link
Member

uriyyo commented Sep 1, 2021

After investigation I think that actual problem is in asyncio.iscoroutine function.

From iscoroutine docs:

This method is different from inspect.iscoroutine() because it returns True for generator-based coroutines.

But actually this function returns true for generators that is not coroutines:

from asyncio import iscoroutine, run
from types import coroutine


@coroutine
def coro():
    yield


def not_coro():
    yield


print(iscoroutine(coro()))     # True
print(iscoroutine(not_coro())) # True

But if we will try to await not_coro() it will fail:

async def main():
    await coro()      # ok
    await not_coro()  # will fail with TypeError: object generator can't be used in 'await' expression

In my opinion asyncio.iscoroutine should check that generator is actually coroutine like inspect.isawaitable do:

cpython/Lib/inspect.py

Lines 351 to 352 in 863154c

isinstance(object, types.GeneratorType) and
bool(object.gi_code.co_flags & CO_ITERABLE_COROUTINE) or

@Fidget-Spinner What is your opinion regarding this problem?

Should we ping Yury and Andrew regarding this?

UPD: While I was playing with as_completed I have found interesting behevior:

def not_coro():
    yield


async def main():
    await gather(not_coro()) # will not fail
    await not_coro() # will fail

It looks like a bug, but I should do more investigation.

@Fidget-Spinner
Copy link
Member

Fidget-Spinner commented Sep 1, 2021

@uriyyo great investigation! That's a really interesting find.

IMO, changing the behavior of asyncio.iscoroutine is backwards incompatible. There is a StackOverflow question on this, and the linked bug report seems to me that Yury was already aware of this behavior. So we should probably stick to changing just as_completed.

I think we can ping Yury and Andrew politely on the bug tracker once OP adds tests like you mentioned.

BTW, I use asyncio, but you probably have more knowledge about asyncio internals than me :). So this is a learning experience for me too (ie I might make mistakes).

@uriyyo
Copy link
Member

uriyyo commented Sep 1, 2021

@Fidget-Spinner Ideal solution will be to change behavior of asyncio.iscoroutine to not return true for a generators that is not coroutines. It will fix this issue and will not allow to use non-coroutine generators with asyncio.gather and others.

For instance currently, code bellow will not fail:

await gather(None for _ in range(10))

When we have generator:

def not_coro():
    yield

It's not valid to await it:

await not_coro()  # TypeError: object generator can't be used in 'await' expression

But all cases below will not fail:

await create_task(not_coro())
await ensure_future(not_coro())
await gather(not_coro())
await wait([not_coro()])
await wait([not_coro()])
await wait_for(not_coro(), 10)
await shield(not_coro())

for f in as_completed([not_coro()]):
    print(await f)

We can fix it be changing asyncio.iscoroutine to:

_COROUTINE_TYPES = (types.CoroutineType, collections.abc.Coroutine)
_iscoroutine_typecache = set()


def iscoroutine(obj):
    """Return True if obj is a coroutine object."""
    if type(obj) in _iscoroutine_typecache:
        return True

    if isinstance(obj, _COROUTINE_TYPES):
        # Just in case we don't want to cache more than 100
        # positive types.  That shouldn't ever happen, unless
        # someone stressing the system on purpose.
        if len(_iscoroutine_typecache) < 100:
            _iscoroutine_typecache.add(type(obj))
        return True
    elif isinstance(obj, types.GeneratorType) and obj.gi_code.co_flags & inspect.CO_ITERABLE_COROUTINE:
        return True
    else:
        return False

But this solution is not backward compatible(

@DevilXD
Copy link
Contributor

DevilXD commented Aug 1, 2022

Just ran into this issue myself and managed to find this PR. Looking forward to it being merged.

@github-actions github-actions bot removed the stale Stale PR or inactive for long period of time. label Aug 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants