Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-141860: Add on_error= keyword arg tomultiprocessing.set_forkserver_preload#141859

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@gpshead
Copy link
Member

@gpsheadgpshead commentedNov 22, 2025
edited
Loading

Add a keyword-onlyon_error parameter tomultiprocessing.set_forkserver_preload(). This allows the user to have exceptions during optionalforkserver start method module preloading cause the forkserver subprocess to warn (generally to stderr) or exit with an error (preventing use of the forkserver) instead of being silently ignored.

Thisalso fixes an oversight, errors when preloading a__main__ module are now treated the similarly. Those would always raise unlike other modules in preload, but that had gone unnoticed as up until bug fix PRGH-135295 in 3.14.1 and 3.13.8, the__main__ module was never actually preloaded.

Based on original work by Nick Neumann@aggieNick02 inGH-99515.


📚 Documentation preview 📚:https://cpython-previews--141859.org.readthedocs.build/

Fixes#141860

@python-cla-bot

This comment was marked as outdated.

@gpsheadgpsheadforce-pushed theclaude/forkserver-raise-exceptions-019rrqWeqQGL2zKWava8nshj branch from2a184cd to39bc49eCompareNovember 22, 2025 22:21
@picnixzpicnixz changed the titleAdd raise_exceptions parameter to multiprocessing.set_forkserver_preloadgh-98552: Add raise_exceptions parameter to multiprocessing.set_forkserver_preloadNov 22, 2025
@gpsheadgpshead changed the titlegh-98552: Add raise_exceptions parameter to multiprocessing.set_forkserver_preloadgh-141860: Add raise_exceptions parameter to multiprocessing.set_forkserver_preloadNov 22, 2025
@gpsheadgpsheadforce-pushed theclaude/forkserver-raise-exceptions-019rrqWeqQGL2zKWava8nshj branch from39bc49e to656274aCompareNovember 22, 2025 22:44
@gpshead
Copy link
MemberAuthor

git commit --amend --reset-author --no-editgit push --force-with-lease

Used to rewrite my sandbox'ed Claude's commit as me for CLA purposes.

@gpsheadgpshead marked this pull request as ready for reviewNovember 22, 2025 22:47
@gpsheadgpsheadforce-pushed theclaude/forkserver-raise-exceptions-019rrqWeqQGL2zKWava8nshj branch 2 times, most recently fromdd6c71b to4994d3fCompareNovember 23, 2025 01:49
gpsheadand others added2 commitsNovember 23, 2025 01:51
Add a keyword-only `raise_exceptions` parameter (default False) to`multiprocessing.set_forkserver_preload()`. When True, ImportErrorexceptions during module preloading cause the forkserver to exit,breaking all use of the forkserver multiprocessing context. Thisallows developers to catch import errors during development ratherthan having them silently ignored.Implementation adds the parameter to both the ForkServer class methodand the BaseContext wrapper, passing it through to the forkservermain() function which conditionally raises ImportError instead ofignoring it.Tests are in new test_multiprocessing_forkserver/test_preload.py withproper resource cleanup using try/finally.Documentation describes the behavior, consequences (forkserver exit,EOFError/ConnectionError on subsequent use), and recommends use duringdevelopment.Based on original work by Nick Neumann inpythonGH-99515.Contributed by Nick Neumann.Co-authored-by: aggieNick02 <nick@pcpartpicker.com>Co-authored-by: Claude (Sonnet 4.5) <noreply@anthropic.com>Co-authored-by: Gregory P. Smith <greg@krypto.org>
Add skip decorators to exclude test_preload.py on Android, iOS,WASI, and other platforms that don't support fork, using theexisting has_fork_support check from test.support.Co-authored-by: Claude (Sonnet 4.5) <noreply@anthropic.com>Co-authored-by: Gregory P. Smith <greg@krypto.org>
@gpsheadgpsheadforce-pushed theclaude/forkserver-raise-exceptions-019rrqWeqQGL2zKWava8nshj branch 2 times, most recently from6e975f3 to73f7489CompareNovember 23, 2025 04:54
Add has_fork_support check at the package level in __init__.py toskip the entire test_multiprocessing_forkserver package on Android,iOS, WASI, and other platforms that don't support fork. This preventsimport errors before individual test modules are loaded.Remove the now-redundant skip decorators from test_preload.py sincethe package-level skip makes them unnecessary.Co-authored-by: Claude (Sonnet 4.5) <noreply@anthropic.com>Co-authored-by: Gregory P. Smith <greg@krypto.org>
@gpsheadgpsheadforce-pushed theclaude/forkserver-raise-exceptions-019rrqWeqQGL2zKWava8nshj branch from73f7489 to5ce91baCompareNovember 23, 2025 06:57
@gpsheadgpshead marked this pull request as draftNovember 23, 2025 07:05
Changed from a boolean raise_exceptions parameter to a more flexibleon_error parameter that accepts 'ignore', 'warn', or 'fail'.- 'ignore' (default): silently ignores import failures- 'warn': emits ImportWarning from forkserver subprocess- 'fail': raises exception, causing forkserver to exitAlso improved error messages by adding .add_note() to connectionfailures when on_error='fail' to guide users to check stderr.Updated both spawn.import_main_path() and module __import__()failure paths to support all three modes using match/case syntax.Co-authored-by: aggieNick02 <nick@pcpartpicker.com>Co-authored-by: Claude (Sonnet 4.5) <noreply@anthropic.com>Co-authored-by: Gregory P. Smith <greg@krypto.org>
@gpsheadgpshead changed the titlegh-141860: Add raise_exceptions parameter to multiprocessing.set_forkserver_preloadgh-141860: Add on_error= keyword arg tomultiprocessing.set_forkserver_preloadNov 23, 2025
- Remove unused warnings import- Use getattr() to safely check for __notes__ attribute- Add assertion for notes existence before checking content
Use warnings.catch_warnings() context manager to ensure ImportWarningis always emitted and never converted to an error, even when the testenvironment has warnings configured with -W error.
For consistency with module preload warnings, use 'Failed to preload__main__' instead of 'Failed to import __main__'.
Extract preload logic into a separate _handle_preload() function toenable targeted unit testing. Add comprehensive unit tests for bothmodule and __main__ preload with all three on_error modes.New tests:- test_handle_preload_main_on_error_{fail,warn,ignore}- test_handle_preload_module_on_error_{fail,warn,ignore}- test_handle_preload_main_valid- test_handle_preload_combinedTotal test count increased from 6 to 14 tests.
Use delete=True (default) for NamedTemporaryFile instead of manuallymanaging cleanup with try/finally blocks. Keep file open during testand let context manager handle automatic deletion. Also remove now-unused os import.
- Remove comments that restate what the code obviously does- Change from 'from multiprocessing.forkserver import _handle_preload'  to 'from multiprocessing import forkserver' and use qualified calls- Makes code cleaner and follows better import conventions
Remove catch_warnings() workaround from production code and insteadfix the root cause: tests were allowing sys.warnoptions to leak intothe forkserver subprocess via _args_from_interpreter_flags().When CI runs with -W error, the forkserver subprocess was inheritingthis flag and converting our ImportWarning calls into exceptions,causing it to crash.Solution: Save and clear sys.warnoptions in test setUp, restore intearDown. This gives the forkserver subprocess a clean warning statewhere warnings.warn() works as intended.Also remove unnecessary set_forkserver_preload([]) call from tearDown.
Explain why we catch broad Exception for import_main_path() (it usesrunpy.run_path() which can raise any exception) vs only ImportErrorfor regular __import__() calls.
Change from single quotes to double quotes when describing stringvalues like "ignore", "warn", and "fail" in docstrings anddocumentation for consistency.
Previous approach of clearing sys.warnoptions broke when CI used -bbflag, because _args_from_interpreter_flags() expects certain warningoptions to exist based on sys.flags settings.Instead of clearing completely, filter out only the specific warningoptions that would convert ImportWarning to errors:- 'error' (converts all warnings)- 'error::ImportWarning' (converts ImportWarning specifically)This preserves options like 'error::BytesWarning' that subprocess's_args_from_interpreter_flags() needs to remove, preventing ValueError.
Remove unnecessary comment and simplify the exception note to justsay 'Check stderr.' instead of the more verbose message.
Clarify that _send_value is a static method specifically to bepicklable as a Process target function.
@gpsheadgpshead marked this pull request as ready for reviewNovember 23, 2025 23:33
Copy link
Contributor

@duanegduaneg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think the approach is good, and doesn't need any changes aside from the test fixes already discussed.

I tried playing around with using an error handler callback instead of/in addition to a string to specify the strategy, but it gets quite messy. There are a couple of places where we want to check what type of error handling the user specified, and that is awkward when it is an arbitrary function.

There don't appear to be any tests for running from package entry points (i.e. with-m <main module>), but they might be a bit awkward to add and given how they work their code paths are possibly already effectively covered. Some quick manual testing seems to show everything functioning correctly.

My other refactoring suggestions are marginal and unimportant, feel free to ignore them.

gpshead reacted with thumbs up emoji
gpsheadand others added4 commitsJanuary 14, 2026 05:53
Integrate sys_argv parameter from main branch (pythongh-135335) withour on_error parameter for set_forkserver_preload.Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
TestHandlePreload tests call spawn.import_main_path() which modifiessys.modules['__main__'] and appends to spawn.old_main_modules. Thisstate persisted after tests, causing subsequent forkserver tests totry loading __main__ from a deleted temp file. With on_error='ignore',the forkserver stayed broken causing process spawn failures and hangs.Fix by adding setUp/tearDown to TestHandlePreload that saves andrestores sys.modules['__main__'] and clears spawn.old_main_modules.Also add suppress_forkserver_stderr() context manager that injectsa stderr-suppressing module via the preload mechanism itself, avoidingnoisy output during tests that expect import failures.Thanks to Duane Griffin for identifying the root cause of the hang.
Replace suppress_forkserver_stderr() with capture_forkserver_stderr()that writes stderr to a temp file instead of /dev/null. This allowstests to assert on the actual warning/error messages.The capture module also enables ImportWarning via filterwarnings()since it's ignored by default in Python. Line buffering ensuresoutput is flushed, and forkserver.main() calls _flush_std_streams()after preloading which guarantees content is written before we read.Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Extract duplicated match on_error logic into a helper function.The warn_stacklevel parameter is required (no default) to ensurecallers explicitly specify the correct level for warning messages.Thanks to Duane Griffin for the suggestion.
@gpsheadgpshead added the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelJan 14, 2026
@bedevere-bot
Copy link

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

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F141859%2Fmerge

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

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelJan 14, 2026
@gpsheadgpshead merged commitc879b2a intopython:mainJan 18, 2026
117 of 119 checks passed
@gpsheadgpshead self-assigned thisJan 18, 2026
@gpsheadgpshead added type-featureA feature request or enhancement 3.15new features, bugs and security fixes labelsJan 18, 2026
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

1 more reviewer

@duanegduanegduaneg left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@gpsheadgpshead

Labels

3.15new features, bugs and security fixestype-featureA feature request or enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Optionally allow exceptions from multiprocessing forkserver preload imports to be raised

3 participants

@gpshead@bedevere-bot@duaneg

[8]ページ先頭

©2009-2026 Movatter.jp