[Python-checkins] bpo-46771: Remove two controversial lines from Task.cancel() (GH-31623)

[gvanrossum]

https://github.com/python/cpython/commit/7d611b4cabaf7925f5f94daddf711d54aeae2cf9
commit: 7d611b4cabaf7925f5f94daddf711d54aeae2cf9
branch: main
author: Guido van Rossum <guido at python.org>
committer: gvanrossum <gvanrossum at gmail.com>
date: 2022-02-28T15:15:56-08:00
summary:

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

files:
M Lib/asyncio/tasks.py
M Lib/test/test_asyncio/test_tasks.py
M Modules/_asynciomodule.c

diff --git a/Lib/asyncio/tasks.py b/Lib/asyncio/tasks.py
index 38c23851102a8..059143fb9086f 100644
--- a/Lib/asyncio/tasks.py
+++ b/Lib/asyncio/tasks.py
@@ -205,8 +205,11 @@ def cancel(self, msg=None):
         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 b30f8f56dfa72..950879204e703 100644
--- a/Lib/test/test_asyncio/test_tasks.py
+++ b/Lib/test/test_asyncio/test_tasks.py
@@ -514,7 +514,11 @@ async def task():
             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 88471239529a5..2a6c0b335ccfb 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;