-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Ports/Python3: crash in child after fork #25263
Comments
@linusg Didn't you say pyrepl was disabled? |
I did, and it is. Please read the code the traceback is pointing to: https://github.com/python/cpython/blob/c5b99f5c2c5347d66b9da362773969c531fb6c85/Lib/_pyrepl/main.py#L30 |
It seems that at least Python 3.13 relies on pthread_self returning the same value in both the parent and the child process. This is the case on: Linux, Hurd, FreeBSD, OpenBSD, NetBSD, MidnightBSD, Cygwin, Haiku, newer versions of Solaris, and macOS #include <stdio.h>
#include <pthread.h>
#include <unistd.h>
int main(int argc, char **argv)
{
pthread_t parent = pthread_self();
if (fork() == 0) {
pthread_t child = pthread_self();
if (pthread_equal(parent, child))
puts("parent == child");
else
puts("parent != child");
}
return 0;
} Curiously, POSIX doesn't seem to mention if the value returned by pthread_self should be inherited by the child process. |
On serenity, pthread_self returns We actually take special care in the fork() LibC wrapper to ensure that the behavior python is expecting here does not happen: serenity/Userland/Libraries/LibC/unistd.cpp Lines 92 to 106 in b471972
The cached thread id and cached process id are cleared after-fork in the child, causing the next access to actually perform the gettid syscall and get the up to date value. |
"Fixing" this issue would in theory make LibCore event loops more exciting, as well as make the implementation of the Shell more tricky. However, those pieces of software are known to "work" on non-serenity platforms that have this unfortunate behavior, so perhaps they're subtly broken as well 🤔 |
It looks like the code that broke us is from this commit python/cpython@e21057b added in python/cpython#118523 as part of python/cpython#117657 We could probably get around this by making a patch to re-add the logic of @colesbury If you have any spare cycles, could you help us figure out if the bug is in cpython or in our OS? :D |
Scrolling through the cpython patch it looks like we've managed to re-incarnate an ancient Solaris 9/HP-UX 11 behavior quirk that was worked around with that https://bugs.python.org/issue7242 Though I'm not sure I'd call a patch to LibC/LibPthread to avoid re-setting pthread_self() in the child after fork a 'bug fix' |
Yes, we can fix this upstream in CPython. Can you open an issue in https://github.com/python/cpython? You can tag me in it. |
Sure, made python/cpython#126688. Thanks! |
Does Serenity preserve thread local storage after fork? |
Would you please verify that python/cpython#126692 fixes the crash on Serenity? |
fork in Python: confirmed working thread_test.py.txt |
Yes, thread local storage is cloned. Instead of storing pthread data structures in %fs or other libc-allocated data, Serenity's pthread APIs mostly just map to querying the kernel about its first-class Thread classes that each Process class owns, using the thread ID. Thread-local storage is allocated and stored in %fs and other thread-pointer-registers. Just not the "info about the current thread" structure musl libc and glibc have. |
It's merged into main. I guess we can just update Python to 3.13.1 when it gets released? If anyone wants to get it working right now, I believe you might get away with just commenting |
Alternatively one could temporarily backport the change to our patch set, not sure how fast the release python release cycle is |
It's expected to be released 2024-12-03, but backporting this change should be easy, it will just get redundant later. |
python/cpython#126765 Closes SerenityOS#25263 Make sure to revert this commit before updating Python.
Backtrace:
The text was updated successfully, but these errors were encountered: