Skip to content

Conversation

@alexdelorenzo
Copy link

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

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!

@alexdelorenzoalexdelorenzo changed the title Use inspect.isawaitable() to detect if as_completed()'s first param is awaitablebpo-44176: Use inspect.isawaitable() to detect if as_completed()'s first param is awaitableMay 19, 2021
@alexdelorenzoalexdelorenzo changed the title bpo-44176: Use inspect.isawaitable() to detect if as_completed()'s first param is awaitablebpo-44176: Use inspect.isawaitable() to detect if as_completed()'s first parameter is awaitableJun 2, 2021
@alexdelorenzoalexdelorenzo changed the title bpo-44176: Use inspect.isawaitable() to detect if as_completed()'s first parameter is awaitablebpo-44176: Allow asyncio.as_completed() first parameter to be a generator yielding awaitablesJun 2, 2021
@alexdelorenzoalexdelorenzo changed the title bpo-44176: Allow asyncio.as_completed() first parameter to be a generator yielding awaitablesbpo-44176: Allow asyncio.as_completed()'s first parameter to be a generator yielding awaitablesJun 2, 2021
@github-actions
Copy link

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

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

@uriyyouriyyo 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 iscoroutinedocs:

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:

fromasyncioimportiscoroutine, runfromtypesimportcoroutine@coroutinedefcoro(): yielddefnot_coro(): yieldprint(iscoroutine(coro())) # Trueprint(iscoroutine(not_coro())) # True

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

asyncdefmain(): awaitcoro() # okawaitnot_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:

defnot_coro(): yieldasyncdefmain(): awaitgather(not_coro()) # will not failawaitnot_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

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

awaitgather(Nonefor_inrange(10))

When we have generator:

defnot_coro(): yield

It's not valid to await it:

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

But all cases below will not fail:

awaitcreate_task(not_coro()) awaitensure_future(not_coro()) awaitgather(not_coro()) awaitwait([not_coro()]) awaitwait([not_coro()]) awaitwait_for(not_coro(), 10) awaitshield(not_coro()) forfinas_completed([not_coro()]): print(awaitf)

We can fix it be changing asyncio.iscoroutine to:

_COROUTINE_TYPES= (types.CoroutineType, collections.abc.Coroutine) _iscoroutine_typecache=set() defiscoroutine(obj): """Return True if obj is a coroutine object."""iftype(obj) in_iscoroutine_typecache: returnTrueifisinstance(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.iflen(_iscoroutine_typecache) <100: _iscoroutine_typecache.add(type(obj)) returnTrueelifisinstance(obj, types.GeneratorType) andobj.gi_code.co_flags&inspect.CO_ITERABLE_COROUTINE: returnTrueelse: returnFalse

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-actionsgithub-actionsbot removed the stale Stale PR or inactive for long period of time. label Aug 8, 2022
Sign up for freeto 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

@alexdelorenzo@the-knights-who-say-ni@uriyyo@Fidget-Spinner@DevilXD@iritkatriel@ezio-melotti@bedevere-bot@kumaraditya303