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-100228: Warn from os.fork() if other threads exist. #100229

Merged
merged 20 commits into from
Dec 29, 2022

Conversation

gpshead
Copy link
Member

@gpshead gpshead commented Dec 14, 2022

Not comprehensive, best effort warning. There are cases when threads exist that this code currently does not detect.

Starting with a DeprecationWarning for now, it is less disruptive than something like RuntimeWarning and most likely to only be seen in people's CI tests - a good place to start with this messaging.

TODO:

  • doctests for multiprocessing failed with an assertion crash in 2854473 about the error being set unexpectedly on one later commit, why? did I fix that? I need to figure out how to run those and try to reproduce it. (could not reproduce, no way it could be related seen upon code inspection)

Not comprehensive, best effort.
@netlify
Copy link

netlify bot commented Dec 14, 2022

Deploy Preview for python-cpython-preview canceled.

Name Link
🔨 Latest commit a91a4d1
🔍 Latest deploy log https://app.netlify.com/sites/python-cpython-preview/deploys/639aae96df45c9000879b05a

* Only warn in the parent process.
* Add a test to test_os that a non-Python thread triggers the warning.
* Avoid the system calls to count threads if we've already warned once.
* The macOS API calls were written blind based on docs, will fix later
  if CI fails.
I don't want the array of thread info but it may insist?
Xavier-Do added a commit to odoo-dev/odoo that referenced this pull request Apr 26, 2024
Since python 3.12 python/cpython#100229, fork
inside a multithreaded process will raise a warning.

We actually have another solution to get rid of the fork that was
implemented in more recent versions.
robodoo pushed a commit to odoo/odoo that referenced this pull request Apr 26, 2024
Since python 3.12 python/cpython#100229, fork
inside a multithreaded process will raise a warning.

We actually have another solution to get rid of the fork that was
implemented in more recent versions.

closes #163562

Signed-off-by: Christophe Monniez (moc) <moc@odoo.com>
@zzzeek
Copy link

zzzeek commented Mar 6, 2025

is the use of fork() with a threaded parent going to be disallowed in a future release? That's what DeprecationWarning means. We have test code which makes use of fork to run some test cases in a separate process. There is no interaction with threads from the parent and the code is safe. We cannot use spawn or forkserver because we are running a Python function defined in the test itself and it cannot be serialized. We can silence this warning but it is not clear here, or even in the changelog given, if this functionality is going to be actually disallowed and I think it's critical to make that clear (also that the answer should be: "no" :) )

@gpshead
Copy link
Member Author

gpshead commented Mar 6, 2025

we can't disable it because people depend on it whether that's wise or not, but it is always the wrong thing to do. we're not going to create a "YouWillSufferMysteriousProblemsWarning" for such purposes. DeprecationWarning is accurate because it is something people used to do a lot but underlying platforms have significantly moved on such that things that relied on what they could get away with 15-20 years ago are no longer feasible in many normal environments today.

the deadlocks can come from the platform level and other low level libraries including the C stdlib and malloc and whatnot. it does not matter if you think you interact with the threads at all, just that they exist and are running is enough to lead to scenarios outside your control where problems happen randomly when forking.

for functions defined in the test themselves, the normal advise is to refactor the code so it is something pickle can handle (generally: not being defined on the fly)

@zzzeek
Copy link

zzzeek commented Mar 6, 2025

Where can I get your input on an issue I'm seeing where I dont actually see why this warning is being emitted? This has to do with the use of the pytest-xdist plugin which uses a tool called execnet. When using this plugin, if our own test uses fork, we get the warning. however, I've created a suite that runs through threading.enumerate() in the parent process, and in the child process - there is no additional thread spawned in either, there is only a single thread, the "main" thread.

output can be seen as follows - as you can see I'm not able to find any additional threads that are known to the Python interpreter. So....execnet is spawning threads outside of the interpreter? is that what has to be happening? is that a bug for them?

initialized: 1/1 workerINFO:main:THREAD FROM MAIN PROCESS 14974: <_MainThread(MainThread, started 140612028988288)>

1 worker [1 item]      
INFO:main:THREAD FROM CHILD PROCESS 14975 (parent: 14974): <_MainThread(MainThread, started 139972663720832)>

.
=========================================================================================== warnings summary ============================================================================================
test.py::TestWhatever::test_thing
  /usr/lib64/python3.12/multiprocessing/popen_fork.py:66: DeprecationWarning: This process (pid=14975) is multi-threaded, use of fork() may lead to deadlocks in the child.
    self.pid = os.fork()

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
===================================================================================== 1 passed, 1 warning in 10.17s =====================================================================================

I've looked at execnet a few times and so far it's opaque to me what it's doing or how pytest-xdist uses it.

@zzzeek
Copy link

zzzeek commented Mar 6, 2025

i've opened an issue at pytest-dev/pytest-xdist#1186

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.

4 participants