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.

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

gh-102512: Fix threading after os.fork() called from a foreign thread #113261

Merged
merged 9 commits into from
Jan 22, 2024

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Dec 18, 2023

Based on #102517. It recreates a _DummyThread as _MainThread after fork.

marmarek and others added 5 commits March 8, 2023 00:13
If os.fork() is called from a DummyThread (a thread started not by the
threading module, in which somebody called threading.current_thread()),
it becomes main thread. Threading shutdown logic relies on the main
thread having tstate_lock, so initialize it for the DummyThread too.

Fixes python#102512
Lib/threading.py Outdated Show resolved Hide resolved
Lib/test/test_threading.py Show resolved Hide resolved
Lib/test/test_threading.py Outdated Show resolved Hide resolved
@serhiy-storchaka
Copy link
Member Author

Thank you for your review Victor. I added more output in tests and added a test for foreign thread without DummyThread.

The code was significantly rewritten. Replacing a DummyThread with a MainThread leaves a DummyThread thread with the same ident (this is disturbing) in _dangling set. If there were references to that thread, they still refer to the DummyThread. So it is better to change the type of the existing instance of DummyThread.

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.

I just suggest minor change to make the test easier to read, feel free to ignore.

I dislike setting self.__class__, but well, it works as expected and the new tests are exhaustive, so I'm fine with it.

Lib/test/test_threading.py Outdated Show resolved Hide resolved
Lib/test/test_threading.py Outdated Show resolved Hide resolved
@serhiy-storchaka serhiy-storchaka merged commit 49785b0 into python:main Jan 22, 2024
29 checks passed
@serhiy-storchaka serhiy-storchaka deleted the bug102512-2 branch January 22, 2024 14:14
@serhiy-storchaka serhiy-storchaka added needs backport to 3.11 only security fixes needs backport to 3.12 bug and security fixes labels Jan 22, 2024
@miss-islington-app
Copy link

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Sorry, @serhiy-storchaka, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 49785b06ded19c7c4afce186bac90fea707470ea 3.11

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 22, 2024
…alled from a foreign thread (pythonGH-113261)

Always set a _MainThread as a main thread after os.fork() is called from
a thread started not by the threading module.

A new _MainThread was already set as a new main thread after fork if
threading.current_thread() was not called for a foreign thread before fork.
Now, if it was called before fork, the implicitly created _DummyThread will
be turned into _MainThread after fork.

It fixes, in particularly, an incompatibility of _DummyThread with
the threading shutdown logic which relies on the main thread
having tstate_lock.

(cherry picked from commit 49785b0)

Co-authored-by: Serhiy Storchaka <[email protected]>
Co-authored-by: Marek Marczykowski-Górecki <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Jan 22, 2024

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

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Jan 22, 2024
serhiy-storchaka added a commit to miss-islington/cpython that referenced this pull request Jan 22, 2024
…ork() called from a foreign thread (pythonGH-113261)

Always set a _MainThread as a main thread after os.fork() is called from
a thread started not by the threading module.

A new _MainThread was already set as a new main thread after fork if
threading.current_thread() was not called for a foreign thread before fork.
Now, if it was called before fork, the implicitly created _DummyThread will
be turned into _MainThread after fork.

It fixes, in particularly, an incompatibility of _DummyThread with
the threading shutdown logic which relies on the main thread
having tstate_lock.

(cherry picked from commit 49785b0)

Co-authored-by: Marek Marczykowski-Górecki <[email protected]>
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Jan 22, 2024
…ork() called from a foreign thread (pythonGH-113261)

Always set a _MainThread as a main thread after os.fork() is called from
a thread started not by the threading module.

A new _MainThread was already set as a new main thread after fork if
threading.current_thread() was not called for a foreign thread before fork.
Now, if it was called before fork, the implicitly created _DummyThread will
be turned into _MainThread after fork.

It fixes, in particularly, an incompatibility of _DummyThread with
the threading shutdown logic which relies on the main thread
having tstate_lock.

(cherry picked from commit 49785b0)

Co-authored-by: Serhiy Storchaka <[email protected]>
Co-authored-by: Marek Marczykowski-Górecki <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Jan 22, 2024

GH-114431 is a backport of this pull request to the 3.11 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.11 only security fixes label Jan 22, 2024
serhiy-storchaka added a commit that referenced this pull request Jan 22, 2024
…called from a foreign thread (GH-113261) (GH-114431)

Always set a _MainThread as a main thread after os.fork() is called from
a thread started not by the threading module.

A new _MainThread was already set as a new main thread after fork if
threading.current_thread() was not called for a foreign thread before fork.
Now, if it was called before fork, the implicitly created _DummyThread will
be turned into _MainThread after fork.

It fixes, in particularly, an incompatibility of _DummyThread with
the threading shutdown logic which relies on the main thread
having tstate_lock.

(cherry picked from commit 49785b0)

Co-authored-by: Marek Marczykowski-Górecki <[email protected]>
serhiy-storchaka added a commit to miss-islington/cpython that referenced this pull request Jan 22, 2024
…alled from a foreign thread (pythonGH-113261)

Always set a _MainThread as a main thread after os.fork() is called from
a thread started not by the threading module.

A new _MainThread was already set as a new main thread after fork if
threading.current_thread() was not called for a foreign thread before fork.
Now, if it was called before fork, the implicitly created _DummyThread will
be turned into _MainThread after fork.

It fixes, in particularly, an incompatibility of _DummyThread with
the threading shutdown logic which relies on the main thread
having tstate_lock.

(cherry picked from commit 49785b0)

Co-authored-by: Serhiy Storchaka <[email protected]>
Co-authored-by: Marek Marczykowski-Górecki <[email protected]>
serhiy-storchaka added a commit that referenced this pull request Jan 22, 2024
…called from a foreign thread (GH-113261) (GH-114430)

Always set a _MainThread as a main thread after os.fork() is called from
a thread started not by the threading module.

A new _MainThread was already set as a new main thread after fork if
threading.current_thread() was not called for a foreign thread before fork.
Now, if it was called before fork, the implicitly created _DummyThread will
be turned into _MainThread after fork.

It fixes, in particularly, an incompatibility of _DummyThread with
the threading shutdown logic which relies on the main thread
having tstate_lock.

(cherry picked from commit 49785b0)

Co-authored-by: Serhiy Storchaka <[email protected]>
Co-authored-by: Marek Marczykowski-Górecki <[email protected]>
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
…alled from a foreign thread (pythonGH-113261)

Always set a _MainThread as a main thread after os.fork() is called from
a thread started not by the threading module.

A new _MainThread was already set as a new main thread after fork if
threading.current_thread() was not called for a foreign thread before fork.
Now, if it was called before fork, the implicitly created _DummyThread will
be turned into _MainThread after fork.

It fixes, in particularly, an incompatibility of _DummyThread with
the threading shutdown logic which relies on the main thread
having tstate_lock.

Co-authored-by: Marek Marczykowski-Górecki <[email protected]>
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
…alled from a foreign thread (pythonGH-113261)

Always set a _MainThread as a main thread after os.fork() is called from
a thread started not by the threading module.

A new _MainThread was already set as a new main thread after fork if
threading.current_thread() was not called for a foreign thread before fork.
Now, if it was called before fork, the implicitly created _DummyThread will
be turned into _MainThread after fork.

It fixes, in particularly, an incompatibility of _DummyThread with
the threading shutdown logic which relies on the main thread
having tstate_lock.

Co-authored-by: Marek Marczykowski-Górecki <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants