asyncio queue: get/put will not awaken next if interrupted

[orbisvicis]

Checklist

• I am confident this is a bug in CPython, not a bug in a third-party project
• I have searched the CPython issue tracker,
  and am confident this bug has not been reported before

CPython versions tested on:

CPython main branch

Operating systems tested on:

Linux

Output from running 'python -VV' on the command line:

Python 3.11.4 (main, Jun 9 2023, 07:59:55) [GCC 12.3.0]

A clear and concise description of the bug:

If the current putter has an exception thrown (such as when the task awaiting put() has an exception thrown) and the queue isn't full (which can happen if another task put()s before the current task is rewoken), then the queue must wake up the next putter, because otherwise nothing else will wake up the next putter. This line is responsible for this:

cpython/Lib/asyncio/queues.py

Line 130 in 42429d3

if not self.full() and not putter.cancelled():

Unfortunately that condition - specifically not putter.cancelled() - will never be met because just before, this:

cpython/Lib/asyncio/queues.py

Line 122 in 42429d3

putter.cancel() # Just in case putter is not done yet.

This affects both put() and get(). I propose moving cancel() to just before the raise.

There is no test case - this is something I noticed while reading asyncio/queue.py.

Linked PRs

• gh-108042: asyncio/queues: clarify comments #108066

[orbisvicis]

The code is correct; it's the documentation that's lacking. The condition in question catches a mismatch between the awaited result and the future's result. Imagine a chain of awaited futures, ex. "await Fut(await Fut(await Fut(await Fut())))". Cancelling any future propagates the error down the chain of await, cancelling each future, and then back up, returning CancelledError. Except of course, when the future already has a result set. This example best illustrates the situation:

import asyncio

async def task_wait(f):
    return await f

async def task_result(fs):
    await asyncio.sleep(1)
    fs[0].set_result(10)
    fs[2].cancel()

async def main():
    fs = [asyncio.Future()]
    for i in range(5):
        f = asyncio.create_task(task_wait(fs[-1]))
        fs.append(f)
    t = asyncio.create_task(task_result(fs))
    try:
        v = await fs[-1]
    except asyncio.CancelledError as e:
        print("main: await: cancelled")
    else:
        print(v)
    print([f.cancelled() for f in fs])

asyncio.run(main())

>>> main: await: cancelled
>>> [False, True, True, True, True, True]

The exact same situation can happen if get_nowait() sets a result on a putter, but then another task (indirectly) cancels the very same putter - a CancelledError is raised, but putter.cancelled() is not true.

task1: put(1)
task2: put(2)
task3: get_nowait() -> task1.putter.set_result()
task4: cancel task1
task1: except CancelledError but not putter.cancelled() -> wake next putter

If the converse occurs it isn't necessary to wake the next putter - if the putter is first cancelled, then get_nowait will simply skip it.

task1: put(1)
task2: put(2)
task4: cancel task1
task3: get_nowait() -> skip task1 -> task2.putter.set_result()

[orbisvicis]

Please change the labels. This isn't a bug. This may not be a documentation issue - the public documentation is correct. Possibly some internal comments could use clarification. I'm not sure how obvious this is to others. Feel free to close if you think there's no need to improve the comments.

[Hels15]

I'm a bit unsure about what you're getting at. If the docs are right and there's no bug, maybe we could just throw in some comments in the code to make it clear for everyone else?

[Hels15]

if so, maybe you can open a pr with the improved comments.

[orbisvicis]

OK but I don't want to make it too verbose, ie not a copy-and-paste of my first comment. How's this:

# We were woken up by put_nowait(), but can't take the
# call. Wake up the next in line.
# 
# This can happen when put_nowait() sets a result on the
# getter, but then another task (indirectly) cancels the
# very same getter - a CancelledError is raised, but since
# a result is already set getter.cancelled() cannot be
# true.
#
# If the converse occurs it isn't necessary to wake the
# next getter - if the getter is first cancelled, then
# put_nowait() will simply skip it.
#
# See github/cpython/issues/108042 for more information.

As an improvement over:

# We were woken up by put_nowait(), but can't take
# the call.  Wake up the next in line.

[gvanrossum]

I'm not sure we need to add such a verbose comment. Sure, the code is subtle. But so is a lot of asyncio internals. I'm closing this as won't fix. (You can still submit a PR with a better comment, but please a lot shorter. Enough that your former self wouldn't have filed a bug. :-)