Skip to content

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

Closed as not planned

Description

@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:

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

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

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

Activity

  1. orbisvicis commented on Aug 16, 2023

    @orbisvicis
    Author

    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()
    
  2. orbisvicis commented on Aug 16, 2023

    @orbisvicis
    Author

    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.

  3. removed
    type-bugAn unexpected behavior, bug, or error
    on Aug 16, 2023
  4. Hels15 commented on Aug 16, 2023

    @Hels15
    Contributor

    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?

  5. Hels15 commented on Aug 16, 2023

    @Hels15
    Contributor

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

  6. orbisvicis commented on Aug 16, 2023

    @orbisvicis
    Author

    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.
    
  7. gvanrossum commented on Aug 16, 2023

    @gvanrossum
    Member

    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. :-)

  8. moved this from Todo to Done in asyncioon Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Projects

    • Status
      Done

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions