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

awaiting asyncio.Future swallows StopIteration #70409

Closed
ikelly mannequin opened this issue Jan 27, 2016 · 20 comments
Closed

awaiting asyncio.Future swallows StopIteration #70409

ikelly mannequin opened this issue Jan 27, 2016 · 20 comments
Labels
expert-asyncio type-bug An unexpected behavior, bug, or error

Comments

@ikelly
Copy link
Mannequin

ikelly mannequin commented Jan 27, 2016

BPO 26221
Nosy @gvanrossum, @Rosuav, @1st1
Files
  • no_stop_iter.patch
  • no_stop_iter.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 2016-03-02.16:04:43.564>
    created_at = <Date 2016-01-27.17:25:33.665>
    labels = ['type-bug', 'expert-asyncio']
    title = 'awaiting asyncio.Future swallows StopIteration'
    updated_at = <Date 2016-03-02.16:04:43.542>
    user = 'https://bugs.python.org/ikelly'

    bugs.python.org fields:

    activity = <Date 2016-03-02.16:04:43.542>
    actor = 'yselivanov'
    assignee = 'none'
    closed = True
    closed_date = <Date 2016-03-02.16:04:43.564>
    closer = 'yselivanov'
    components = ['asyncio']
    creation = <Date 2016-01-27.17:25:33.665>
    creator = 'ikelly'
    dependencies = []
    files = ['41980', '41986']
    hgrepos = []
    issue_num = 26221
    keywords = ['patch']
    message_count = 20.0
    messages = ['259036', '259041', '259044', '259045', '259049', '259056', '259285', '260563', '260574', '260577', '260584', '260590', '260591', '260597', '260616', '260618', '260622', '260638', '261117', '261118']
    nosy_count = 5.0
    nosy_names = ['gvanrossum', 'ikelly', 'python-dev', 'Rosuav', 'yselivanov']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue26221'
    versions = ['Python 3.5']

    @ikelly
    Copy link
    Mannequin Author

    ikelly mannequin commented Jan 27, 2016

    I was playing around with this class for adapting regular iterators to async iterators using BaseEventLoop.run_in_executor:

    import asyncio
    
    class AsyncIteratorWrapper:
    
        def __init__(self, iterable, loop=None, executor=None):
            self._iterator = iter(iterable)
            self._loop = loop or asyncio.get_event_loop()
            self._executor = executor
    
        async def __aiter__(self):
            return self
    
        async def __anext__(self):
            try:
                return await self._loop.run_in_executor(
                        self._executor, next, self._iterator)
            except StopIteration:
                raise StopAsyncIteration

    Unfortunately this fails because when next raises StopIteration, run_in_executor swallows the exception and just returns None back to the coroutine, resulting in an infinite iterator of Nones.

    @ikelly ikelly mannequin added the expert-asyncio label Jan 27, 2016
    @gvanrossum
    Copy link
    Member

    gvanrossum commented Jan 27, 2016

    What are you trying to do here? Can you post a simple example of an iterator that you would like to use with this? Without that it just raises my hackles -- it seems totally wrong to run an iterator in another thread. (Or is the iterator really a coroutine/future?)

    @ikelly
    Copy link
    Mannequin Author

    ikelly mannequin commented Jan 27, 2016

    The idea is that the wrapped iterator is something potentially blocking, like a database cursor that doesn't natively support asyncio. Usage would be something like this:

    async def get_data():
        cursor.execute('select * from stuff')
        async for row in AsyncIteratorWrapper(cursor):
            process(row)

    Investigating this further, I think the problem is actually in await, not run_in_executor:

    >>> async def test():
    ...     fut = asyncio.Future()
    ...     fut.set_exception(StopIteration())
    ...     print(await fut)
    ... 
    >>> loop.run_until_complete(test())
    None

    @gvanrossum
    Copy link
    Member

    gvanrossum commented Jan 27, 2016

    StopIteration has a special meaning. Don't use set_exception() with it.

    You probably need a more roundabout way to do this.

    Instead of submitting each __next__() call to the executor separately, you should submit something to the executor that pulls the items from the iterator and sticks them into a queue; then on the asyncio side you pull them out of the queue.

    You can use an asyncio.Queue as the queue, and use loop.call_soon_threadsafe() to put things into that queue from the tread.

    @ikelly
    Copy link
    Mannequin Author

    ikelly mannequin commented Jan 27, 2016

    Fair enough. I think there should be some documentation though to the effect that coroutines aren't robust to passing StopIteration across coroutine boundaries. It's particularly surprising with PEP-492 coroutines, since those aren't even iterators and intuitively should ignore StopIteration like normal functions do.

    As it happens, this variation (moving the try-except into the executor thread) does turn out to work but is probably best avoided for the same reason. I don't think it's obviously bad code though:

    class AsyncIteratorWrapper:
    
        def __init__(self, iterable, loop=None, executor=None):
            self._iterator = iter(iterable)
            self._loop = loop or asyncio.get_event_loop()
            self._executor = executor
    
        async def __aiter__(self):
            return self
    
        async def __anext__(self):
            def _next(iterator):
                try:
                    return next(iterator)
                except StopIteration:
                    raise StopAsyncIteration
            return await self._loop.run_in_executor(
                    self._executor, _next, self._iterator)

    @gvanrossum
    Copy link
    Member

    gvanrossum commented Jan 27, 2016

    Can you suggest a sentence to insert into the docs and a place where
    to insert it? (As you can imagine I'm pretty blind for such issues
    myself.)

    @ikelly
    Copy link
    Mannequin Author

    ikelly mannequin commented Jan 31, 2016

    The place I'd expect to find it is in https://docs.python.org/3/library/asyncio-task.html#coroutines, in the list of "things a coroutine can do". The first two bullets in the list say that any exceptions raised will be propagated. Maybe there should be a note after the bullet list to the effect that "StopIteration carries special meaning to coroutines and will not be propagated if raised by an awaited coroutine or future."

    @ikelly
    Copy link
    Mannequin Author

    ikelly mannequin commented Feb 20, 2016

    Chris Angelico suggested on python-list that another possibly useful thing to do would be to add a "from __future__ import generator_stop" to asyncio/futures.py. This would at least have the effect of causing "await future" to raise a RuntimeError instead of silently returning None if a StopIteration is set on the future. Future.__iter__ is the only generator in the file, so this change shouldn't have any other effects.

    @ikelly ikelly mannequin changed the title asynco run_in_executor swallows StopIteration awaiting asyncio.Future swallows StopIteration Feb 20, 2016
    @gvanrossum
    Copy link
    Member

    gvanrossum commented Feb 20, 2016

    Chris, can you help out here? I still don't understand the issue here. Since "from __future__ import generator_stop" only works in 3.5+ it would not work in Python 3.3/3.4 (supported by upstream asyncio with literally the same source code currently). If there's no use case for f.set_exception(StopIteration) maybe we should just complain about that?

    @Rosuav
    Copy link
    Contributor

    Rosuav commented Feb 20, 2016

    Ultimately, it's the exact same thing that PEP-479 is meant to deal with - raising StopIteration is functionally identical to returning. I don't use asyncio enough to be certain, but I'm not aware of any good reason to inject a StopIteration into it; maybe an alternative solution is to add a check in set_exception "if isinstance(exception, StopIteration): raise DontBeAFool"?

    @gvanrossum
    Copy link
    Member

    gvanrossum commented Feb 21, 2016

    OK, since eventually there won't be a way to inject StopIteration into
    a Future anyway (it'll always raise RuntimeError once PEP-479 is the
    default behavior) we should just reject this in set_exception().

    @Rosuav
    Copy link
    Contributor

    Rosuav commented Feb 21, 2016

    POC patch, no tests. Is TypeError right? Should it be ValueError, since the notional type is "Exception"?

    @gvanrossum
    Copy link
    Member

    gvanrossum commented Feb 21, 2016

    I think TypeError is fine. I would make the message a bit longer to
    explain carefully what's the matter.

    @Rosuav
    Copy link
    Contributor

    Rosuav commented Feb 21, 2016

    How about "StopException interacts badly with generators and cannot be raised into a Future"?

    @gvanrossum
    Copy link
    Member

    gvanrossum commented Feb 21, 2016

    S.G.T.M.

    On Sunday, February 21, 2016, Chris Angelico <report@bugs.python.org> wrote:

    Chris Angelico added the comment:

    How about "StopException interacts badly with generators and cannot be
    raised into a Future"?

    ----------


    Python tracker <report@bugs.python.org <javascript:;>>
    <http://bugs.python.org/issue26221\>


    @Rosuav
    Copy link
    Contributor

    Rosuav commented Feb 21, 2016

    Wording changed, and a simple test added. I'm currently seeing failures in test_site, but that probably means I've messed something up on my system.

    @gvanrossum
    Copy link
    Member

    gvanrossum commented Feb 21, 2016

    Would you mind reworking this as a PR for github.com/python/asyncio ?
    That's still considered "upstream" for asyncio.

    --Guido

    On Sun, Feb 21, 2016 at 8:17 AM, Chris Angelico <report@bugs.python.org> wrote:

    Chris Angelico added the comment:

    Wording changed, and a simple test added. I'm currently seeing failures in test_site, but that probably means I've messed something up on my system.

    ----------
    Added file: http://bugs.python.org/file41986/no_stop_iter.patch


    Python tracker <report@bugs.python.org>
    <http://bugs.python.org/issue26221\>


    @Rosuav
    Copy link
    Contributor

    Rosuav commented Feb 21, 2016

    Opened python/asyncio#322

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 2, 2016

    New changeset ef5265bc07bb by Yury Selivanov in branch '3.5':
    asyncio: Prevent StopIteration from being thrown into a Future
    https://hg.python.org/cpython/rev/ef5265bc07bb

    New changeset 5e2f7e51af51 by Yury Selivanov in branch 'default':
    Merge 3.5 (issue bpo-26221)
    https://hg.python.org/cpython/rev/5e2f7e51af51

    @1st1
    Copy link
    Member

    1st1 commented Mar 2, 2016

    Merged.

    @1st1 1st1 closed this as completed Mar 2, 2016
    @1st1 1st1 added the type-bug An unexpected behavior, bug, or error label Mar 2, 2016
    @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
    expert-asyncio type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants