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

asyncio.Task doesn't propagate CancelledError() exception correctly. #89553

Closed
pagliariccim mannequin opened this issue Oct 6, 2021 · 20 comments
Closed

asyncio.Task doesn't propagate CancelledError() exception correctly. #89553

pagliariccim mannequin opened this issue Oct 6, 2021 · 20 comments
Labels
3.11 bug and security fixes topic-asyncio type-bug An unexpected behavior, bug, or error

Comments

@pagliariccim
Copy link
Mannequin

pagliariccim mannequin commented Oct 6, 2021

BPO 45390
Nosy @gvanrossum, @asvetlov, @cjerdonek, @serhiy-storchaka, @1st1, @graingert, @bensimner
PRs
  • bpo-45390: Propagate CancelledError's message from cancelled task to its awaiter #31383
  • Files
  • task_bug.py
  • task_bug.py
  • task_bug.py
  • 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 2022-02-22.09:46:40.916>
    created_at = <Date 2021-10-06.11:30:42.256>
    labels = ['3.11', 'type-bug', 'expert-asyncio']
    title = "asyncio.Task doesn't propagate CancelledError() exception correctly."
    updated_at = <Date 2022-02-23.18:04:42.655>
    user = 'https://bugs.python.org/pagliariccim'

    bugs.python.org fields:

    activity = <Date 2022-02-23.18:04:42.655>
    actor = 'gvanrossum'
    assignee = 'none'
    closed = True
    closed_date = <Date 2022-02-22.09:46:40.916>
    closer = 'asvetlov'
    components = ['asyncio']
    creation = <Date 2021-10-06.11:30:42.256>
    creator = 'pagliaricci.m'
    dependencies = []
    files = ['50328', '50333', '50334']
    hgrepos = []
    issue_num = 45390
    keywords = ['patch']
    message_count = 20.0
    messages = ['403294', '403297', '403350', '403360', '403521', '403530', '403531', '403532', '403533', '403570', '413392', '413395', '413397', '413679', '413779', '413794', '413808', '413830', '413836', '413838']
    nosy_count = 8.0
    nosy_names = ['gvanrossum', 'asvetlov', 'chris.jerdonek', 'serhiy.storchaka', 'yselivanov', 'graingert', 'bjs', 'pagliaricci.m']
    pr_nums = ['31383']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue45390'
    versions = ['Python 3.11']

    @pagliariccim
    Copy link
    Mannequin Author

    pagliariccim mannequin commented Oct 6, 2021

    I've spotted a little bug in how asyncio.CancelledError() exception is propagated inside an asyncio.Task.

    Since python 3.9 the asyncio.Task.cancel() method has a new 'msg' parameter, that will create an asyncio.CancelledError(msg) exception incorporating that message.

    The exception is successfully propagated to the coroutine the asyncio.Task is running, so the coroutine successfully gets raised an asyncio.CancelledError(msg) with the specified message in asyncio.Task.cancel(msg) method.

    But, once the asyncio.Task is cancelled, is impossible to retrieve that original asyncio.CancelledError(msg) exception with the message, because it seems that *a new* asyncio.CancelledError() [without the message] is raised when asyncio.Task.result() or asyncio.Task.exception() methods are called.

    I have the feeling that this is just wrong, and that the original message specified in asyncio.Task.cancel(msg) should be propagated even also asyncio.Task.result() is called.

    I'm including a little snippet of code that clearly shows this bug.

    I'm using python 3.9.6, in particular:
    Python 3.9.6 (default, Aug 21 2021, 09:02:49)
    [GCC 10.2.1 20210110] on linux

    @pagliariccim pagliariccim mannequin added 3.9 only security fixes topic-asyncio type-bug An unexpected behavior, bug, or error labels Oct 6, 2021
    @bensimner
    Copy link
    Mannequin

    bensimner mannequin commented Oct 6, 2021

    This seems to be present in both the Python implementation as well as the accelerated C _asyncio module.

    It looks like that when a Task awaits a cancelled future,
    the task itself is cancelled but the cancellation message is not propagated to the task.
    https://github.com/python/cpython/blob/main/Lib/asyncio/tasks.py#L242

    @cjerdonek
    Copy link
    Member

    But, once the asyncio.Task is cancelled, is impossible to retrieve that original asyncio.CancelledError(msg) exception with the message, because it seems that *a new* asyncio.CancelledError() [without the message] is raised when asyncio.Task.result() or asyncio.Task.exception() methods are called.

    You say it's "impossible", but isn't the message accessible via the exception chain (and visible in the traceback)? One benefit of not duplicating the message on the internal call to cancel() is that it makes it easier to pinpoint which CancelledError object is associated with the user's call to cancel(), and which is associated with the call done by asyncio internals, which is a different cancellation. Another benefit is that it prevents info from being duplicated in the traceback.

    @graingert
    Copy link
    Mannequin

    graingert mannequin commented Oct 7, 2021

    afaik this is intentional https://bugs.python.org/issue31033

    @pagliariccim
    Copy link
    Mannequin Author

    pagliariccim mannequin commented Oct 9, 2021

    OK, I see your point.
    But I still can't understand one thing and I think it's very confusing:

    1. if you see my example, inside the job() coroutine, I get correctly
      cancelled with an `asyncio.CancelledError` exception containing my message.
    2. Now: if I re-raise the asyncio.CancelledError as-is, I lose the message,
      if I call the `asyncio.Task.exception()` function.
    3. If I raise a *new* asyncio.CancelledError with a new message, inside the
      job() coroutine's `except asyncio.CancelledError:` block, I still lose the
      message if I call `asyncio.Task.exception()`.
    4. But if I raise another exception, say `raise ValueError("TEST")`, always
      from the `except asyncio.CancelledError:` block of the job() coroutine, I
      *get* the message!
      I get `ValueError("TEST")` by calling `asyncio.Task.exception()`, while I
      don't with the `asyncio.CancelledError()` one.

    Is this really wanted? Sorry, but I still find this a lot confusing.
    Shouldn't it be better to return from the asyncio.Task.exception() the
    old one (containing the message) ?
    Or, otherwise, create a new instance of the exception for ALL the
    exception classes?

    Thank you for your time,
    My Best Regards,

    M.

    On Thu, Oct 7, 2021 at 10:25 AM Thomas Grainger <report@bugs.python.org>
    wrote:

    Thomas Grainger <tagrain@gmail.com> added the comment:

    afaik this is intentional https://bugs.python.org/issue31033

    ----------
    nosy: +graingert


    Python tracker <report@bugs.python.org>
    <https://bugs.python.org/issue45390\>


    @cjerdonek
    Copy link
    Member

    1. Now: if I re-raise the asyncio.CancelledError as-is, I lose the message,
      if I call the asyncio.Task.exception() function.

    Re-raise asyncio.CancelledError where? (And what do you mean by "re-raise"?) Call asyncio.Task.exception() where? This isn't part of your example, so it's not clear what you mean exactly.

    @pagliariccim
    Copy link
    Mannequin Author

    pagliariccim mannequin commented Oct 9, 2021

    Chris,
    I'm attaching to this e-mail the code I'm referring to.
    As you can see, in line 10, I re-raise the asyncio.CancelledError exception
    with a message "TEST".
    That message is lost, due to the reasons we've talked about.

    My point is that, if we substitute that line 10, with the commented line
    11, and we comment the line 10, so we raise a ValueError("TEST") exception,
    as you can see, the message "TEST" is NOT LOST.
    I just find this counter-intuitive, and error-prone.

    AT LEAST should be very well specified in the docs.

    Regards,
    M.

    On Sat, Oct 9, 2021 at 2:51 PM Chris Jerdonek <report@bugs.python.org>
    wrote:

    Chris Jerdonek chris.jerdonek@gmail.com added the comment:

    1. Now: if I re-raise the asyncio.CancelledError as-is, I lose the
      message,
      if I call the asyncio.Task.exception() function.

    Re-raise asyncio.CancelledError where? (And what do you mean by
    "re-raise"?) Call asyncio.Task.exception() where? This isn't part of your
    example, so it's not clear what you mean exactly.

    ----------


    Python tracker <report@bugs.python.org>
    <https://bugs.python.org/issue45390\>


    @cjerdonek
    Copy link
    Member

    I still don't see you calling asyncio.Task.exception() in your new attachment...

    @pagliariccim
    Copy link
    Mannequin Author

    pagliariccim mannequin commented Oct 9, 2021

    Chris,
    ok, I have modified the snippet of code to better show what I mean.
    Still here, the message of the CancelledError exception is lost, but if I
    comment line 10, and uncomment line 11, so I throw a ValueError("TEST"),
    that "TEST" string will be printed, so the message is not lost.
    Again, I just find this behavior very counter-intuitive, and should be VERY
    WELL documented in the docs.

    Thanks,
    M.

    On Sat, Oct 9, 2021 at 3:06 PM Chris Jerdonek <report@bugs.python.org>
    wrote:

    Chris Jerdonek <chris.jerdonek@gmail.com> added the comment:

    I still don't see you calling asyncio.Task.exception() in your new
    attachment...

    ----------


    Python tracker <report@bugs.python.org>
    <https://bugs.python.org/issue45390\>


    @cjerdonek
    Copy link
    Member

    Here's a simplification of Marco's snippet to focus the discussion.

    import asyncio
    
    async def job():
        # raise RuntimeError('error!')
        await asyncio.sleep(5)
    
    async def main():
        task = asyncio.create_task(job())
        await asyncio.sleep(1)
        task.cancel('cancel job')
        await task
    
    if __name__=="__main__":
        asyncio.run(main())

    Running this pre-Python 3.9 gives something like this--

    Traceback (most recent call last):
      File "test.py", line 15, in <module>
        asyncio.run(main())
      File "/.../python3.7/asyncio/runners.py", line 43, in run
        return loop.run_until_complete(main)
      File "/.../python3.7/asyncio/base_events.py", line 579, in run_until_complete
        return future.result()
    concurrent.futures._base.CancelledError

    Running this with Python 3.9+ gives something like the following. The difference is that the traceback now starts at the sleep() call:

    Traceback (most recent call last):
      File "/.../test.py", line 6, in job
        await asyncio.sleep(5)
      File "/.../python3.9/asyncio/tasks.py", line 654, in sleep
        return await future
    asyncio.exceptions.CancelledError: cancel job
    
    During handling of the above exception, another exception occurred:
    
    Traceback (most recent call last):
      File "/.../test.py", line 12, in main
        await task
    asyncio.exceptions.CancelledError
    
    During handling of the above exception, another exception occurred:
    
    Traceback (most recent call last):
      File "/.../test.py", line 15, in <module>
        asyncio.run(main())
      File "/.../python3.9/asyncio/runners.py", line 44, in run
        return loop.run_until_complete(main)
      File "/.../python3.9/asyncio/base_events.py", line 642, in run_until_complete
        return future.result()
    asyncio.exceptions.CancelledError

    Uncommenting the RuntimeError turns it into this--

    Traceback (most recent call last):
      File "/.../test.py", line 15, in <module>
        asyncio.run(main())
      File "/.../python3.9/asyncio/runners.py", line 44, in run
        return loop.run_until_complete(main)
      File "/.../python3.9/asyncio/base_events.py", line 642, in run_until_complete
        return future.result()
      File "/.../test.py", line 12, in main
        await task
      File "/.../test.py", line 5, in job
        raise RuntimeError('error!')
    RuntimeError: error!

    I agree it would be a lot nicer if the original CancelledError('cancel job') could bubble up just like the RuntimeError does, instead of creating a new CancelledError at each await and chaining it to the previous CancelledError. asyncio's creation of a new CancelledError at each stage predates the PR that added the chaining, so this could be viewed as an evolution of the change that added the chaining.

    I haven't checked to be sure, but the difference in behavior between CancelledError and other exceptions might be explained by the following lines:

    except exceptions.CancelledError as exc:
    # Save the original exception so we can chain it later.
    self._cancelled_exc = exc
    super().cancel() # I.e., Future.cancel(self).
    except (KeyboardInterrupt, SystemExit) as exc:
    super().set_exception(exc)
    raise
    except BaseException as exc:
    super().set_exception(exc)

    You can see that for exceptions other than CancelledError, the exception is propagated by calling super().set_exception(exc), whereas with CancelledError, it is propagated by calling super().cancel() again.

    Maybe this would even be an easy change to make. Instead of asyncio creating a new CancelledError and chaining it to the previous, asyncio can just raise the existing one. For the pure Python implementation at least, it may be as simple as making a change here, inside _make_cancelled_error():

    if self._cancel_message is None:
    exc = exceptions.CancelledError()
    else:
    exc = exceptions.CancelledError(self._cancel_message)
    exc.__context__ = self._cancelled_exc
    # Remove the reference since we don't need this anymore.
    self._cancelled_exc = None
    return exc

    @asvetlov asvetlov added 3.11 bug and security fixes and removed 3.9 only security fixes labels Feb 17, 2022
    @asvetlov
    Copy link
    Contributor

    I have a pull request for the issue.
    It doesn't use Future.set_exception() but creates a new CancelledError() with propagated message.
    The result is the same, except raised exceptions are not comparable by is check.
    As a benefit, _cancelled_exc works with the patch, exc.context is correctly set.

    The patch is not backported because it changes existing behavior a little. I'd like to avoid a situation when third-party code works with Python 3.11+, 3.10.3+, and 3.9.11+ only.

    @pagliariccim
    Copy link
    Mannequin Author

    pagliariccim mannequin commented Feb 17, 2022

    Andrew,
    many thanks for your time, solving this issue.
    I think your solution is the best to fix this little problem and I agree
    with you on backporting.

    My Best Regards,
    and thanks again.

    Marco

    On Thu, Feb 17, 2022 at 10:29 AM Andrew Svetlov <report@bugs.python.org>
    wrote:

    Andrew Svetlov andrew.svetlov@gmail.com added the comment:

    I have a pull request for the issue.
    It doesn't use Future.set_exception() but creates a new CancelledError()
    with propagated message.
    The result is the same, except raised exceptions are not comparable by
    is check.
    As a benefit, _cancelled_exc works with the patch, exc.context is
    correctly set.

    The patch is not backported because it changes existing behavior a little.
    I'd like to avoid a situation when third-party code works with Python
    3.11+, 3.10.3+, and 3.9.11+ only.

    ----------


    Python tracker <report@bugs.python.org>
    <https://bugs.python.org/issue45390\>


    @cjerdonek
    Copy link
    Member

    Andrew, the approach I described would I feel be much better. It would result in more concise, less verbose tracebacks, as opposed to more verbose -- not just because the message won't be repeated, but also because it eliminates the unneeded creation of intermediate exceptions. It would also cause is checks to work, which is what we want since behaviorally it should be the same exception.

    @asvetlov
    Copy link
    Contributor

    New changeset 4140bcb by Andrew Svetlov in branch 'main':
    bpo-45390: Propagate CancelledError's message from cancelled task to its awaiter (GH-31383)
    4140bcb

    @serhiy-storchaka
    Copy link
    Member

    Seems a CancelledError message can be lost also in Condition.wait().

    @cjerdonek
    Copy link
    Member

    For future reference, with Andrew's change merged above, the traceback for the example snippet in my message above:
    https://bugs.python.org/issue45390#msg403570
    is now the following. Observe that (1) the call to sleep() continues to be present, but (2) without introducing two new intermediate CancelledErrors, which increase the verbosity of the traceback:

    Traceback (most recent call last):
      File "/home/andrew/projects/cpython/exc_traceback.py", line 14, in <module>
        asyncio.run(main())
        ^^^^^^^^^^^^^^^^^^^
      File "/home/andrew/projects/cpython/Lib/asyncio/runners.py", line 44, in run
        return loop.run_until_complete(main)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/home/andrew/projects/cpython/Lib/asyncio/base_events.py", line 640, in run_until_complete
        return future.result()
               ^^^^^^^^^^^^^^^
      File "/home/andrew/projects/cpython/exc_traceback.py", line 11, in main
        await task
        ^^^^^^^^^^
      File "/home/andrew/projects/cpython/exc_traceback.py", line 5, in job
        await asyncio.sleep(5)
        ^^^^^^^^^^^^^^^^^^^^^^
      File "/home/andrew/projects/cpython/Lib/asyncio/tasks.py", line 619, in sleep
        return await future
               ^^^^^^^^^^^^
    asyncio.exceptions.CancelledError: cancel job

    (This is copied from Andrew's comment in the PR here:
    #31383 (comment) )

    Serhiy, can you provide a sample snippet for your case with output, like I did in my message linked above?

    @asvetlov
    Copy link
    Contributor

    Serhiy is right, Condition.wait() has the following code:

        finally:
            # Must reacquire lock even if wait is cancelled
            cancelled = False
            while True:
                try:
                    await self.acquire()
                    break
                except exceptions.CancelledError:
                    cancelled = True
    
                if cancelled:
                    raise exceptions.CancelledError

    It swallows CancelledError exceptions from waiters and re-raises CancelledError without the cancellation message.

    @serhiy-storchaka
    Copy link
    Member

    Also Future.result() and Future.exception() can raise a CancelledError. So a CancelledError raised in a task may not contain a message passed to Task.cancel().

    import asyncio
    import random
    
    async def main():
        fut = asyncio.Future()
        fut.cancel()
        async def job():
            if random.random() < 0.5:
                await asyncio.sleep(2)
            fut.result()
            await asyncio.sleep(5)
        task = asyncio.create_task(job())
        await asyncio.sleep(1)
        task.cancel("cancel task")
        await task
    
    asyncio.run(main())

    You need to catch a CancelledError raised in a coroutine and re-raise a new CancelledError with the specified cancel message if the task was cancelled.

    @graingert
    Copy link
    Mannequin

    graingert mannequin commented Feb 23, 2022

    there could be multiple messages here

    perhaps it could be:

            finally:
                # Must reacquire lock even if wait is cancelled
                cancelled = []
                while True:
                    try:
                        await self.acquire()
                        break
                    except exceptions.CancelledError as e:
                        cancelled.append(e)
    
                if len(cancelled) > 1:
                    raise ExceptionGroup("Cancelled", cancelled)
                if cancelled:
                    raise cancelled[0]
    

    @gvanrossum
    Copy link
    Member

    We should really stop appending to a closed issue.

    Anyway, raising ExceptionGroup is backwards incompatible, since "except CancelledError" wouldn't cancel the group.

    @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
    3.11 bug and security fixes topic-asyncio type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants