Confusing CancelError message if multiple cancellations are scheduled

[asvetlov]

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]

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.

[serhiy-storchaka]

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

[gvanrossum]

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]

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

[cjerdonek]

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]

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]

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]

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]

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]

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]

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]

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]

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

[fancidev]

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]

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