Confusing error during cyclic yield

[MaxvonTettenborn]

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]

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

[gvanrossum]

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]

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]

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]

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]

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

[1st1]

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]

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

[gvanrossum]

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]

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]

Thank you for reporting this! Closing the issue.

[MaxvonTettenborn]

You are very welcome, glad I could help.