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

raise a Warning when os.fork() is called and the process has multiple threads #100228

Closed
gpshead opened this issue Dec 14, 2022 · 5 comments
Closed
Labels
3.12 bugs and security fixes type-feature A feature request or enhancement

Comments

@gpshead
Copy link
Member

gpshead commented Dec 14, 2022

os.fork() in a multi-threaded application is a likely source of deadlocks on many platforms. We should raise a warning when people call os.fork() from a process that we know has other threads running.

This came from discussion in https://discuss.python.org/t/switching-default-multiprocessing-context-to-spawn-on-posix-as-well/21868/, though I believe many of us have pondered doing it in the past.

Linked PRs

@gpshead gpshead added the type-feature A feature request or enhancement label Dec 14, 2022
gpshead added a commit that referenced this issue Dec 29, 2022
Not comprehensive, best effort warning. There are cases when threads exist on some platforms that this code cannot detect. macOS when API permissions allow and Linux with a readable /proc procfs present are the currently supported cases where a warning should show up reliably.

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.
@hauntsaninja
Copy link
Contributor

Thanks for adding this!

@pitrou
Copy link
Member

pitrou commented Feb 11, 2023

Why a DeprecationWarning? This suggests that something is deprecated while it is not... ResourceWarning would sound more appropriate IMHO.

@gpshead
Copy link
Member Author

gpshead commented Feb 12, 2023

Yeah, that question came up on the PR: https://github.com/python/cpython/pull/100229/files#r1048391023

This is one of those cases where we worry about being too noisy if we use a warning that people see outside of their test environment (where deprecation warnings tend to be turned on, they're off by default otherwise).

Lots of code in the world for better or worse "works fine" calling fork() from a threaded application because the software stacks that so many things build on top of don't care: glibc and it's malloc being the prime example. POSIX says it is never okay to do it, but because it worked on old glibc's and glibc malloc: that implementation gets maintained in such a super conservative manner that it does not as a general rule cause problems with threads+fork itself. Other things do, other libc implementations, many other malloc implementations (which often for much better performance specifically in the face of threading), other libraries code may use, etc.

So we want it to be a soft nudge to start with. We can "increase" the verbosity to a different warning type in the future. Or if this proves too disruptive as is, reconsider. Unfortunately we don't have an existing Warning class for this kind of situation.

A perhaps more appropriately named subclass (so that it is hidden by default) of DeprecationWarning could be created so that it at least doesn't sound like we're removing the ability for people to try and shoot themselves in the foot in the future?

I'm expecting we'll hear from users about the noise level by the time we're in the beta phase.

@gpshead
Copy link
Member Author

gpshead commented Sep 12, 2023

Relevant discussion about this warning in 3.12 release candidate end user testing: https://discuss.python.org/t/concerns-regarding-deprecation-of-fork-with-alive-threads/33555

gpshead added a commit that referenced this issue Sep 23, 2023
Document the `os.fork` posix threads detected `DeprecationWarning` in 3.12 What's New, os, multiprocessing, and concurrent.futures docs.

Many reviews and doc cleanup edits by Adam & Hugo. 🥳 

Co-authored-by: Adam Turner <[email protected]>
Co-authored-by: Hugo van Kemenade <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Sep 23, 2023
…thonGH-109767)

Document the `os.fork` posix threads detected `DeprecationWarning` in 3.12 What's New, os, multiprocessing, and concurrent.futures docs.

Many reviews and doc cleanup edits by Adam & Hugo. 🥳

(cherry picked from commit 5e7ea95)

Co-authored-by: Gregory P. Smith <[email protected]>
Co-authored-by: Adam Turner <[email protected]>
Co-authored-by: Hugo van Kemenade <[email protected]>
Yhg1s pushed a commit that referenced this issue Sep 24, 2023
…H-109767) (#109773)

* gh-100228: Document the os.fork threads DeprecationWarning. (GH-109767)

Document the `os.fork` posix threads detected `DeprecationWarning` in 3.12 What's New, os, multiprocessing, and concurrent.futures docs.

Many reviews and doc cleanup edits by Adam & Hugo. 🥳

(cherry picked from commit 5e7ea95)

Co-authored-by: Gregory P. Smith <[email protected]>
Co-authored-by: Adam Turner <[email protected]>
Co-authored-by: Hugo van Kemenade <[email protected]>

* link to the discussion thread from whatsnew

Include the link to the discussion in the what's new text per @malemberg's comment on. #109767

(i'll follow up with a PR to main to include this edit there as well)

---------

Co-authored-by: Gregory P. Smith <[email protected]>
Co-authored-by: Adam Turner <[email protected]>
Co-authored-by: Hugo van Kemenade <[email protected]>
csm10495 pushed a commit to csm10495/cpython that referenced this issue Sep 28, 2023
…thon#109767)

Document the `os.fork` posix threads detected `DeprecationWarning` in 3.12 What's New, os, multiprocessing, and concurrent.futures docs.

Many reviews and doc cleanup edits by Adam & Hugo. 🥳 

Co-authored-by: Adam Turner <[email protected]>
Co-authored-by: Hugo van Kemenade <[email protected]>
Glyphack pushed a commit to Glyphack/cpython that referenced this issue Sep 2, 2024
…thon#109767)

Document the `os.fork` posix threads detected `DeprecationWarning` in 3.12 What's New, os, multiprocessing, and concurrent.futures docs.

Many reviews and doc cleanup edits by Adam & Hugo. 🥳 

Co-authored-by: Adam Turner <[email protected]>
Co-authored-by: Hugo van Kemenade <[email protected]>
gpauloski added a commit to proxystore/taps that referenced this issue Sep 5, 2024
This raises a deprecation warning on POSIX where the default is fork()
which will change in 3.14. It is also unsafe to fork the test process
because it is multithreaded.

See python/cpython#100228 and
https://docs.python.org/3/library/multiprocessing.html#contexts-and-start-methods
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.12 bugs and security fixes type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

4 participants
@gpshead @pitrou @hauntsaninja and others