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

bpo-45390: Propagate CancelledError's message from cancelled task to its awaiter #31383

Merged
merged 7 commits into from
Feb 21, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions Doc/library/asyncio-task.rst
Original file line number Diff line number Diff line change
Expand Up @@ -843,6 +843,9 @@ Task Object
.. versionchanged:: 3.9
Added the *msg* parameter.

.. versionchanged:: 3.11
The ``msg`` parameter is propagated from cancelled task to its awaiter.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only the msg parameter or the exception? Is the latter an implementation detail?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC there are no guarantees about the identity of the exception object.

My personal opinion is that the whole "cancelled message" concept was a mistake, but it's too late to roll it back.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think msg needs to be mentioned in the versionchanged, IMO. The PR is more about eliminating unneeded intermediate CancelledError's, resulting in a more compact traceback, etc. In other words, this PR would be useful even without the msg argument. It's more a side effect that (in most cases), the msg will bubble out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you propose better text, please?
The change is user-faced, the most visible difference is keeping/swallowing the cancellation message on crossing tasks border.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, then I'm okay with what you had.


.. _asyncio_example_task_cancel:

The following example illustrates how coroutines can intercept
Expand Down
5 changes: 5 additions & 0 deletions Lib/asyncio/futures.py
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,11 @@ def _make_cancelled_error(self):
This should only be called once when handling a cancellation since
it erases the saved context exception value.
"""
if self._cancelled_exc is not None:
exc = self._cancelled_exc
self._cancelled_exc = None
return exc

if self._cancel_message is None:
exc = exceptions.CancelledError()
else:
Expand Down
66 changes: 50 additions & 16 deletions Lib/test/test_asyncio/test_tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -124,9 +124,11 @@ async def coro():
t.cancel('my message')
self.assertEqual(t._cancel_message, 'my message')

with self.assertRaises(asyncio.CancelledError):
with self.assertRaises(asyncio.CancelledError) as cm:
self.loop.run_until_complete(t)

self.assertEqual('my message', cm.exception.args[0])

def test_task_cancel_message_setter(self):
async def coro():
pass
Expand All @@ -135,9 +137,11 @@ async def coro():
t._cancel_message = 'my new message'
self.assertEqual(t._cancel_message, 'my new message')

with self.assertRaises(asyncio.CancelledError):
with self.assertRaises(asyncio.CancelledError) as cm:
self.loop.run_until_complete(t)

self.assertEqual('my new message', cm.exception.args[0])

def test_task_del_collect(self):
class Evil:
def __del__(self):
Expand Down Expand Up @@ -590,11 +594,11 @@ async def coro():
with self.assertRaises(asyncio.CancelledError) as cm:
loop.run_until_complete(task)
exc = cm.exception
self.assertEqual(exc.args, ())
self.assertEqual(exc.args, expected_args)

actual = get_innermost_context(exc)
self.assertEqual(actual,
(asyncio.CancelledError, expected_args, 2))
(asyncio.CancelledError, expected_args, 0))

def test_cancel_with_message_then_future_exception(self):
# Test Future.exception() after calling cancel() with a message.
Expand Down Expand Up @@ -624,11 +628,39 @@ async def coro():
with self.assertRaises(asyncio.CancelledError) as cm:
loop.run_until_complete(task)
exc = cm.exception
self.assertEqual(exc.args, ())
self.assertEqual(exc.args, expected_args)

actual = get_innermost_context(exc)
self.assertEqual(actual,
(asyncio.CancelledError, expected_args, 2))
(asyncio.CancelledError, expected_args, 0))

def test_cancellation_exception_context(self):
loop = asyncio.new_event_loop()
self.set_event_loop(loop)
fut = loop.create_future()

async def sleep():
fut.set_result(None)
await asyncio.sleep(10)

async def coro():
inner_task = self.new_task(loop, sleep())
await fut
loop.call_soon(inner_task.cancel, 'msg')
try:
await inner_task
except asyncio.CancelledError as ex:
raise ValueError("cancelled") from ex

task = self.new_task(loop, coro())
with self.assertRaises(ValueError) as cm:
loop.run_until_complete(task)
exc = cm.exception
self.assertEqual(exc.args, ('cancelled',))

actual = get_innermost_context(exc)
self.assertEqual(actual,
(asyncio.CancelledError, ('msg',), 1))

def test_cancel_with_message_before_starting_task(self):
loop = asyncio.new_event_loop()
Expand All @@ -648,11 +680,11 @@ async def coro():
with self.assertRaises(asyncio.CancelledError) as cm:
loop.run_until_complete(task)
exc = cm.exception
self.assertEqual(exc.args, ())
self.assertEqual(exc.args, ('my message',))

actual = get_innermost_context(exc)
self.assertEqual(actual,
(asyncio.CancelledError, ('my message',), 2))
(asyncio.CancelledError, ('my message',), 0))

def test_cancel_yield(self):
async def task():
Expand Down Expand Up @@ -2296,15 +2328,17 @@ async def main():
try:
loop.run_until_complete(main())
except asyncio.CancelledError as exc:
self.assertEqual(exc.args, ())
exc_type, exc_args, depth = get_innermost_context(exc)
self.assertEqual((exc_type, exc_args),
(asyncio.CancelledError, expected_args))
# The exact traceback seems to vary in CI.
self.assertIn(depth, (2, 3))
self.assertEqual(exc.args, expected_args)
actual = get_innermost_context(exc)
self.assertEqual(
actual,
(asyncio.CancelledError, expected_args, 0),
)
else:
self.fail('gather did not propagate the cancellation '
'request')
self.fail(
'gather() does not propagate CancelledError '
'raised by inner task to the gather() caller.'
)

def test_exception_traceback(self):
# See http://bugs.python.org/issue28843
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Propagate :exc:`asyncio.CancelledError` message from inner task to outer
awaiter.
42 changes: 19 additions & 23 deletions Modules/_asynciomodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ typedef enum {
int prefix##_blocking; \
PyObject *dict; \
PyObject *prefix##_weakreflist; \
_PyErr_StackItem prefix##_cancelled_exc_state;
PyObject *prefix##_cancelled_exc;

typedef struct {
FutureObj_HEAD(fut)
Expand Down Expand Up @@ -496,7 +496,7 @@ future_init(FutureObj *fut, PyObject *loop)
Py_CLEAR(fut->fut_exception);
Py_CLEAR(fut->fut_source_tb);
Py_CLEAR(fut->fut_cancel_msg);
_PyErr_ClearExcState(&fut->fut_cancelled_exc_state);
Py_CLEAR(fut->fut_cancelled_exc);

fut->fut_state = STATE_PENDING;
fut->fut_log_tb = 0;
Expand Down Expand Up @@ -612,25 +612,32 @@ future_set_exception(FutureObj *fut, PyObject *exc)
}

static PyObject *
create_cancelled_error(PyObject *msg)
create_cancelled_error(FutureObj *fut)
{
PyObject *exc;
if (fut->fut_cancelled_exc != NULL) {
/* transfer ownership */
exc = fut->fut_cancelled_exc;
fut->fut_cancelled_exc = NULL;
return exc;
}
PyObject *msg = fut->fut_cancel_msg;
if (msg == NULL || msg == Py_None) {
exc = PyObject_CallNoArgs(asyncio_CancelledError);
} else {
exc = PyObject_CallOneArg(asyncio_CancelledError, msg);
}
PyException_SetContext(exc, fut->fut_cancelled_exc);
Py_CLEAR(fut->fut_cancelled_exc);
return exc;
}

static void
future_set_cancelled_error(FutureObj *fut)
{
PyObject *exc = create_cancelled_error(fut->fut_cancel_msg);
PyObject *exc = create_cancelled_error(fut);
PyErr_SetObject(asyncio_CancelledError, exc);
Py_DECREF(exc);

_PyErr_ChainStackItem(&fut->fut_cancelled_exc_state);
}

static int
Expand Down Expand Up @@ -793,7 +800,7 @@ FutureObj_clear(FutureObj *fut)
Py_CLEAR(fut->fut_exception);
Py_CLEAR(fut->fut_source_tb);
Py_CLEAR(fut->fut_cancel_msg);
_PyErr_ClearExcState(&fut->fut_cancelled_exc_state);
Py_CLEAR(fut->fut_cancelled_exc);
Py_CLEAR(fut->dict);
return 0;
}
Expand All @@ -809,11 +816,8 @@ FutureObj_traverse(FutureObj *fut, visitproc visit, void *arg)
Py_VISIT(fut->fut_exception);
Py_VISIT(fut->fut_source_tb);
Py_VISIT(fut->fut_cancel_msg);
Py_VISIT(fut->fut_cancelled_exc);
Py_VISIT(fut->dict);

_PyErr_StackItem *exc_state = &fut->fut_cancelled_exc_state;
Py_VISIT(exc_state->exc_value);

return 0;
}

Expand Down Expand Up @@ -1369,15 +1373,7 @@ static PyObject *
_asyncio_Future__make_cancelled_error_impl(FutureObj *self)
/*[clinic end generated code: output=a5df276f6c1213de input=ac6effe4ba795ecc]*/
{
PyObject *exc = create_cancelled_error(self->fut_cancel_msg);
_PyErr_StackItem *exc_state = &self->fut_cancelled_exc_state;

if (exc_state->exc_value) {
PyException_SetContext(exc, Py_NewRef(exc_state->exc_value));
_PyErr_ClearExcState(exc_state);
}

return exc;
return create_cancelled_error(self);
}

/*[clinic input]
Expand Down Expand Up @@ -2677,7 +2673,7 @@ task_step_impl(TaskObj *task, PyObject *exc)

if (!exc) {
/* exc was not a CancelledError */
exc = create_cancelled_error(task->task_cancel_msg);
exc = create_cancelled_error((FutureObj*)task);

if (!exc) {
goto fail;
Expand Down Expand Up @@ -2751,8 +2747,8 @@ task_step_impl(TaskObj *task, PyObject *exc)
Py_XDECREF(et);

FutureObj *fut = (FutureObj*)task;
_PyErr_StackItem *exc_state = &fut->fut_cancelled_exc_state;
exc_state->exc_value = ev;
/* transfer ownership */
fut->fut_cancelled_exc = ev;

return future_cancel(fut, NULL);
}
Expand Down