Skip to content

bpo-34872: Fix self-cancellation in C implementation of asyncio.Task#9679

Merged
1st1 merged 1 commit into
python:masterfrom
elprans:fix-ctask-self-cancel
Oct 3, 2018
Merged

bpo-34872: Fix self-cancellation in C implementation of asyncio.Task#9679
1st1 merged 1 commit into
python:masterfrom
elprans:fix-ctask-self-cancel

Conversation

@elprans

@elprans elprans commented Oct 2, 2018 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

The C implementation of asyncio.Task currently fails to perform the
cancellation cleanup correctly in the following scenario.

async def task1():
    async def task2():
        await task3     # task3 is never cancelled

    asyncio.current_task().cancel()
    await asyncio.create_task(task2())

The actuall error is a hardcoded call to future_cancel() instead of
calling the cancel() method of a future-like object.

Thanks to Vladimir Matveev for noticing the code discrepancy and to
Yury Selivanov for coming up with a pathological scenario.

https://bugs.python.org/issue34872

@elprans
bpo-34872: Fix self-cancellation in C implementation of asyncio.Task
548ce9d 
The C implementation of asyncio.Task currently fails to perform the
cancellation cleanup correctly in the following scenario.

    async def task1():
        async def task2():
            await task3     # task3 is never cancelled

        asyncio.current_task().cancel()
        await asyncio.create_task(task2())

The actuall error is a hardcoded call to `future_cancel()` instead of
calling the `cancel()` method of a future-like object.

Thanks to Vladimir Matveev for noticing the code discrepancy and to
Yury Selivanov for coming up with a pathological scenario.
@elprans elprans requested review from 1st1 and asvetlov as code owners October 2, 2018 23:37
@1st1 1st1 self-assigned this Oct 2, 2018
asvetlov
asvetlov reviewed Oct 3, 2018
View reviewed changes
Comment thread Modules/_asynciomodule.c
PyObject *r;
r = future_cancel(fut);
int is_true;
r = _PyObject_CallMethodId(fut, &PyId_cancel, NULL);

@asvetlov asvetlov Oct 3, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix is correct.
But I'd like to go one step forward: use Future_CheckExact and Task_CheckExact to call future_cancel() and _asyncio_Task_cancel_impl() as fast paths.

@1st1 1st1 Oct 3, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Id keep it as simple as possible as it needs to be backported to 3.6 & 3.7. A proper refactoring for C code is needed anyways, and I'll add this optimization.

@asvetlov asvetlov Oct 3, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

@1st1 1st1 merged commit 0c797a6 into python:master Oct 3, 2018
@miss-islington

miss-islington commented Oct 3, 2018

Copy link
Copy Markdown
Contributor

Thanks @elprans for the PR, and @1st1 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7.
🐍🍒⛏🤖

@miss-islington

miss-islington commented Oct 3, 2018

Copy link
Copy Markdown
Contributor

Sorry, @elprans and @1st1, I could not cleanly backport this to 3.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 0c797a6aca1c293e530e18c5e9fa02c670a9a4ed 3.7

@miss-islington

miss-islington commented Oct 3, 2018

Copy link
Copy Markdown
Contributor

Sorry, @elprans and @1st1, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 0c797a6aca1c293e530e18c5e9fa02c670a9a4ed 3.6

@1st1

1st1 commented Oct 3, 2018

Copy link
Copy Markdown
Member

@elprans please submit two PRs to backport the change to 3.7 and 3.6. The automatic cherry-picking has failed :(

@bedevere-bot

bedevere-bot commented Oct 3, 2018

Copy link
Copy Markdown

GH-9690 is a backport of this pull request to the 3.6 branch.

elprans added a commit to elprans/cpython that referenced this pull request Oct 3, 2018
@elprans
[3.6] bpo-34872: Fix self-cancellation in C implementation of asyncio…
3ef533c 
….Task (pythonGH-9679)

The C implementation of asyncio.Task currently fails to perform the
cancellation cleanup correctly in the following scenario.

    async def task1():
        async def task2():
            await task3     # task3 is never cancelled

        asyncio.current_task().cancel()
        await asyncio.create_task(task2())

The actuall error is a hardcoded call to `future_cancel()` instead of
calling the `cancel()` method of a future-like object.

Thanks to Vladimir Matveev for noticing the code discrepancy and to
Yury Selivanov for coming up with a pathological scenario.

(cherry picked from commit 548ce9d)
@bedevere-bot

bedevere-bot commented Oct 3, 2018

Copy link
Copy Markdown

GH-9691 is a backport of this pull request to the 3.7 branch.

elprans added a commit to elprans/cpython that referenced this pull request Oct 3, 2018
@elprans
[3.7] bpo-34872: Fix self-cancellation in C implementation of asyncio…
3bb1d52 
….Task (pythonGH-9679)

The C implementation of asyncio.Task currently fails to perform the
cancellation cleanup correctly in the following scenario.

    async def task1():
        async def task2():
            await task3     # task3 is never cancelled

        asyncio.current_task().cancel()
        await asyncio.create_task(task2())

The actuall error is a hardcoded call to `future_cancel()` instead of
calling the `cancel()` method of a future-like object.

Thanks to Vladimir Matveev for noticing the code discrepancy and to
Yury Selivanov for coming up with a pathological scenario..
(cherry picked from commit 548ce9d)

Co-authored-by: Elvis Pranskevichus <elvis@magic.io>
miss-islington pushed a commit that referenced this pull request Oct 3, 2018
@elprans @miss-islington
[3.6] bpo-34872: Fix self-cancellation in C implementation of asyncio…
166773d 
….Task (GH-9679) (GH-9690)

The C implementation of asyncio.Task currently fails to perform the
cancellation cleanup correctly in the following scenario.

    async def task1():
        async def task2():
            await task3     # task3 is never cancelled

        asyncio.current_task().cancel()
        await asyncio.create_task(task2())

The actuall error is a hardcoded call to `future_cancel()` instead of
calling the `cancel()` method of a future-like object.

Thanks to Vladimir Matveev for noticing the code discrepancy and to
Yury Selivanov for coming up with a pathological scenario.

(cherry picked from commit 548ce9d)


https://bugs.python.org/issue34872
miss-islington pushed a commit that referenced this pull request Oct 3, 2018
@elprans @miss-islington
[3.7] bpo-34872: Fix self-cancellation in C implementation of asyncio…
a67bd53 
….Task (GH-9679) (GH-9691)

The C implementation of asyncio.Task currently fails to perform the
cancellation cleanup correctly in the following scenario.

    async def task1():
        async def task2():
            await task3     # task3 is never cancelled

        asyncio.current_task().cancel()
        await asyncio.create_task(task2())

The actuall error is a hardcoded call to `future_cancel()` instead of
calling the `cancel()` method of a future-like object.

Thanks to Vladimir Matveev for noticing the code discrepancy and to
Yury Selivanov for coming up with a pathological scenario..
(cherry picked from commit 548ce9d)

Co-authored-by: Elvis Pranskevichus <elvis@magic.io>


https://bugs.python.org/issue34872
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@1st1 1st1 1st1 approved these changes

@asvetlov asvetlov asvetlov left review comments

Assignees

@1st1 1st1

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@elprans @miss-islington @1st1 @bedevere-bot @asvetlov @the-knights-who-say-ni