From 7d611b4cabaf7925f5f94daddf711d54aeae2cf9 Mon Sep 17 00:00:00 2001 From: Guido van Rossum Date: Mon, 28 Feb 2022 15:15:56 -0800 Subject: [PATCH] bpo-46771: Remove two controversial lines from Task.cancel() (GH-31623) Also from the _asyncio C accelerator module, and adjust one test that the change caused to fail. For more discussion see the discussion starting here: https://github.com/python/cpython/pull/31394#issuecomment-1053545331 (Basically, @asvetlov proposed to return False from cancel() when there is already a pending cancellation, and I went along, even though it wasn't necessary for the task group implementation, and @agronholm has come up with a counterexample that fails because of this change. So now I'm changing it back to the old semantics (but still bumping the counter) until we can have a proper discussion about this.) --- Lib/asyncio/tasks.py | 7 +++++-- Lib/test/test_asyncio/test_tasks.py | 6 +++++- Modules/_asynciomodule.c | 10 +++++++--- 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/Lib/asyncio/tasks.py b/Lib/asyncio/tasks.py index 38c23851102..059143fb908 100644 --- a/Lib/asyncio/tasks.py +++ b/Lib/asyncio/tasks.py @@ -205,8 +205,11 @@ class Task(futures._PyFuture): # Inherit Python Task implementation if self.done(): return False self._num_cancels_requested += 1 - if self._num_cancels_requested > 1: - return False + # These two lines are controversial. See discussion starting at + # https://github.com/python/cpython/pull/31394#issuecomment-1053545331 + # Also remember that this is duplicated in _asynciomodule.c. + # if self._num_cancels_requested > 1: + # return False if self._fut_waiter is not None: if self._fut_waiter.cancel(msg=msg): # Leave self._fut_waiter; it may be a Task that diff --git a/Lib/test/test_asyncio/test_tasks.py b/Lib/test/test_asyncio/test_tasks.py index b30f8f56dfa..950879204e7 100644 --- a/Lib/test/test_asyncio/test_tasks.py +++ b/Lib/test/test_asyncio/test_tasks.py @@ -514,7 +514,11 @@ class BaseTaskTests: self.assertTrue(t.cancel()) self.assertTrue(t.cancelling()) self.assertIn(" cancelling ", repr(t)) - self.assertFalse(t.cancel()) + + # Since we commented out two lines from Task.cancel(), + # this t.cancel() call now returns True. + # self.assertFalse(t.cancel()) + self.assertTrue(t.cancel()) with self.assertRaises(asyncio.CancelledError): loop.run_until_complete(t) diff --git a/Modules/_asynciomodule.c b/Modules/_asynciomodule.c index 88471239529..2a6c0b335cc 100644 --- a/Modules/_asynciomodule.c +++ b/Modules/_asynciomodule.c @@ -2206,9 +2206,13 @@ _asyncio_Task_cancel_impl(TaskObj *self, PyObject *msg) } self->task_num_cancels_requested += 1; - if (self->task_num_cancels_requested > 1) { - Py_RETURN_FALSE; - } + + // These three lines are controversial. See discussion starting at + // https://github.com/python/cpython/pull/31394#issuecomment-1053545331 + // and corresponding code in tasks.py. + // if (self->task_num_cancels_requested > 1) { + // Py_RETURN_FALSE; + // } if (self->task_fut_waiter) { PyObject *res;