Skip to content

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

Merged
encukou merged 10 commits intopython:mainfrom
graingert:fix-athrow-throw-asend-throw-concurrent
May 1, 2024
Merged

GH-117881: fix athrow().throw()/asend().throw() concurrent access#117882
encukou merged 10 commits intopython:mainfrom
graingert:fix-athrow-throw-asend-throw-concurrent

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

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

@graingert graingert Apr 15, 2024

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

@graingert graingert Apr 26, 2024

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

@graingert graingert Apr 26, 2024

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

graingert 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?

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
g = gen()
with self.assertRaises(MyException):
g.aclose().throw(MyException)
del g
Copy link
Contributor Author

@graingert graingert Apr 26, 2024

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
@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

miss-islington-app bot commented May 1, 2024

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

miss-islington-app bot commented May 1, 2024

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