gh-117378: Fix multiprocessing forkserver preload sys.path inheritance. by gpshead · Pull Request #126538 · python/cpython · GitHub

Fixes the multiprocessing "forkserver" start method forkserver process to correctly inherit the parent's sys.path during the importing of multiprocessing.set_forkserver_preload modules in the same manner as sys.path is configured when executing work items in the worker processes.

This bug could cause some forkserver module preloading to silently fail to be preloaded, leading to a performance degration in child processes due to additional repeated work. It could also have led to a side effect of "" still being in sys.path during forkserver preload imports instead of the absolute path of the directory that workers see.

A workaround for the bug that this fixes was to set PYTHONPATH in the environment before the forkserver process was started.

Issue: forkserver.set_forkserver_preload() silent preload import failures when sys.path is required #117378

…ritance. `sys.path` was not properly being sent from the parent process when launching the multiprocessing forkserver process to preload imports. This bug has been there since the forkserver start method was introduced in Python ~3.4. It was always _supposed_ to inherit `sys.path` the same way the spawn method does. Observable behavior change: A `''` value in `sys.path` will now be replaced in the forkserver's `sys.path` with an absolute pathname `os.path.abspath(os.getcwd())` saved at the time that `multiprocessing` was imported in the parent process as it already was when using the spawn start method. A workaround for the bug this fixes was to set PYTHONPATH in the environment before the forkserver process was started.

🤖 New build scheduled with the buildbot fleet by @gpshead for commit cf07467 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

🤖 New build scheduled with the buildbot fleet by @gpshead for commit cf07467 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

LGTM besides some minor suggestions.

Test for __main__ is not required if it is difficult to write it or it is too slow, but it would be better to add it.

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

Test for __main__ is not required if it is difficult to write it or it is too slow, but it would be better to add it.

reading the code and old changes... It looks like there is no possible way for main_path= to be set on the multiprocessing's forkserver.main() call since 2013's 9a76735 for #64145.

the forkserver.main call is constructed in forkserver.py getting its two possible allowed named args from spawn.get_preparation_data() which was changed to set "init_main_from_name" instead of "main_path" in the above change. Effectively making the main_path= code dead in forkserver.

I'll file a separate issue to track resolving that (remove it or recreate some missing functionality with a test).

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

…ritance. (pythonGH-126538) pythongh-117378: Fix multiprocessing forkserver preload sys.path inheritance. `sys.path` was not properly being sent from the parent process when launching the multiprocessing forkserver process to preload imports. This bug has been there since the forkserver start method was introduced in Python 3.4. It was always _supposed_ to inherit `sys.path` the same way the spawn method does. Observable behavior change: A `''` value in `sys.path` will now be replaced in the forkserver's `sys.path` with an absolute pathname `os.path.abspath(os.getcwd())` saved at the time that `multiprocessing` was imported in the parent process as it already was when using the spawn start method. **This will only be observable during forkserver preload imports**. The code invoked before calling things in another process already correctly sets `sys.path`. Which is likely why this went unnoticed for so long as a mere performance issue in some configurations. A workaround for the bug on impacted Pythons is to set PYTHONPATH in the environment before multiprocessing's forkserver process was started. Not perfect as that is then inherited by other children, etc, but likely good enough for many people's purposes. (cherry picked from commit 9d08423) Co-authored-by: Gregory P. Smith <greg@krypto.org> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

Sorry, @gpshead, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 9d08423b6e0fa89ce9cfea08e580ed72e5db8c70 3.12

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

…eritance. (GH-126538) (GH-126632) gh-117378: Fix multiprocessing forkserver preload sys.path inheritance. (GH-126538) `sys.path` was not properly being sent from the parent process when launching the multiprocessing forkserver process to preload imports. This bug has been there since the forkserver start method was introduced in Python 3.4. It was always _supposed_ to inherit `sys.path` the same way the spawn method does. Observable behavior change: A `''` value in `sys.path` will now be replaced in the forkserver's `sys.path` with an absolute pathname `os.path.abspath(os.getcwd())` saved at the time that `multiprocessing` was imported in the parent process as it already was when using the spawn start method. **This will only be observable during forkserver preload imports**. The code invoked before calling things in another process already correctly sets `sys.path`. Which is likely why this went unnoticed for so long as a mere performance issue in some configurations. A workaround for the bug on impacted Pythons is to set PYTHONPATH in the environment before multiprocessing's forkserver process was started. Not perfect as that is then inherited by other children, etc, but likely good enough for many people's purposes. (cherry picked from commit 9d08423) Co-authored-by: Gregory P. Smith <greg@krypto.org> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

…th inheritance. (pythonGH-126538) pythongh-117378: Fix multiprocessing forkserver preload sys.path inheritance. `sys.path` was not properly being sent from the parent process when launching the multiprocessing forkserver process to preload imports. This bug has been there since the forkserver start method was introduced in Python 3.4. It was always _supposed_ to inherit `sys.path` the same way the spawn method does. Observable behavior change: A `''` value in `sys.path` will now be replaced in the forkserver's `sys.path` with an absolute pathname `os.path.abspath(os.getcwd())` saved at the time that `multiprocessing` was imported in the parent process as it already was when using the spawn start method. **This will only be observable during forkserver preload imports**. The code invoked before calling things in another process already correctly sets `sys.path`. Which is likely why this went unnoticed for so long as a mere performance issue in some configurations. A workaround for the bug on impacted Pythons is to set PYTHONPATH in the environment before multiprocessing's forkserver process was started. Not perfect as that is then inherited by other children, etc, but likely good enough for many people's purposes. (cherry picked from commit 9d08423) Co-authored-by: Gregory P. Smith <greg@krypto.org> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

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

…eritance. (GH-126538) (GH-126633) gh-117378: Fix multiprocessing forkserver preload sys.path inheritance. `sys.path` was not properly being sent from the parent process when launching the multiprocessing forkserver process to preload imports. This bug has been there since the forkserver start method was introduced in Python 3.4. It was always _supposed_ to inherit `sys.path` the same way the spawn method does. Observable behavior change: A `''` value in `sys.path` will now be replaced in the forkserver's `sys.path` with an absolute pathname `os.path.abspath(os.getcwd())` saved at the time that `multiprocessing` was imported in the parent process as it already was when using the spawn start method. **This will only be observable during forkserver preload imports**. The code invoked before calling things in another process already correctly sets `sys.path`. Which is likely why this went unnoticed for so long as a mere performance issue in some configurations. A workaround for the bug on impacted Pythons is to set PYTHONPATH in the environment before multiprocessing's forkserver process was started. Not perfect as that is then inherited by other children, etc, but likely good enough for many people's purposes. (cherry picked from commit 9d08423) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

…ritance. (pythonGH-126538) pythongh-117378: Fix multiprocessing forkserver preload sys.path inheritance. `sys.path` was not properly being sent from the parent process when launching the multiprocessing forkserver process to preload imports. This bug has been there since the forkserver start method was introduced in Python 3.4. It was always _supposed_ to inherit `sys.path` the same way the spawn method does. Observable behavior change: A `''` value in `sys.path` will now be replaced in the forkserver's `sys.path` with an absolute pathname `os.path.abspath(os.getcwd())` saved at the time that `multiprocessing` was imported in the parent process as it already was when using the spawn start method. **This will only be observable during forkserver preload imports**. The code invoked before calling things in another process already correctly sets `sys.path`. Which is likely why this went unnoticed for so long as a mere performance issue in some configurations. A workaround for the bug on impacted Pythons is to set PYTHONPATH in the environment before multiprocessing's forkserver process was started. Not perfect as that is then inherited by other children, etc, but likely good enough for many people's purposes. Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

gpshead added 2 commits November 7, 2024 00:56

NEWS entry

702e0b7

gpshead added type-bug An unexpected behavior, bug, or error stdlib Standard Library Python modules in the Lib/ directory topic-multiprocessing needs backport to 3.12 only security fixes needs backport to 3.13 only security fixes labels Nov 7, 2024

gpshead self-assigned this Nov 7, 2024

bedevere-app Bot added the awaiting core review label Nov 7, 2024

bedevere-app Bot mentioned this pull request Nov 7, 2024

forkserver.set_forkserver_preload() silent preload import failures when sys.path is required #117378

Closed

gpshead requested a review from serhiy-storchaka November 7, 2024 09:47

serhiy-storchaka reviewed Nov 7, 2024

View reviewed changes

Comment thread Lib/multiprocessing/forkserver.py Outdated

gpshead commented Nov 7, 2024

View reviewed changes

Comment thread Lib/test/test_multiprocessing_forkserver/test_forkserver_main.py Outdated

gpshead added 2 commits November 7, 2024 22:31

move sys.path setting up per review.

7ad114f

Redo the test as an sys.path integration test.

cf07467

gpshead added 🔨 test-with-buildbots Test PR w/ buildbots; report in status section 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section labels Nov 9, 2024

bedevere-bot removed 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section 🔨 test-with-buildbots Test PR w/ buildbots; report in status section labels Nov 9, 2024

serhiy-storchaka approved these changes Nov 9, 2024

View reviewed changes

Comment thread Lib/test/_test_multiprocessing.py

Comment thread Lib/test/_test_multiprocessing.py

Comment thread Lib/test/_test_multiprocessing.py Outdated

Comment thread Lib/test/_test_multiprocessing.py

bedevere-app Bot added awaiting merge and removed awaiting core review labels Nov 9, 2024

avoid := for readability

4fe3389

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>

reword NEWS

3f486f8

gpshead enabled auto-merge (squash) November 9, 2024 22:22

gpshead mentioned this pull request Nov 9, 2024

multiprocessing forkserver main contains dead main_path= handling code #126631

Closed

news sphinx-lint fix

2444519

gpshead merged commit 9d08423 into python:main Nov 9, 2024

bedevere-app Bot removed the awaiting merge label Nov 9, 2024

bedevere-app Bot removed the needs backport to 3.13 only security fixes label Nov 9, 2024

gpshead deleted the bug/multiprocessing-forkserver-preload-sys-path branch November 9, 2024 23:48

bedevere-app Bot removed the needs backport to 3.12 only security fixes label Nov 9, 2024

gh-117378: Fix multiprocessing forkserver preload sys.path inheritance. by gpshead · Pull Request #126538 · python/cpython · GitHub

Repository navigation

gh-117378: Fix multiprocessing forkserver preload sys.path inheritance. - #126538

gh-117378: Fix multiprocessing forkserver preload sys.path inheritance.#126538
gpshead merged 7 commits into
python:mainfrom
gpshead:bug/multiprocessing-forkserver-preload-sys-path

gpshead commented Nov 7, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Nov 9, 2024

Uh oh!

bedevere-bot commented Nov 9, 2024

Uh oh!

serhiy-storchaka left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gpshead commented Nov 9, 2024

Uh oh!

miss-islington-app Bot commented Nov 9, 2024

Uh oh!

miss-islington-app Bot commented Nov 9, 2024

Uh oh!

bedevere-app Bot commented Nov 9, 2024

Uh oh!

bedevere-app Bot commented Nov 9, 2024

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gh-117378: Fix multiprocessing forkserver preload sys.path inheritance. by gpshead · Pull Request #126538 · python/cpython · GitHub

Repository navigation

gh-117378: Fix multiprocessing forkserver preload sys.path inheritance. - #126538

gh-117378: Fix multiprocessing forkserver preload sys.path inheritance.#126538gpshead merged 7 commits intopython:mainpython/cpython:mainfrom gpshead:bug/multiprocessing-forkserver-preload-sys-pathgpshead/cpython:bug/multiprocessing-forkserver-preload-sys-pathCopy head branch name to clipboard

Conversation

gpshead commented Nov 7, 2024 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Nov 9, 2024

Uh oh!

bedevere-bot commented Nov 9, 2024

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gpshead commented Nov 9, 2024

Uh oh!

miss-islington-app Bot commented Nov 9, 2024

Uh oh!

miss-islington-app Bot commented Nov 9, 2024

Uh oh!

bedevere-app Bot commented Nov 9, 2024

Uh oh!

bedevere-app Bot commented Nov 9, 2024

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gh-117378: Fix multiprocessing forkserver preload sys.path inheritance.#126538
gpshead merged 7 commits into
python:mainfrom
gpshead:bug/multiprocessing-forkserver-preload-sys-path

gpshead commented Nov 7, 2024 •
edited by bedevere-app Bot
Loading