GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-44176: Allow `asyncio.as_completed()`'s first parameter to be a generator yielding awaitables by alexdelorenzo · Pull Request #26228 · python/cpython

Conversation

@alexdelorenzo

@alexdelorenzo alexdelorenzo commented May 19, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Julian reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@alexdelorenzo
Use inspect.isawaitable() to detect if as_completed()'s first param i…
2372e2c
…s awaitable
@alexdelorenzo
alexdelorenzo requested review from 1st1 and asvetlov as code owners May 19, 2021 05:03
@the-knights-who-say-ni the-knights-who-say-ni added the CLA not signed label May 19, 2021
@the-knights-who-say-ni

the-knights-who-say-ni commented May 19, 2021

Copy link
Copy Markdown

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!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added the awaiting review label May 19, 2021
@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
@the-knights-who-say-ni the-knights-who-say-ni added CLA signed and removed CLA not signed labels Jun 2, 2021
@blurb-it
📜🤖 Added by blurb_it.
e962f20
@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
alexdelorenzo and others added 2 commits June 2, 2021 19:37
@alexdelorenzo
Merge branch 'python:main' into master
ce552b6
@alexdelorenzo
Use inspect.isawaitable() to detect if wait()'s first parameter is aw…
f75ef19
…aitable
@github-actions

github-actions Bot commented Jul 3, 2021

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Jul 3, 2021
uriyyo
uriyyo requested changes Sep 1, 2021
View reviewed changes

@uriyyo uriyyo left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Sep 1, 2021
@uriyyo

uriyyo commented Sep 1, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Fidget-Spinner

Fidget-Spinner commented Sep 1, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@uriyyo

uriyyo commented Sep 1, 2021

Copy link
Copy Markdown
Member

@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(

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@alexdelorenzo alexdelorenzo mannequin mentioned this pull request Apr 10, 2022
asyncio.as_completed() raises TypeError when the first supplied parameter is a generator that yields awaitables #88342
Closed
@ezio-melotti ezio-melotti removed the CLA signed label Jul 13, 2022
@DevilXD

DevilXD commented Aug 1, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot removed the stale Stale PR or inactive for long period of time. label Aug 8, 2022
@iritkatriel iritkatriel added the topic-asyncio label Dec 6, 2022
@kumaraditya303 kumaraditya303 closed this Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@uriyyo uriyyo uriyyo requested changes
@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner
@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting core review topic-asyncio

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@alexdelorenzo @the-knights-who-say-ni @uriyyo @Fidget-Spinner @DevilXD @iritkatriel @ezio-melotti @bedevere-bot @kumaraditya303
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.