investigate task/future cancellation in asynciomodule.c

[1st1]

BPO	34872
Nosy	@ned-deily, @asvetlov, @elprans, @serhiy-storchaka, @1st1, @miss-islington, @vladima
PRs	bpo-34872: Fix self-cancellation in C implementation of asyncio.Task #9679 [3.6] bpo-34872: Fix self-cancellation in C implementation of asyncio.Task (GH-9679) #9690 [3.7] bpo-34872: Fix self-cancellation in C implementation of asyncio.Task (GH-9679) #9691 Fix a compiler warning added in bpo-34872. #9722 [3.7] Fix a compiler warning added in bpo-34872. (GH-9722). #9726 [3.6] [3.7] Fix a compiler warning added in bpo-34872. (GH-9722). (GH-9726) #9728

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2018-10-08.02:18:37.769>
created_at = <Date 2018-10-02.19:30:38.011>
labels = ['3.7', '3.8', 'expert-asyncio']
title = 'investigate task/future cancellation in asynciomodule.c'
updated_at = <Date 2018-10-08.02:19:33.500>
user = 'https://github.com/1st1'

bugs.python.org fields:

activity = <Date 2018-10-08.02:19:33.500>
actor = 'ned.deily'
assignee = 'none'
closed = True
closed_date = <Date 2018-10-08.02:18:37.769>
closer = 'yselivanov'
components = ['asyncio']
creation = <Date 2018-10-02.19:30:38.011>
creator = 'yselivanov'
dependencies = []
files = []
hgrepos = []
issue_num = 34872
keywords = ['patch']
message_count = 16.0
messages = ['326896', '326897', '326928', '326974', '326982', '326984', '326985', '326986', '326990', '326999', '327163', '327172', '327177', '327314', '327315', '327316']
nosy_count = 7.0
nosy_names = ['ned.deily', 'asvetlov', 'Elvis.Pranskevichus', 'serhiy.storchaka', 'yselivanov', 'miss-islington', 'v2m']
pr_nums = ['9679', '9690', '9691', '9722', '9726', '9728']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue34872'
versions = ['Python 3.6', 'Python 3.7', 'Python 3.8']

[1st1]

Vladimir Matveev has discovered that C and Python implementation of asyncio.Task diverge:

• asynciomodule.c:
  

  cpython/Modules/_asynciomodule.c

  Line 2716 in 9012a0f

  r = future_cancel(fut);

• tasks.py:
  

  cpython/Lib/asyncio/tasks.py

  Line 286 in 9012a0f

  if self._fut_waiter.cancel():

In tasks.py we call "fut_obj.cancel()", so either "Task.cancel()" or "Future.cancel()" can be called depending on what "fut_obj" is.

In asynciomodule.c we essentially always call "Future.cancel(fut_obj)". This probably leads to unexpected behaviour if "fut_obj" is either a Task or a Task subclass with an overloaded "cancel()" method.

We need to first write a set of tests to understand the scope of this; then we'll need to patch asynciomodule.c and backport this to 3.7 & 3.6.

[1st1]

Elvis, please take a look at this.

[1st1]

Ned, will we have 3.7.1rc2? If so, would it be possible to include the fix for this one?

[1st1]

New changeset 0c797a6 by Yury Selivanov (Elvis Pranskevichus) in branch 'master':
bpo-34872: Fix self-cancellation in C implementation of asyncio.Task (GH-9679)
0c797a6

[miss-islington]

New changeset 166773d by Miss Islington (bot) (Elvis Pranskevichus) in branch '3.6':
[3.6] bpo-34872: Fix self-cancellation in C implementation of asyncio.Task (GH-9679) (GH-9690)
166773d

[miss-islington]

New changeset a67bd53 by Miss Islington (bot) (Elvis Pranskevichus) in branch '3.7':
[3.7] bpo-34872: Fix self-cancellation in C implementation of asyncio.Task (GH-9679) (GH-9691)
a67bd53

[1st1]

Ned, elevating this to "release blocker", see https://bugs.python.org/msg326928

Feel free to close this issue.

[ned-deily]

Prior to this, there were no plans to have a 3.7.1rc2. We have three options:

1. do a 3.7.1rc2 with this and possibly a few other cherry-picked changes
2. just cherry-pick this fix into 3.7.1 final
3. wait for 3.7.2 (in one to three months)

What's your assessment of the risk of option 2?

[1st1]

What's your assessment of the risk of option 2?

Let's not rush it in without testing, for sure.

Having 3.7.1rc2 would be great, but really up to you.

[serhiy-storchaka]

Seems this change introduced a compiler warning.

/home/serhiy/py/cpython3.7/Modules/_asynciomodule.c: In function ‘task_step_impl’:
/home/serhiy/py/cpython3.7/Modules/_asynciomodule.c:2666:44: warning: passing argument 1 of ‘_PyObject_CallMethodId’ from incompatible pointer type [-Wincompatible-pointer-types]
r = _PyObject_CallMethodId(fut, &PyId_cancel, NULL);
^~~
In file included from ./Include/Python.h:128:0,
from /home/serhiy/py/cpython3.7/Modules/_asynciomodule.c:1:
./Include/abstract.h:315:24: note: expected ‘PyObject * {aka struct _object *}’ but argument is of type ‘FutureObj * {aka struct <anonymous> *}’
PyAPI_FUNC(PyObject *) _PyObject_CallMethodId(PyObject *obj,
^~~~~~~~~~~~~~~~~~~~~~

[serhiy-storchaka]

New changeset addf8af by Serhiy Storchaka in branch 'master':
Fix a compiler warning added in bpo-34872. (GH-9722)
addf8af