gh-141860: Add on_error= keyword arg to multiprocessing.set_forkserver_preload
#141859
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment was marked as outdated.
This comment was marked as outdated.
gpshead
force-pushed
the
claude/forkserver-raise-exceptions-019rrqWeqQGL2zKWava8nshj
branch
from
2a184cd to
39bc49e
Compare
picnixz
changed the title
gpshead
changed the title
gpshead
force-pushed
the
claude/forkserver-raise-exceptions-019rrqWeqQGL2zKWava8nshj
branch
from
39bc49e to
656274a
Compare
Member
Author
Used to rewrite my sandbox'ed Claude's commit as me for CLA purposes. |
gpshead
force-pushed
the
claude/forkserver-raise-exceptions-019rrqWeqQGL2zKWava8nshj
branch
2 times, most recently
from
dd6c71b to
4994d3f
Compare
Add a keyword-only `raise_exceptions` parameter (default False) to `multiprocessing.set_forkserver_preload()`. When True, ImportError exceptions during module preloading cause the forkserver to exit, breaking all use of the forkserver multiprocessing context. This allows developers to catch import errors during development rather than having them silently ignored. Implementation adds the parameter to both the ForkServer class method and the BaseContext wrapper, passing it through to the forkserver main() function which conditionally raises ImportError instead of ignoring it. Tests are in new test_multiprocessing_forkserver/test_preload.py with proper resource cleanup using try/finally. Documentation describes the behavior, consequences (forkserver exit, EOFError/ConnectionError on subsequent use), and recommends use during development. Based on original work by Nick Neumann in pythonGH-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 the existing 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>
gpshead
force-pushed
the
claude/forkserver-raise-exceptions-019rrqWeqQGL2zKWava8nshj
branch
2 times, most recently
from
6e975f3 to
73f7489
Compare
Add has_fork_support check at the package level in __init__.py to skip the entire test_multiprocessing_forkserver package on Android, iOS, WASI, and other platforms that don't support fork. This prevents import errors before individual test modules are loaded. Remove the now-redundant skip decorators from test_preload.py since the 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>
gpshead
force-pushed
the
claude/forkserver-raise-exceptions-019rrqWeqQGL2zKWava8nshj
branch
from
73f7489 to
5ce91ba
Compare
Changed from a boolean raise_exceptions parameter to a more flexible on_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 exit Also improved error messages by adding .add_note() to connection failures 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>
gpshead
changed the title
multiprocessing.set_forkserver_preload
- 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 ImportWarning is always emitted and never converted to an error, even when the test environment 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 to
enable targeted unit testing. Add comprehensive unit tests for both
module 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_combined
Total test count increased from 6 to 14 tests.
Use delete=True (default) for NamedTemporaryFile instead of manually managing cleanup with try/finally blocks. Keep file open during test and 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 instead fix the root cause: tests were allowing sys.warnoptions to leak into the forkserver subprocess via _args_from_interpreter_flags(). When CI runs with -W error, the forkserver subprocess was inheriting this flag and converting our ImportWarning calls into exceptions, causing it to crash. Solution: Save and clear sys.warnoptions in test setUp, restore in tearDown. This gives the forkserver subprocess a clean warning state where 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 uses runpy.run_path() which can raise any exception) vs only ImportError for regular __import__() calls.
Change from single quotes to double quotes when describing string values like "ignore", "warn", and "fail" in docstrings and documentation for consistency.
Previous approach of clearing sys.warnoptions broke when CI used -bb flag, because _args_from_interpreter_flags() expects certain warning options to exist based on sys.flags settings. Instead of clearing completely, filter out only the specific warning options 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 just say 'Check stderr.' instead of the more verbose message.
Clarify that _send_value is a static method specifically to be picklable as a Process target function.
gpshead
commented
Nov 23, 2025
duaneg
reviewed
Nov 25, 2025
Contributor
duaneg
left a comment
duaneg
left a comment
There was a problem hiding this comment.
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.
Integrate sys_argv parameter from main branch (pythongh-135335) with our 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 modifies sys.modules['__main__'] and appends to spawn.old_main_modules. This state persisted after tests, causing subsequent forkserver tests to try 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 and restores sys.modules['__main__'] and clears spawn.old_main_modules. Also add suppress_forkserver_stderr() context manager that injects a stderr-suppressing module via the preload mechanism itself, avoiding noisy 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 allows tests 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 ensures output 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 ensure callers explicitly specify the correct level for warning messages. Thanks to Duane Griffin for the suggestion.
gpshead
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @gpshead for commit 6ffe214 🤖 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-bot
removed
the
🔨 test-with-buildbots
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add a keyword-only
on_errorparameter tomultiprocessing.set_forkserver_preload(). This allows the user to have exceptions during optionalforkserverstart 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.This also 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 PR GH-135295 in 3.14.1 and 3.13.8, the__main__module was never actually preloaded.Based on original work by Nick Neumann @aggieNick02 in GH-99515.
📚 Documentation preview 📚: https://cpython-previews--141859.org.readthedocs.build/
Fixes #141860