This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Confusing CancelError message if multiple cancellations are scheduled
Type: Stage: resolved
Components: asyncio Versions: Python 3.11
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: ajoino, alex.gronholm, asvetlov, chris.jerdonek, dreamsorcerer, gvanrossum, iritkatriel, jab, njs, serhiy.storchaka, tinchester, yselivanov
Priority: normal Keywords: patch

Created on 2022-02-22 22:02 by asvetlov, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 31840 merged asvetlov, 2022-03-12 19:08
Messages (13)
msg413749 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2022-02-22 22:02
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.
msg413753 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2022-02-22 22:49
What about `CancelledError(*msg_list)` or `CancelledError(*reversed(msg_list))`? It is backward compatible and all messages are uniformely represented.
msg413755 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2022-02-22 23:05
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.
msg413757 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2022-02-22 23:38
Deprecation is a good answer. 
Let's not forget to apply it to 3.11 then.
msg413776 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2022-02-23 07:16
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.
msg413817 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2022-02-23 16:46
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).
msg413821 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2022-02-23 17:03
For reference, the msg parameter of Task.cancel() was added in issue31033.

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.
msg415013 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2022-03-12 20:55
Before we land GH-31840 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 GH-19951 it looks Yury Selivanov also really liked it.
msg415014 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2022-03-12 21:08
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.
msg415098 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2022-03-13 20:39
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.
msg415099 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2022-03-13 20:42
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.
msg415879 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2022-03-23 15:43
New changeset 0360e9f34659e7d7f3dae021b82f78452db8c714 by Andrew Svetlov in branch 'main':
bpo-46829: Deprecate passing a message into Future.cancel() and Task.cancel() (GH-31840)
https://github.com/python/cpython/commit/0360e9f34659e7d7f3dae021b82f78452db8c714
msg415883 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2022-03-23 16:01
Fixed by deprecating the message argument to cancel(). It will be removed in 3.13.
History
Date User Action Args
2022-04-11 14:59:56adminsetgithub: 90985
2022-03-23 16:01:27gvanrossumsetstatus: open -> closed
resolution: fixed
messages: + msg415883

stage: patch review -> resolved
2022-03-23 15:43:15gvanrossumsetmessages: + msg415879
2022-03-13 20:42:00yselivanovsetmessages: + msg415099
2022-03-13 20:39:49yselivanovsetmessages: + msg415098
2022-03-12 21:08:21asvetlovsetmessages: + msg415014
2022-03-12 20:55:28gvanrossumsetmessages: + msg415013
2022-03-12 19:08:55asvetlovsetkeywords: + patch
stage: patch review
pull_requests: + pull_request29940
2022-02-23 17:03:30serhiy.storchakasetmessages: + msg413821
2022-02-23 16:46:25gvanrossumsetmessages: + msg413817
2022-02-23 07:16:05chris.jerdoneksetmessages: + msg413776
2022-02-22 23:38:05asvetlovsetmessages: + msg413757
2022-02-22 23:05:27gvanrossumsetmessages: + msg413755
2022-02-22 22:49:41serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg413753
2022-02-22 22:02:51asvetlovcreate