Skip to content
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

bpo-46070: Fix asyncio initialisation guard #30423

Merged
merged 2 commits into from
Jan 7, 2022
Merged

bpo-46070: Fix asyncio initialisation guard #30423

merged 2 commits into from
Jan 7, 2022

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Jan 5, 2022 •
edited
Loading

If init flag is set, exit module init successfully immediately.
If not, only set the flag after successful module initialisation.

https://bugs.python.org/issue46070

bpo-46070: Fix asyncio initialisation guard
7783785 
If init flag is set, exit successfully immediately.
If not, only set the flag after successful initialisation.
@erlend-aasland erlend-aasland added needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes labels Jan 5, 2022
@erlend-aasland
Copy link
Contributor Author

Maybe we should run this through the buildbots.

@erlend-aasland
Copy link
Contributor Author

This might qualify for a NEWS entry, as it fixes segfaults on 3.10 and 3.9 (and 3.8, but 3.8 is in security-fix-mode-only).

@vstinner
Copy link
Member

vstinner commented Jan 7, 2022

module_init() is called in parallel multiple times from different interpreters (in different threads). I understand that module_init() leaks references when calling multiple times in parallel, since it doesn't use Py_SETREF(). For now, I suggest to not worry about that. I suggest to rather invest time on converting _asyncio static types to heap types and then port _asyncio to multi-phase init: move static globals into a module state.

vstinner
vstinner approved these changes Jan 7, 2022
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Jan 7, 2022 •
edited
Loading

module_init() is called in parallel multiple times from different interpreters (in different threads). I understand that module_init() leaks references when calling multiple times in parallel, since it doesn't use Py_SETREF(). For now, I suggest to not worry about that. I suggest to rather invest time on converting _asyncio static types to heap types and then port _asyncio to multi-phase init: move static globals into a module state.

Yes, there are ref leaks, but as you say they are best fixed by converting global state to module state and implement proper multi-phase init. I've got a private branch for this work. This demands a separate bpo issue, and we should involve the asyncio maintaner before continuing with that effort :)

Thanks for reviewing!

@vstinner
Copy link
Member

vstinner commented Jan 7, 2022

This might qualify for a NEWS entry, as it fixes segfaults on 3.10 and 3.9 (and 3.8, but 3.8 is in security-fix-mode-only).

Oh, I didn't notice the failing "news" job. Yes, please document the fix. If I understand correctly, this change fix a crash an importing the asyncio module from different sub-interpreters in parallel. And the workaround was to import asyncio in the main interpreter before spawning sub-interpreters.

@erlend-aasland
Copy link
Contributor Author

[...] Yes, please document the fix. If I understand correctly, this change fix a crash an importing the asyncio module from different sub-interpreters in parallel. And the workaround was to import asyncio in the main interpreter before spawning sub-interpreters.

Sure, I'll try to write something sensible :) Also, we could probably use a more compact version of your reproducer as a regression test.

@vstinner
Copy link
Member

vstinner commented Jan 7, 2022

Adding a test is optional for me. It's up to me. But I would like to see a NEWS entry please.

@erlend-aasland
Copy link
Contributor Author

Adding a test is optional for me. It's up to me. But I would like to see a NEWS entry please.

You got it 🥁

vstinner
vstinner approved these changes Jan 7, 2022
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

@vstinner vstinner merged commit b127e70 into python:main Jan 7, 2022
@miss-islington
Copy link
Contributor

Thanks @erlend-aasland for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9, 3.10.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 7, 2022
@miss-islington
bpo-46070: Fix asyncio initialisation guard (pythonGH-30423)
7057c07 
If init flag is set, exit successfully immediately.
If not, only set the flag after successful initialization.
(cherry picked from commit b127e70)

Co-authored-by: Erlend Egeberg Aasland <[email protected]>
@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Jan 7, 2022
@bedevere-bot
Copy link

GH-30453 is a backport of this pull request to the 3.10 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 7, 2022
@miss-islington
bpo-46070: Fix asyncio initialisation guard (pythonGH-30423)
cacc876 
If init flag is set, exit successfully immediately.
If not, only set the flag after successful initialization.
(cherry picked from commit b127e70)

Co-authored-by: Erlend Egeberg Aasland <[email protected]>
@bedevere-bot
Copy link

GH-30454 is a backport of this pull request to the 3.9 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label Jan 7, 2022
miss-islington added a commit that referenced this pull request Jan 7, 2022
@miss-islington
bpo-46070: Fix asyncio initialisation guard (GH-30423)
9d18045 
If init flag is set, exit successfully immediately.
If not, only set the flag after successful initialization.
(cherry picked from commit b127e70)

Co-authored-by: Erlend Egeberg Aasland <[email protected]>
miss-islington added a commit that referenced this pull request Jan 7, 2022
@miss-islington
bpo-46070: Fix asyncio initialisation guard (GH-30423)
4d2cfd3 
If init flag is set, exit successfully immediately.
If not, only set the flag after successful initialization.
(cherry picked from commit b127e70)

Co-authored-by: Erlend Egeberg Aasland <[email protected]>
@erlend-aasland erlend-aasland deleted the asyncio-init-guard branch January 7, 2022 16:09
@prahal prahal mentioned this pull request Mar 22, 2022
Closed
@graysky graysky mannequin mentioned this pull request Apr 10, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner approved these changes

@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner

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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@erlend-aasland @vstinner @miss-islington @bedevere-bot @the-knights-who-say-ni