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

Confusing CancelError message if multiple cancellations are scheduled #90985

Closed
asvetlov opened this issue Feb 22, 2022 · 15 comments
Closed

Confusing CancelError message if multiple cancellations are scheduled #90985

asvetlov opened this issue Feb 22, 2022 · 15 comments
Labels
3.11 only security fixes topic-asyncio

Comments

@asvetlov
Copy link
Contributor

BPO 46829
Nosy @gvanrossum, @njsmith, @jab, @asvetlov, @agronholm, @cjerdonek, @serhiy-storchaka, @1st1, @Tinche, @iritkatriel, @Dreamsorcerer, @ajoino
PRs
  • bpo-46829: Deprecate passing a message into Future.cancel() and Task.cancel() #31840
  • 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-03-23.16:01:27.456>
    created_at = <Date 2022-02-22.22:02:51.215>
    labels = ['3.11', 'expert-asyncio']
    title = 'Confusing CancelError message if multiple cancellations are scheduled'
    updated_at = <Date 2022-03-23.16:01:27.455>
    user = 'https://github.com/asvetlov'

    bugs.python.org fields:

    activity = <Date 2022-03-23.16:01:27.455>
    actor = 'gvanrossum'
    assignee = 'none'
    closed = True
    closed_date = <Date 2022-03-23.16:01:27.456>
    closer = 'gvanrossum'
    components = ['asyncio']
    creation = <Date 2022-02-22.22:02:51.215>
    creator = 'asvetlov'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 46829
    keywords = ['patch']
    message_count = 13.0
    messages = ['413749', '413753', '413755', '413757', '413776', '413817', '413821', '415013', '415014', '415098', '415099', '415879', '415883']
    nosy_count = 12.0
    nosy_names = ['gvanrossum', 'njs', 'jab', 'asvetlov', 'alex.gronholm', 'chris.jerdonek', 'serhiy.storchaka', 'yselivanov', 'tinchester', 'iritkatriel', 'dreamsorcerer', 'ajoino']
    pr_nums = ['31840']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue46829'
    versions = ['Python 3.11']

    @asvetlov
    Copy link
    Contributor Author

    Suppose multiple task.cancel(msg) with different messages are called on the same event loop iteration.

    What message (cancel_exc.args[0]) should be sent on the next loop iteration?

    As of Python 3.10 it is the message from the last task.cancel(msg) call.
    The main branch changed it to the first message (it is a subject for discussion still).
    Both choices are equally bad. The order of tasks execution at the same loop iteration is weak and depends on very many circumstances. Effectively, first-cancelled-message and last-cancelled-message are equal to random-message.

    This makes use of cancellation message not robust: a task can be cancelled by many sources, task-groups adds even more mess.

    Guido van Rossum suggested that messages should be collected in a list and raised altogether. There is a possibility to do it in a backward-compatible way: construct the exception as CancelledError(last_msg, tuple(msg_list)). args[0] is args[1][-1]. Weird but works.

    .cancel() should add None to the list of cancelled messages.
    The message list should be cleared when a new CancelledError is constructed and thrown into cancelling task.

    Working with exc.args[0] / exc.args[1] is tedious and error-prone. I propose adding exc.msgs property. Not sure if the last added message is worth a separate attribute, a robust code should not rely on messages order as I described above.

    The single message is not very useful but a list of messages can be used in timeouts implementation as cancellation count alternative.

    I don't have a strong preference now but want to carefully discuss possible opportunities before making the final decision.

    @asvetlov asvetlov added 3.11 only security fixes topic-asyncio labels Feb 22, 2022
    @serhiy-storchaka
    Copy link
    Member

    What about CancelledError(*msg_list) or CancelledError(*reversed(msg_list))? It is backward compatible and all messages are uniformely represented.

    @gvanrossum
    Copy link
    Member

    I would like to go on record (again) as preferring to get rid of the cancel-message parameter altogether. Let's deprecate it. When we initially designed things without a cancel message we did it on purpose -- "cancellation" is a single flag, not a collection of data.

    @asvetlov
    Copy link
    Contributor Author

    Deprecation is a good answer.
    Let's not forget to apply it to 3.11 then.

    @cjerdonek
    Copy link
    Member

    I don't really understand all the hate around the idea of a cancel message. One reason it's useful is that it provides a simple way to say *why* something is being cancelled or *what* is cancelling it. It provides additional context to the exception, in the same way that a normal exception's message can provide additional helpful context.

    Take for example this chunk of code:

    import asyncio
    import random
    
    async def job():
        await asyncio.sleep(5)
    
    async def main():
        task = asyncio.create_task(job())
        await asyncio.sleep(1)
        r = random.random()
        if r < 0.5:
            task.cancel()
        else:
            task.cancel()
        await task
    
    if __name__=="__main__":
        asyncio.run(main())

    Without passing a message, there's no way to tell which of the two lines called cancel, or why. The tracebacks are identical in both cases. This is even in the simplest case where only one cancellation is occurring. Passing a message lets one disambiguate the two call sites. And if there is truly a race condition where it's up in the air between two things cancelling it, I think it would be okay for the chosen message to be indeterminate, as either would have instigated the cancel in the absence of the other.

    @gvanrossum
    Copy link
    Member

    But that example is made-up. Is there a real-world situation where you need to know the call site, and it wouldn't be obvious from other log messages?

    Directly cancelling a task without also somehow catching the cancellation (like in the timeout or task group cases) feels like an odd practice to me.

    And passing the cancel message through is complex (as we've seen in recent PRs).

    @serhiy-storchaka
    Copy link
    Member

    For reference, the msg parameter of Task.cancel() was added in bpo-31033.

    It seems that the initial use case was for debugging. I do not see how it differs from the following example:

        r = random.random()
        if r < 0.5:
            x = 0
        else:
            x = 0
        1/x

    In the traceback we see the line where an error occurred but we do not see a line which lead to this error.

    @gvanrossum
    Copy link
    Member

    Before we land #76021 we should have a somewhat more public discussion (e.g. on python-dev or maybe in Async-SIG, https://discuss.python.org/c/async-sig/20; or at least here) about deprecating the cancel message. I'm all for it but certainly Chris Jerdonek (who wrote the original code, see bpo-31033) needs to have a say, and from a comment on #64150 it looks Yury Selivanov also really liked it.

    @asvetlov
    Copy link
    Contributor Author

    If the cancellation message should be kept it needs improvements anyway, the single message doesn't work well with multiple .cancel() calls.

    I can imagine a 'CancelledError(*msgs)' and 'raise exc.drop_msg(msg)' as a function equivalent of task cancellation counter and .cancel() / .uncancel() pairing. A similar to MultiError, some sort of.

    The counter is easier to understand I guess.

    Both multi-message and counter require new APIs, timeout() can be adapted to any solution.

    @1st1
    Copy link
    Member

    1st1 commented Mar 13, 2022

    Andrew asked me for my opinion on the matter -- I think we should get rid of the message. Exception messages for "signals" can be easily lost if an exception was re-raised. If the reason of why something is being cancelled is important (in my experience it never is) it should be recorded elsewhere.

    @1st1
    Copy link
    Member

    1st1 commented Mar 13, 2022

    IOW I think that supporting custom messages is a needless complication of our API. Given how complex task trees can become with TaskGroups collecting those messages and presenting them all to the user isn't trivial, showing just first/last defeats the purpose. So i'm in favor of dropping it.

    @gvanrossum
    Copy link
    Member

    New changeset 0360e9f by Andrew Svetlov in branch 'main':
    bpo-46829: Deprecate passing a message into Future.cancel() and Task.cancel() (GH-31840)
    0360e9f

    @gvanrossum
    Copy link
    Member

    gvanrossum commented Mar 23, 2022

    Fixed by deprecating the message argument to cancel(). It will be removed in 3.13 3.14.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    gvanrossum added a commit that referenced this issue Oct 7, 2022
    Reason: we were too hasty in deprecating this.
    We shouldn't deprecate it before we have a replacement.
    miss-islington pushed a commit to miss-islington/cpython that referenced this issue Oct 7, 2022
    …ythonGH-97999)
    
    Reason: we were too hasty in deprecating this.
    We shouldn't deprecate it before we have a replacement.
    (cherry picked from commit 09de8d7)
    
    Co-authored-by: Guido van Rossum <guido@python.org>
    miss-islington added a commit that referenced this issue Oct 7, 2022
    Reason: we were too hasty in deprecating this.
    We shouldn't deprecate it before we have a replacement.
    (cherry picked from commit 09de8d7)
    
    Co-authored-by: Guido van Rossum <guido@python.org>
    carljm added a commit to carljm/cpython that referenced this issue Oct 8, 2022
    * main:
      pythongh-86298: Ensure that __loader__ and __spec__.loader agree in warnings.warn_explicit() (pythonGH-97803)
      pythongh-82874: Convert remaining importlib format uses to f-str. (python#98005)
      Docs: Fix backtick errors found by sphinx-lint (python#97998)
      pythongh-97850: Remove deprecated functions from `importlib.utils` (python#97898)
      Remove extra spaces in custom openSSL documentation. (python#93568)
      pythonGH-90985: Revert  "Deprecate passing a message into cancel()" (python#97999)
    mpage pushed a commit to mpage/cpython that referenced this issue Oct 11, 2022
    …ython#97999)
    
    Reason: we were too hasty in deprecating this.
    We shouldn't deprecate it before we have a replacement.
    pablogsal pushed a commit that referenced this issue Oct 24, 2022
    Reason: we were too hasty in deprecating this.
    We shouldn't deprecate it before we have a replacement.
    (cherry picked from commit 09de8d7)
    
    Co-authored-by: Guido van Rossum <guido@python.org>
    @fancidev
    Copy link
    Contributor

    Late to the party, but an alternative to make both party happy might be to let Task.cancel accept an exception object that’s derived from asyncio.CancelledError?

    @xitop
    Copy link

    xitop commented Dec 5, 2022

    Could the problem discussed here be solved with exceptions notes? The PEP-678 mentions adding notes to caught exceptions, but I tested that it is possible to create an exception instance with several notes added and raise it (I did not test throwing it into a coroutine though).

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.11 only security fixes topic-asyncio
    Projects
    None yet
    Development

    No branches or pull requests

    7 participants