Skip to content

bpo-46771: Remove two controversial lines from Task.cancel() #31623

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.

Sign up for GitHub

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

Merged
gvanrossum merged 1 commit into from
Feb 28, 2022
Merged

bpo-46771: Remove two controversial lines from Task.cancel() #31623

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Remove two controversial lines from Task.cancel() 
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:
#31394 (comment)

(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.)
  • Loading branch information
@gvanrossum
gvanrossum committed Feb 28, 2022
commit 0feaadeb17c5bab88981dfafa1f8ae056b139b47
7 changes: 5 additions & 2 deletions Lib/asyncio/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 5 additions & 1 deletion Lib/test/test_asyncio/test_tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
10 changes: 7 additions & 3 deletions Modules/_asynciomodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down