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

Confusing error during cyclic yield #72159

Closed
MaxvonTettenborn mannequin opened this issue Sep 6, 2016 · 12 comments
Closed

Confusing error during cyclic yield #72159

MaxvonTettenborn mannequin opened this issue Sep 6, 2016 · 12 comments
Labels
topic-asyncio type-bug An unexpected behavior, bug, or error

Comments

@MaxvonTettenborn
Copy link
Mannequin

MaxvonTettenborn mannequin commented Sep 6, 2016

BPO 27972
Nosy @gvanrossum, @vstinner, @1st1
PRs
  • [Do Not Merge] Convert Misc/NEWS so that it is managed by towncrier #552
  • 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-10-09.16:22:05.899>
    created_at = <Date 2016-09-06.10:16:54.827>
    labels = ['type-bug', 'expert-asyncio']
    title = 'Confusing error during cyclic yield'
    updated_at = <Date 2017-03-31.16:36:32.309>
    user = 'https://bugs.python.org/MaxvonTettenborn'

    bugs.python.org fields:

    activity = <Date 2017-03-31.16:36:32.309>
    actor = 'dstufft'
    assignee = 'none'
    closed = True
    closed_date = <Date 2016-10-09.16:22:05.899>
    closer = 'yselivanov'
    components = ['asyncio']
    creation = <Date 2016-09-06.10:16:54.827>
    creator = 'Max von Tettenborn'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 27972
    keywords = []
    message_count = 12.0
    messages = ['274547', '274697', '278203', '278215', '278216', '278217', '278218', '278219', '278246', '278371', '278372', '278407']
    nosy_count = 5.0
    nosy_names = ['gvanrossum', 'vstinner', 'python-dev', 'yselivanov', 'Max von Tettenborn']
    pr_nums = ['552']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue27972'
    versions = ['Python 3.5']

    @MaxvonTettenborn
    Copy link
    Mannequin Author

    MaxvonTettenborn mannequin commented Sep 6, 2016

    Below code reproduces the problem. The resulting error is a RecursionError and it is very hard to trace that to the cause of the problem, which is the runner task and the stop task yielding from each other, forming a deadlock.

    I think, an easy to make mistake like that should raise a clearer exception. And maybe I am mistaken, but it should in principle be possible for the event loop to detect a cyclic yield, right?

    import asyncio
    
    
    class A:
        @asyncio.coroutine
        def start(self):
            self.runner_task = asyncio.ensure_future(self.runner())
    
        @asyncio.coroutine
        def stop(self):
            self.runner_task.cancel()
            yield from self.runner_task
    
        @asyncio.coroutine
        def runner(self):
            try:
                while True:
                    yield from asyncio.sleep(5)
            except asyncio.CancelledError:
                yield from self.stop()
                return
    
    
    def do_test():
        @asyncio.coroutine
        def f():
            a = A()
            yield from a.start()
            yield from asyncio.sleep(1)
            yield from a.stop()
    asyncio.get_event_loop().run_until_complete(f())
    

    @MaxvonTettenborn MaxvonTettenborn mannequin added topic-asyncio type-bug An unexpected behavior, bug, or error labels Sep 6, 2016
    @gvanrossum
    Copy link
    Member

    Thanks for the report. I've never seen this before, so I doubt it is a
    common mistake. Yury have you ever seen this?

    --Guido (mobile)

    @1st1
    Copy link
    Member

    1st1 commented Oct 6, 2016

    This is an interesting mind twister. The key problem is that self.runner_task is blocked on itself: so Task._fut_waiter is set to the Task.

    Therefore when the task is being cancelled, Task.cancel simply recurses.

    One way to solve this is to prohibit tasks to await on themselves. Right now the following code "kind of" works:

      async def f():
        loop.call_later(1, lambda: t.set_result(42))
        return await t
    
      loop = asyncio.get_event_loop()
      t = loop.create_task(f())
      print(loop.run_until_complete(t))

    albeit it logs errors about invalid Future state.

    My proposal is to raise a ValueError if Task is asked to await on itself.

    Guido, what do you think?

    @gvanrossum
    Copy link
    Member

    It's pretty perverse. But how would you detect this case? Does it
    require changes to CPython or only to asyncio? Does it require a spec
    change anywhere?

    @1st1
    Copy link
    Member

    1st1 commented Oct 6, 2016

    It's pretty perverse. But how would you detect this case?

    In Task._step, we can check if the future the task is about to await on is "self".

    @gvanrossum
    Copy link
    Member

    Is that enough? What if the recursion involves several tasks waiting
    for each other in a cycle?

    @1st1
    Copy link
    Member

    1st1 commented Oct 6, 2016

    Is that enough? What if the recursion involves several tasks waiting
    for each other in a cycle?

    I'm not sure... Maybe it's OK when two tasks await on each other, I think the current Task implementation should be able to handle that. The problem with the Task awaiting itself is that the current implementation just crashes with a RecursionError.

    @gvanrossum
    Copy link
    Member

    Maybe it could be fixed rather than making this a checked failure?

    @gvanrossum
    Copy link
    Member

    I've meditated on this and I've changed my mind. A task that awaits
    itself is so obviously not following the task protocol that it should
    be shot on sight. No exceptions. (Only the task itself should ever set
    the result or exception, and *only* by returning or raising. So the
    only way a task waiting for itself could be unblocked is through
    cancellation, and there are better ways to wait forever until
    cancelled.)

    IOW @1st1 I think it's okay to detect this and raise an exception.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 9, 2016

    New changeset 8d877876aa89 by Yury Selivanov in branch '3.5':
    Issue bpo-27972: Prohibit Tasks to await on themselves.
    https://hg.python.org/cpython/rev/8d877876aa89

    New changeset 41c4f535b5c0 by Yury Selivanov in branch '3.6':
    Merge 3.5 (issue bpo-27972)
    https://hg.python.org/cpython/rev/41c4f535b5c0

    New changeset 47720192b318 by Yury Selivanov in branch 'default':
    Merge 3.6 (issue bpo-27972)
    https://hg.python.org/cpython/rev/47720192b318

    @1st1
    Copy link
    Member

    1st1 commented Oct 9, 2016

    Thank you for reporting this! Closing the issue.

    @1st1 1st1 closed this as completed Oct 9, 2016
    @MaxvonTettenborn
    Copy link
    Mannequin Author

    MaxvonTettenborn mannequin commented Oct 10, 2016

    You are very welcome, glad I could help.

    @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

    2 participants