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-91401: Add a failsafe way to disable vfork. #91490

Merged
merged 5 commits into from Apr 25, 2022

Conversation

gpshead
Copy link
Member

@gpshead gpshead commented Apr 13, 2022

Just in case there is ever an issue with _posixsubprocess's use of
vfork() due to the complexity of using it properly and potential
directions that Linux platforms where it defaults to on could take, this
adds a failsafe so that users can disable its use entirely by setting
a global flag.

No known reason to disable it exists. But it'd be a shame to encounter
one and not be able to use CPython without patching and rebuilding it.

See the linked issue for some discussion on reasoning.

Just in case there is ever an issue with _posixsubprocess's use of
vfork() due to the complexity of using it properly and potential
directions that Linux platforms where it defaults to on could take, this
adds a failsafe so that users can disable its use entirely by setting
a global flag.

No known reason to disable it exists. But it'd be a shame to encounter
one and not be able to use CPython without patching and rebuilding it.

See the linked issue for some discussion on reasoning.
@gpshead gpshead added type-bug An unexpected behavior, bug, or error type-feature A feature request or enhancement labels Apr 13, 2022
@gpshead gpshead self-assigned this Apr 13, 2022
@gpshead gpshead marked this pull request as ready for review Apr 14, 2022
@gpshead gpshead requested a review from vstinner Apr 14, 2022
Copy link
Member

@vstinner vstinner left a comment

Why not a simple boolean flag?

Can subprocess disable vfork using disable_vfork_reason if vfork is not available? You can expose a simple private flag in _posixsubprocess saying if vfork is available. Something similar to _testcapi.WITH_PYMALLOC or _thread.TIMEOUT_MAX.

Doc/library/subprocess.rst Outdated Show resolved Hide resolved
@vstinner
Copy link
Member

vstinner commented Apr 19, 2022

I added subprocess._USE_POSIX_SPAWN so people can opt-out if they encounter problems with posix_spawn().

@gpshead
Copy link
Member Author

gpshead commented Apr 20, 2022

I added subprocess._USE_POSIX_SPAWN so people can opt-out if they encounter problems with posix_spawn().

Being consistent with that seems worthwhile, I'll rework this PR. I expect I'm overthinking it with the whole "make it a string" thing and pleading for people to include an explanation of why.

@vstinner
Copy link
Member

vstinner commented Apr 20, 2022

I chose to add a private attribute since the default value must be safe for all users. But for the corner cases, IMO a private attribute is enough.

For vfork, well, it's up to you to add a private or public attribute.

gpshead added 2 commits Apr 25, 2022
No double negatives. Also documents _USE_POSIX_SPAWN.
Rather than re-encoding our platform selection here (configure.ac
and runtime logic in _posixsubprocess already does that).
Copy link
Member

@vstinner vstinner left a comment

LGTM. I just have a doubt about the doc format, but you will see when it will be rendered on docs.python.org.

code.

.. versionadded:: 3.8 ``_USE_POSIX_SPAWN``
.. versionadded:: 3.11 ``_USE_VFORK``
Copy link
Member

@vstinner vstinner Apr 25, 2022

Choose a reason for hiding this comment

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

I never tried this syntax, I don't know if it's properly rendered. You will see :-)

@gpshead gpshead merged commit cd5726f into python:main Apr 25, 2022
13 checks passed
@gpshead gpshead deleted the vfork-monsters branch Apr 25, 2022
JelleZijlstra added a commit to python/typeshed that referenced this pull request Apr 26, 2022
Debatable whether to include these, but they are now documented: python/cpython#91490
AlexWaygood pushed a commit to python/typeshed that referenced this pull request Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants