Skip to content

GH-117881: fix athrow().throw()/asend().throw() concurrent access #117882

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

Jump to bottom
Merged
merged 10 commits into from
May 1, 2024
Merged

GH-117881: fix athrow().throw()/asend().throw() concurrent access #117882

merged 10 commits into from
May 1, 2024

Conversation

graingert
Copy link
Contributor

@graingert graingert commented Apr 15, 2024 •
edited by bedevere-app bot
Loading

@bedevere-app bedevere-app bot mentioned this pull request Apr 15, 2024
Closed
graingert
graingert commented Apr 15, 2024
View reviewed changes
Objects/genobject.c
@@ -1771,6 +1771,7 @@ async_gen_asend_send(PyAsyncGenASend *o, PyObject *arg)

if (o->ags_state == AWAITABLE_STATE_INIT) {
if (o->ags_gen->ag_running_async) {
o->ags_state = AWAITABLE_STATE_CLOSED;
Copy link
Contributor Author

@graingert graingert Apr 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this was set in async_gen_athrow_send but forgotten in async_gen_asend_send, and also results in an unawaited coroutine warning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cpython/Objects/genobject.c

Lines 2098 to 2113 in 8418dcc

if (o->agt_state == AWAITABLE_STATE_INIT) {
if (o->agt_gen->ag_running_async) {
o->agt_state = AWAITABLE_STATE_CLOSED;
if (o->agt_args == NULL) {
PyErr_SetString(
PyExc_RuntimeError,
"aclose(): asynchronous generator is already running");
}
else {
PyErr_SetString(
PyExc_RuntimeError,
"athrow(): asynchronous generator is already running");
}
return NULL;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably could do with a test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

test added in 1220aee

@graingert graingert marked this pull request as ready for review April 15, 2024 08:01
@graingert graingert requested a review from markshannon as a code owner April 15, 2024 08:01
graingert
graingert commented Apr 15, 2024
View reviewed changes
graingert
graingert commented Apr 15, 2024
View reviewed changes
graingert
graingert commented Apr 15, 2024
View reviewed changes
@graingert graingert mentioned this pull request Apr 23, 2024
Merged
encukou
encukou reviewed Apr 24, 2024
View reviewed changes
Copy link
Member

@encukou encukou left a comment •
edited
Loading

Choose a reason for hiding this comment

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

As in #117851: I'm not an expert in this area.
It makes sense to start the generator in send/throw.

What I don't get is the purpose of all the synchronization between ags_state and ags_gen->ag_running_async/ags_gen->ag_closed. What's the difference between what they represent? Should they go out of sync at some point?

@graingert
Copy link
Contributor Author

graingert commented Apr 24, 2024 •
edited
Loading

I think it's possible for them to go out of sync, but I think I have a fix for that here #117906

@encukou
Copy link
Member

encukou commented Apr 24, 2024

Is it a bug if they go out of sync?
If it is, and ag_running_async & ag_closed hold the necessary info, why have ags_state at all?

@graingert
Copy link
Contributor Author

Is it a bug if they go out of sync? If it is, and ag_running_async & ag_closed hold the necessary info, why have ags_state at all?

you can have multiple of each agen.aclose() and agen.anext() objects constructed at the same time, and they each need to know what state they are in so they can update the agen fields appropriately

@graingert
Copy link
Contributor Author

graingert commented Apr 24, 2024 •
edited
Loading

this is where gen->ags_state and ag_running_async go out of sync:

import types
import itertools

@types.coroutine
def _async_yield(v):
    return (yield v)


async def agenfn():
    for i in itertools.count():
        await _async_yield(i)
    return
    yield

agen = agenfn()
gen = agen.__anext__()
print(f"{gen.send(None)=}")
print(f"{agen.ag_running=}")
gen.close()  # uh-oh, gen->ags_state is CLOSED
print(f"{agen.ag_running=}")  # but the async generator is still running
agen.aclose().send(None)  # and it's broken so we can't aclose it

@encukou
Copy link
Member

encukou commented Apr 25, 2024

Ah! Got it, thank you!

encukou
encukou approved these changes Apr 25, 2024
View reviewed changes
Copy link
Member

@encukou encukou left a comment

Choose a reason for hiding this comment

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

The PR looks good to me. But, it would be great to have a more comprehensive way to find issues like this. I commented on the issue.

hugovk
hugovk previously requested changes Apr 25, 2024
View reviewed changes
@bedevere-app

This comment was marked as resolved.

Sign in to view
@hugovk hugovk dismissed their stale review April 25, 2024 10:03

Was testing the bot

@graingert graingert requested a review from hugovk April 25, 2024 15:29
@hugovk hugovk removed their request for review April 25, 2024 17:54
@hugovk
Copy link
Member

hugovk commented Apr 25, 2024

No need for me to review, I was only testing the bot earlier. 👍

graingert
graingert commented Apr 26, 2024
View reviewed changes
graingert
graingert commented Apr 26, 2024
View reviewed changes
Lib/test/test_asyncgen.py
@@ -1809,9 +1957,56 @@ class MyException(Exception):
g = gen()
with self.assertRaises(MyException):
g.aclose().throw(MyException)
del g
Copy link
Contributor Author

Choose a reason for hiding this comment

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

driveby: oops this wasn't running

@graingert
Copy link
Contributor Author

graingert commented Apr 30, 2024 •
edited
Loading

@encukou is there anything else I need to do on this? I'd like to start looking at fixing athrow().close() and asend().close() next which builds on this work

@encukou
Copy link
Member

encukou commented May 1, 2024

No, sorry for the delay!

@encukou encukou merged commit fc7e1aa into python:main May 1, 2024
36 checks passed
@graingert graingert deleted the fix-athrow-throw-asend-throw-concurrent branch May 1, 2024 06:49
@graingert graingert added the needs backport to 3.12 only security fixes label May 1, 2024
@miss-islington-app
Copy link

Thanks @graingert for the PR, and @encukou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Sorry, @graingert and @encukou, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker fc7e1aa3c001bbce25973261fba457035719a559 3.12

@bedevere-app
Copy link

bedevere-app bot commented May 1, 2024

GH-118458 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label May 1, 2024
encukou pushed a commit that referenced this pull request May 2, 2024
@graingert
[3.12] GH-117881: fix athrow().throw()/asend().throw() concurrent acc…
c48e0ef 
…ess (GH-117882) (#118458)

GH-117881: fix athrow().throw()/asend().throw() concurrent access (GH-117882)

(cherry picked from commit fc7e1aa)
SonicField pushed a commit to SonicField/cpython that referenced this pull request May 8, 2024
@graingert @SonicField
pythonGH-117881: fix athrow().throw()/asend().throw() concurrent acce…
9fa6181 
…ss (pythonGH-117882)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@encukou encukou encukou approved these changes

@hugovk hugovk hugovk left review comments

@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

Assignees

@encukou encukou

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@graingert @encukou @hugovk