classification
Title: avoid explicit generator type check in asyncio
Type: behavior Stage: resolved
Components: asyncio Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: gvanrossum, martin.panter, python-dev, scoder, vstinner, yselivanov
Priority: normal Keywords: patch

Created on 2015-04-19 10:21 by scoder, last changed 2015-05-31 15:29 by yselivanov. This issue is now closed.

Files
File name Uploaded Description Edit
coroutine.patch yselivanov, 2015-05-30 20:01 review
Messages (18)
msg241506 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2015-04-19 10:21
asyncio/coroutines.py contains this code:

"""
_COROUTINE_TYPES = (types.GeneratorType, CoroWrapper)

def iscoroutine(obj):
    """Return True if obj is a coroutine object."""
    return isinstance(obj, _COROUTINE_TYPES)
"""

In other places, it uses inspect.isgenerator() to do the same thing. This is inconsistent and should be changed. Code that wants to patch inspect.isgenerator() in order to support other generator types shouldn't also have to patch asyncio directly, and code that wants to support other generator types in asyncio shouldn't have to patch both places either.
msg241595 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2015-04-20 03:32
Uh, wait. Who's patching anything? That breaks the warranty.
msg241609 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2015-04-20 06:04
I was (silently) hoping that this patching would eventually not be necessary anymore because the one place (currently inspect.isgenerator()) would be adapted to check for the generator protocol rather than the generator type. But that was going to go into a separate ticket. :)

I'm not sure inspect.isgenerator() is the right place, though, as the module tends to deal with types, not protocols. I think the right fix overall would be a Generator ABC class that defines the protocol.

What Cython does now in order to make asyncio work with Cython compiled generators is this:

https://github.com/cython/cython/blob/4af42443bd37f6207143f8870904f780a65bb3e3/Cython/Utility/Generator.c#L824

https://github.com/cython/cython/blob/4af42443bd37f6207143f8870904f780a65bb3e3/Cython/Utility/Generator.c#L906

Aweful, but currently required due to the type check, which prevents objects that implement the generator protocol from being handled correctly by asyncio.
msg241676 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2015-04-20 19:37
I created issue 24018 for adding a Generator ABC to collections.abc.
msg243782 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-05-21 19:36
This issue was resolved in https://github.com/python/asyncio/commit/3a09a93277afc2cdb43badf92a2c85c2789813f6.
msg244453 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2015-05-30 09:26
I found one more place in asyncio.coroutines, around line 190 in the coroutine() decorator:

    if inspect.isgeneratorfunction(func):
        coro = func
    else:
        @functools.wraps(func)
        def coro(*args, **kw):
            res = func(*args, **kw)
            if isinstance(res, futures.Future) or inspect.isgenerator(res):
                res = yield from res
            return res

The wrapped isgenerator() check should be replaced by an Awaitable ABC check, e.g. this works for me:

"""
             res = func(*args, **kw)
             if isinstance(res, futures.Future) or inspect.isgenerator(res):
                 res = yield from res
+            else:
+                await_res =  getattr(res, '__await__', None)
+                if await_res is not None:
+                    res = yield from await_res()
             return res
"""
msg244459 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-05-30 14:16
What inspect.isgeneratorfunction(func) returns for Cython generators?
msg244474 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2015-05-30 17:56
Cython functions that return a generator aren't special in any way, so it's actually correct that isgeneratorfunction() always returns False. I mean, I could set the CO_COROUTINE flag on the code object that we fake, but that wouldn't help much as Cython's functions are not Python functions, so isfunction() will also return False for them and no-one would look at the flag in the first place. And finally, a Cython generator is not a Python generator (with byte code position etc.), so isgenerator() also correctly returns False.

At least iscoroutine() and iswaitable() will return True for Cython Coroutines now.
msg244479 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2015-05-30 18:18
Hmm, I just noticed that this only started failing when I disabled the asyncio module patching for 3.5beta1+, assuming that it now all works. A side effect of that was that also the inspect module is no longer patched into returning True from isgenerator() for Cython generators. While there might be code that fails when it tries to inspect Cython generators as Python generators (as the first don't have byte code), I think it's still the easiest fix to simply keep the inspect patching. That would then trigger the right code path here without changes in asyncio.
msg244482 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2015-05-30 18:28
... except that the returned object may not actually be a Cython generator but a GeneratorWrapper, now that the patch from issue 24316 is in place. :)

Then I guess we're back to the point where duck-typing and calling __await__() is a good idea.
msg244488 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-05-30 20:01
Stefan,

> Then I guess we're back to the point where duck-typing and calling __await__() is a good idea.

Please test the attached patch.  I agree this is a good idea.
msg244497 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2015-05-30 21:00
Works for me.

Why is the AwaitableABC type check needed, in addition to looking up the relevant method? IIUC, the type check will just do a lookup of the same method if the type hasn't been registered as Awaitable explicitly.
msg244498 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-05-30 21:01
> Why is the AwaitableABC type check needed, in addition to looking up the relevant method? IIUC, the type check will just do a lookup of the same method if the type hasn't been registered as Awaitable explicitly.

Because __await__ should be defined on the class, not on the method (as for all other magic methods in Python).
msg244507 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-05-31 01:02
New changeset b7b73029c825 by Yury Selivanov in branch '3.4':
Issue 24004: Support Awaitables (pep 492) in @asyncio.coroutine decorator
https://hg.python.org/cpython/rev/b7b73029c825

New changeset d1959cafc68c by Yury Selivanov in branch '3.5':
Issue 24004: Support Awaitables (pep 492) in @asyncio.coroutine decorator
https://hg.python.org/cpython/rev/d1959cafc68c

New changeset 6c53591d589b by Yury Selivanov in branch 'default':
Issue 24004: Support Awaitables (pep 492) in @asyncio.coroutine decorator
https://hg.python.org/cpython/rev/6c53591d589b
msg244508 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-05-31 01:05
New changeset 5f1e24f083c7 by Yury Selivanov in branch '3.5':
Issue 24004: Add a unittest for @asyncio.coroutine supporting Awaitables
https://hg.python.org/cpython/rev/5f1e24f083c7

New changeset 9d261141eb0c by Yury Selivanov in branch 'default':
Issue 24004: Add a unittest for @asyncio.coroutine supporting Awaitables
https://hg.python.org/cpython/rev/9d261141eb0c
msg244524 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-05-31 08:00
Yury, your last change causes DeprecationWarning:

[ 38/398] test_asyncio
/media/disk/home/proj/python/cpython/Lib/test/test_asyncio/test_pep492.py:119: DeprecationWarning: Please use assertEqual instead.
  self.assertEquals(coro.send(None), 'spam')
msg244543 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-05-31 15:29
New changeset 8296f0119f20 by Yury Selivanov in branch '3.5':
Issue 24004: Fix DeprecationWarning in a unittest
https://hg.python.org/cpython/rev/8296f0119f20

New changeset 60f5091cbfbf by Yury Selivanov in branch 'default':
Issue 24004: Fix DeprecationWarning in a unittest
https://hg.python.org/cpython/rev/60f5091cbfbf
msg244544 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2015-05-31 15:29
Thanks, Martin!
History
Date User Action Args
2015-05-31 15:29:12yselivanovsetmessages: + msg244544
2015-05-31 15:29:05python-devsetmessages: + msg244543
2015-05-31 08:00:02martin.pantersetnosy: + martin.panter
messages: + msg244524
2015-05-31 01:06:23yselivanovsetstatus: open -> closed
resolution: fixed
2015-05-31 01:05:11python-devsetmessages: + msg244508
2015-05-31 01:03:00python-devsetnosy: + python-dev
messages: + msg244507
2015-05-30 21:01:56yselivanovsetmessages: + msg244498
2015-05-30 21:00:37scodersetstatus: pending -> open

messages: + msg244497
2015-05-30 20:02:03yselivanovsetstatus: closed -> pending
resolution: fixed -> (no value)
2015-05-30 20:01:44yselivanovsetfiles: + coroutine.patch
keywords: + patch
messages: + msg244488
2015-05-30 18:28:37scodersetmessages: + msg244482
2015-05-30 18:18:52scodersetmessages: + msg244479
2015-05-30 17:56:27scodersetmessages: + msg244474
2015-05-30 14:16:40yselivanovsetmessages: + msg244459
2015-05-30 09:26:32scodersetmessages: + msg244453
2015-05-21 19:36:03yselivanovsetstatus: open -> closed
versions: - Python 3.4
messages: + msg243782

resolution: fixed
stage: resolved
2015-04-20 19:37:04scodersetmessages: + msg241676
2015-04-20 06:04:55scodersetmessages: + msg241609
2015-04-20 03:32:39gvanrossumsetmessages: + msg241595
2015-04-19 10:21:02scodercreate