classification
Title: investigate task/future cancellation in asynciomodule.c
Type: Stage: resolved
Components: asyncio Versions: Python 3.8, Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Elvis.Pranskevichus, asvetlov, miss-islington, ned.deily, serhiy.storchaka, v2m, yselivanov
Priority: normal Keywords: patch

Created on 2018-10-02 19:30 by yselivanov, last changed 2018-10-08 02:19 by ned.deily. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 9679 merged Elvis.Pranskevichus, 2018-10-02 23:37
PR 9690 merged Elvis.Pranskevichus, 2018-10-03 15:06
PR 9691 merged Elvis.Pranskevichus, 2018-10-03 15:14
PR 9722 merged serhiy.storchaka, 2018-10-05 18:00
PR 9726 merged serhiy.storchaka, 2018-10-05 18:28
PR 9728 merged miss-islington, 2018-10-05 18:58
Messages (16)
msg326896 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-10-02 19:30
Vladimir Matveev has discovered that C and Python implementation of asyncio.Task diverge:

* asynciomodule.c:
https://github.com/python/cpython/blob/9012a0fb4c4ec1afef9efb9fdb0964554ea17983/Modules/_asynciomodule.c#L2716

* tasks.py:
https://github.com/python/cpython/blob/9012a0fb4c4ec1afef9efb9fdb0964554ea17983/Lib/asyncio/tasks.py#L286

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.
msg326897 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-10-02 19:30
Elvis, please take a look at this.
msg326928 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-10-03 00:31
Ned, will we have 3.7.1rc2?  If so, would it be possible to include the fix for this one?
msg326974 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-10-03 14:30
New changeset 0c797a6aca1c293e530e18c5e9fa02c670a9a4ed by Yury Selivanov (Elvis Pranskevichus) in branch 'master':
bpo-34872: Fix self-cancellation in C implementation of asyncio.Task (GH-9679)
https://github.com/python/cpython/commit/0c797a6aca1c293e530e18c5e9fa02c670a9a4ed
msg326982 - (view) Author: miss-islington (miss-islington) Date: 2018-10-03 15:28
New changeset 166773df0ce6c852130f524029fa2e62b37b89cb 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)
https://github.com/python/cpython/commit/166773df0ce6c852130f524029fa2e62b37b89cb
msg326984 - (view) Author: miss-islington (miss-islington) Date: 2018-10-03 15:49
New changeset a67bd53d3f80ac9c518b5426aa35459bd373b2b3 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)
https://github.com/python/cpython/commit/a67bd53d3f80ac9c518b5426aa35459bd373b2b3
msg326985 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-10-03 15:52
Ned, elevating this to "release blocker", see https://bugs.python.org/msg326928

Feel free to close this issue.
msg326986 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2018-10-03 15:58
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?
msg326990 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-10-03 16:13
> 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.
msg326999 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-10-03 18:16
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,
                        ^~~~~~~~~~~~~~~~~~~~~~
msg327163 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-10-05 18:20
New changeset addf8afb43af58b9bf56a0ecfd0f316dd60ac0c3 by Serhiy Storchaka in branch 'master':
Fix a compiler warning added in bpo-34872. (GH-9722)
https://github.com/python/cpython/commit/addf8afb43af58b9bf56a0ecfd0f316dd60ac0c3
msg327172 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-10-05 18:58
New changeset d9212200fe8ddb55d73b8231869cfbb32635ba92 by Serhiy Storchaka in branch '3.7':
[3.7] Fix a compiler warning added in bpo-34872. (GH-9722). (GH-9726)
https://github.com/python/cpython/commit/d9212200fe8ddb55d73b8231869cfbb32635ba92
msg327177 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-10-05 19:15
New changeset dd0670f12b159eff5336d6011f046e1ccac495e1 by Serhiy Storchaka (Miss Islington (bot)) in branch '3.6':
Fix a compiler warning added in bpo-34872. (GH-9722). (GH-9726) (GH-9728)
https://github.com/python/cpython/commit/dd0670f12b159eff5336d6011f046e1ccac495e1
msg327314 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2018-10-08 02:13
Is there any reason this issue needs to remain open?  Or can we at least remove the "release blocker" priority?
msg327315 - (view) Author: Yury Selivanov (yselivanov) * (Python committer) Date: 2018-10-08 02:18
Closing it now, Ned, thanks!  I assume it will make it into 3.7.1rc2, right?
msg327316 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2018-10-08 02:19
Both 3.7.1rc2 and 3.6.7rc2
History
Date User Action Args
2018-10-08 02:19:33ned.deilysetmessages: + msg327316
2018-10-08 02:18:37yselivanovsetstatus: open -> closed
priority: release blocker -> normal
messages: + msg327315

resolution: fixed
stage: patch review -> resolved
2018-10-08 02:13:22ned.deilysetmessages: + msg327314
2018-10-05 19:15:40serhiy.storchakasetmessages: + msg327177
2018-10-05 18:58:28miss-islingtonsetpull_requests: + pull_request9113
2018-10-05 18:58:23serhiy.storchakasetmessages: + msg327172
2018-10-05 18:28:49serhiy.storchakasetpull_requests: + pull_request9111
2018-10-05 18:20:09serhiy.storchakasetmessages: + msg327163
2018-10-05 18:00:25serhiy.storchakasetpull_requests: + pull_request9107
2018-10-03 18:16:40serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg326999
2018-10-03 16:13:38yselivanovsetmessages: + msg326990
2018-10-03 15:58:03ned.deilysetmessages: + msg326986
2018-10-03 15:52:57yselivanovsetpriority: normal -> release blocker

messages: + msg326985
2018-10-03 15:49:06miss-islingtonsetmessages: + msg326984
2018-10-03 15:28:52miss-islingtonsetnosy: + miss-islington
messages: + msg326982
2018-10-03 15:14:48Elvis.Pranskevichussetpull_requests: + pull_request9078
2018-10-03 15:06:51Elvis.Pranskevichussetpull_requests: + pull_request9077
2018-10-03 14:30:38yselivanovsetmessages: + msg326974
2018-10-03 00:31:48yselivanovsetnosy: + ned.deily
messages: + msg326928
2018-10-02 23:37:31Elvis.Pranskevichussetkeywords: + patch
stage: patch review
pull_requests: + pull_request9068
2018-10-02 20:15:12v2msetnosy: + v2m
2018-10-02 19:30:48yselivanovsetmessages: + msg326897
2018-10-02 19:30:38yselivanovcreate