classification
Title: Confusing error during cyclic yield
Type: behavior Stage: resolved
Components: asyncio Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Max von Tettenborn, gvanrossum, python-dev, vstinner, yselivanov
Priority: normal Keywords:

Created on 2016-09-06 10:16 by Max von Tettenborn, last changed 2017-03-31 16:36 by dstufft. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 552 closed dstufft, 2017-03-31 16:36
Messages (12)
msg274547 - (view) Author: Max von Tettenborn (Max von Tettenborn) Date: 2016-09-06 10:16
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())
msg274697 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2016-09-07 01:35
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)
msg278203 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-10-06 18:50
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?
msg278215 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2016-10-06 21:47
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?
msg278216 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-10-06 21:50
> 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".
msg278217 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2016-10-06 21:53
Is that enough? What if the recursion involves several tasks waiting
for each other in a cycle?
msg278218 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-10-06 21:58
> 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.
msg278219 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2016-10-06 22:13
Maybe it could be fixed rather than making this a checked failure?
msg278246 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2016-10-07 14:35
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.
msg278371 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-10-09 16:21
New changeset 8d877876aa89 by Yury Selivanov in branch '3.5':
Issue #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 #27972)
https://hg.python.org/cpython/rev/41c4f535b5c0

New changeset 47720192b318 by Yury Selivanov in branch 'default':
Merge 3.6 (issue #27972)
https://hg.python.org/cpython/rev/47720192b318
msg278372 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2016-10-09 16:22
Thank you for reporting this! Closing the issue.
msg278407 - (view) Author: Max von Tettenborn (Max von Tettenborn) Date: 2016-10-10 09:11
You are very welcome, glad I could help.
History
Date User Action Args
2017-03-31 16:36:32dstufftsetpull_requests: + pull_request1051
2016-10-10 09:11:22Max von Tettenbornsetmessages: + msg278407
2016-10-09 16:22:05yselivanovsetstatus: open -> closed
resolution: fixed
messages: + msg278372

stage: resolved
2016-10-09 16:21:21python-devsetnosy: + python-dev
messages: + msg278371
2016-10-07 14:35:47gvanrossumsetmessages: + msg278246
2016-10-06 22:13:25gvanrossumsetmessages: + msg278219
2016-10-06 21:58:37yselivanovsetmessages: + msg278218
2016-10-06 21:53:45gvanrossumsetmessages: + msg278217
2016-10-06 21:50:02yselivanovsetmessages: + msg278216
2016-10-06 21:47:53gvanrossumsetmessages: + msg278215
2016-10-06 18:50:04yselivanovsetmessages: + msg278203
2016-09-07 01:35:01gvanrossumsetmessages: + msg274697
2016-09-06 10:16:54Max von Tettenborncreate