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

avoid explicit generator type check in asyncio #68192

Closed
scoder opened this issue Apr 19, 2015 · 18 comments
Closed

avoid explicit generator type check in asyncio #68192

scoder opened this issue Apr 19, 2015 · 18 comments
Labels
topic-asyncio type-bug An unexpected behavior, bug, or error

Comments

@scoder
Copy link
Contributor

scoder commented Apr 19, 2015

BPO 24004
Nosy @gvanrossum, @scoder, @vstinner, @vadmium, @1st1
Files
  • coroutine.patch
  • 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:

    assignee = None
    closed_at = <Date 2015-05-31.01:06:23.847>
    created_at = <Date 2015-04-19.10:21:02.253>
    labels = ['type-bug', 'expert-asyncio']
    title = 'avoid explicit generator type check in asyncio'
    updated_at = <Date 2015-05-31.15:29:12.912>
    user = 'https://github.com/scoder'

    bugs.python.org fields:

    activity = <Date 2015-05-31.15:29:12.912>
    actor = 'yselivanov'
    assignee = 'none'
    closed = True
    closed_date = <Date 2015-05-31.01:06:23.847>
    closer = 'yselivanov'
    components = ['asyncio']
    creation = <Date 2015-04-19.10:21:02.253>
    creator = 'scoder'
    dependencies = []
    files = ['39569']
    hgrepos = []
    issue_num = 24004
    keywords = ['patch']
    message_count = 18.0
    messages = ['241506', '241595', '241609', '241676', '243782', '244453', '244459', '244474', '244479', '244482', '244488', '244497', '244498', '244507', '244508', '244524', '244543', '244544']
    nosy_count = 6.0
    nosy_names = ['gvanrossum', 'scoder', 'vstinner', 'python-dev', 'martin.panter', 'yselivanov']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue24004'
    versions = ['Python 3.5']

    @scoder
    Copy link
    Contributor Author

    scoder commented Apr 19, 2015

    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.

    @scoder scoder added topic-asyncio type-bug An unexpected behavior, bug, or error labels Apr 19, 2015
    @gvanrossum
    Copy link
    Member

    Uh, wait. Who's patching anything? That breaks the warranty.

    @scoder
    Copy link
    Contributor Author

    scoder commented Apr 20, 2015

    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.

    @scoder
    Copy link
    Contributor Author

    scoder commented Apr 20, 2015

    I created bpo-24018 for adding a Generator ABC to collections.abc.

    @1st1
    Copy link
    Member

    1st1 commented May 21, 2015

    This issue was resolved in python/asyncio@3a09a93.

    @1st1 1st1 closed this as completed May 21, 2015
    @scoder
    Copy link
    Contributor Author

    scoder commented May 30, 2015

    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
    """

    @1st1
    Copy link
    Member

    1st1 commented May 30, 2015

    What inspect.isgeneratorfunction(func) returns for Cython generators?

    @scoder
    Copy link
    Contributor Author

    scoder commented May 30, 2015

    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.

    @scoder
    Copy link
    Contributor Author

    scoder commented May 30, 2015

    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.

    @scoder
    Copy link
    Contributor Author

    scoder commented May 30, 2015

    ... except that the returned object may not actually be a Cython generator but a GeneratorWrapper, now that the patch from bpo-24316 is in place. :)

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

    @1st1
    Copy link
    Member

    1st1 commented May 30, 2015

    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.

    @1st1 1st1 reopened this May 30, 2015
    @scoder
    Copy link
    Contributor Author

    scoder commented May 30, 2015

    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.

    @1st1
    Copy link
    Member

    1st1 commented May 30, 2015

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

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 31, 2015

    New changeset b7b73029c825 by Yury Selivanov in branch '3.4':
    bpo-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':
    bpo-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':
    bpo-24004: Support Awaitables (PEP-492) in @asyncio.coroutine decorator
    https://hg.python.org/cpython/rev/6c53591d589b

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 31, 2015

    New changeset 5f1e24f083c7 by Yury Selivanov in branch '3.5':
    bpo-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':
    bpo-24004: Add a unittest for @asyncio.coroutine supporting Awaitables
    https://hg.python.org/cpython/rev/9d261141eb0c

    @1st1 1st1 closed this as completed May 31, 2015
    @vadmium
    Copy link
    Member

    vadmium commented May 31, 2015

    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')

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 31, 2015

    New changeset 8296f0119f20 by Yury Selivanov in branch '3.5':
    bpo-24004: Fix DeprecationWarning in a unittest
    https://hg.python.org/cpython/rev/8296f0119f20

    New changeset 60f5091cbfbf by Yury Selivanov in branch 'default':
    bpo-24004: Fix DeprecationWarning in a unittest
    https://hg.python.org/cpython/rev/60f5091cbfbf

    @1st1
    Copy link
    Member

    1st1 commented May 31, 2015

    Thanks, Martin!

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    topic-asyncio type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants