FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-126688: Reinit import lock after fork by colesbury · Pull Request #126692 · python/cpython · GitHub

/ cpython Public

gh-126688: Reinit import lock after fork - #126692

Merged
colesbury merged 3 commits into
python:mainfrom
colesbury:gh-126688-import-lock
Nov 12, 2024
ConversationCommits (3)ChecksFiles changed
Merged

gh-126688: Reinit import lock after fork#126692
colesbury merged 3 commits into
python:mainfrom
colesbury:gh-126688-import-lock

Conversation

colesbury commented Nov 11, 2024 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Contributor

The PyMutex implementation supports unlocking after fork because we clear the list of watiers in parking_lot.c. This doesn't work as well for _PyRecursiveMutex because on some systems, such as SerenityOS, the thread id is not preserved across fork().

pythongh-126688: Reinit import lock after fork
5e86cca 
The PyMutex implementation supports unlocking after fork because we
clear the list of watiers in parking_lot.c. This doesn't work as well
for _PyRecursiveMutex because on some systems, such as SerenityOS, the
thread id is not preserved across fork().
bedevere-app Bot mentioned this pull request Nov 11, 2024
Closed
colesbury mentioned this pull request Nov 11, 2024
Closed
colesbury added the needs backport to 3.13 bugs and security fixes label Nov 11, 2024

oskar-skog commented Nov 11, 2024

Copy link
Copy Markdown

I can confirm applying the diff from 25aee21 to 27d5db7 to the Python 3.13.0 port in SerenityOS fixes the issue with fork.

colesbury commented Nov 11, 2024

Copy link
Copy Markdown
Contributor Author

Thanks @oskar-skog. My first attempt had a bug and I also had to merge "main" back into the PR branch to fix some other test issues. Sorry if that complicated applying the diff to 3.13.

You may already know this, but you can get the PR as a diff with a URL like https://github.com/python/cpython/pull/126692.patch. That's sometimes easier to apply than diffing between two commits involving merges.

colesbury marked this pull request as ready for review November 11, 2024 20:02
colesbury requested a review from Yhg1s November 11, 2024 20:02

oskar-skog commented Nov 11, 2024

Copy link
Copy Markdown

No, it actually made it simpler to patch, and I didn't know you could just download a PR as a patch.

colesbury requested a review from gpshead November 11, 2024 20:35
colesbury merged commit 5610860 into python:main Nov 12, 2024

miss-islington-app Bot commented Nov 12, 2024

Copy link
Copy Markdown

Thanks @colesbury for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

colesbury deleted the gh-126688-import-lock branch November 12, 2024 20:54
ambv added needs backport to 3.13 bugs and security fixes and removed needs backport to 3.13 bugs and security fixes labels Nov 12, 2024

miss-islington-app Bot commented Nov 12, 2024

Copy link
Copy Markdown

Thanks @colesbury for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 12, 2024
pythongh-126688: Reinit import lock after fork (pythonGH-126692)
6f77b81 
The PyMutex implementation supports unlocking after fork because we
clear the list of waiters in parking_lot.c. This doesn't work as well
for _PyRecursiveMutex because on some systems, such as SerenityOS, the
thread id is not preserved across fork().
(cherry picked from commit 5610860)

Co-authored-by: Sam Gross <colesbury@gmail.com>

bedevere-app Bot commented Nov 12, 2024

Copy link
Copy Markdown

GH-126765 is a backport of this pull request to the 3.13 branch.

bedevere-app Bot removed the needs backport to 3.13 bugs and security fixes label Nov 12, 2024
colesbury added a commit that referenced this pull request Nov 12, 2024
[3.13] gh-126688: Reinit import lock after fork (GH-126692) (GH-126765)
13fe8f9 
The PyMutex implementation supports unlocking after fork because we
clear the list of waiters in parking_lot.c. This doesn't work as well
for _PyRecursiveMutex because on some systems, such as SerenityOS, the
thread id is not preserved across fork().
(cherry picked from commit 5610860)

Co-authored-by: Sam Gross <colesbury@gmail.com>
ericsnowcurrently reviewed Nov 21, 2024
View reviewed changes
Comment thread Python/import.c
Comment on lines +128 to +129
// gh-126688: Thread id may change after fork() on some operating systems.
IMPORT_LOCK(interp).thread = PyThread_get_thread_ident_ex();

ericsnowcurrently Nov 21, 2024

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Wouldn't this lead to deadlocks if some other thread happened to be holding the import lock when we forked? It seems like the child process needs to know the thread ID from which it forked.

colesbury Nov 21, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

We acquire the import lock before forking so that no other thread holds it:

cpython/Modules/posixmodule.c

Lines 618 to 624 in 09c240f

void
PyOS_BeforeFork(void)
{
PyInterpreterState *interp = _PyInterpreterState_GET();
run_at_forkers(interp->before_forkers, 1);
_PyImport_AcquireLock(interp);

ericsnowcurrently Nov 21, 2024

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Hmm, looks like we always acquire the import lock in PyOS_BeforeFork(). It should be fine then.

ericsnowcurrently mentioned this pull request Nov 21, 2024
Draft
picnixz pushed a commit to picnixz/cpython that referenced this pull request Dec 8, 2024
pythongh-126688: Reinit import lock after fork (python#126692)
45320c2 
The PyMutex implementation supports unlocking after fork because we
clear the list of waiters in parking_lot.c. This doesn't work as well
for _PyRecursiveMutex because on some systems, such as SerenityOS, the
thread id is not preserved across fork().
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
pythongh-126688: Reinit import lock after fork (python#126692)
6b28e03 
The PyMutex implementation supports unlocking after fork because we
clear the list of waiters in parking_lot.c. This doesn't work as well
for _PyRecursiveMutex because on some systems, such as SerenityOS, the
thread id is not preserved across fork().
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ericsnowcurrently ericsnowcurrently left review comments
brettcannon brettcannon approved these changes
gpshead gpshead approved these changes
kumaraditya303 kumaraditya303 approved these changes
ncoghlan Awaiting requested review from ncoghlan ncoghlan is a code owner
warsaw Awaiting requested review from warsaw warsaw is a code owner
Yhg1s Awaiting requested review from Yhg1s

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL