GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-91401: Add a failsafe way to disable vfork. by gpshead · Pull Request #91490 · python/cpython

Conversation

gpshead commented Apr 13, 2022

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          pythongh-91401: Add a failsafe way to disable vfork.

aba06a0

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 added type-bug

An unexpected behavior, bug, or error

type-feature A feature request or enhancement labels

Apr 13, 2022

bedevere-bot added the awaiting core review label

Apr 13, 2022

gpshead self-assigned this

Apr 13, 2022

gpshead marked this pull request as ready for review

April 14, 2022 22:27

gpshead requested a review from vstinner

April 14, 2022 22:27

vstinner reviewed

Apr 19, 2022

View reviewed changes

vstinner left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gpshead reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

Doc/library/subprocess.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

vstinner commented Apr 19, 2022

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gpshead commented Apr 20, 2022

Copy link

Copy Markdown

Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner commented Apr 20, 2022

Copy link

Copy Markdown

Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gpshead added 2 commits

April 25, 2022 21:17


          Switch the attr name to _USE_VFORK for consistency

c99fb75

No double negatives. Also documents _USE_POSIX_SPAWN.


          Add a comment, default to True.

b794394

Rather than re-encoding our platform selection here (configure.ac
and runtime logic in _posixsubprocess already does that).

gpshead added 3.10 (EOL)

end of life

needs backport to 3.10 and removed 3.10 (EOL) end of life labels

Apr 25, 2022


          NEWS entry.

9154ad8

vstinner approved these changes

Apr 25, 2022

View reviewed changes

vstinner left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Doc/library/subprocess.rst

		code.

		.. versionadded:: 3.8 ``_USE_POSIX_SPAWN``
		.. versionadded:: 3.11 ``_USE_VFORK``

vstinner Apr 25, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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 :-)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-bot added awaiting merge and removed awaiting core review labels

Apr 25, 2022


          Add a unittest for _USE_VFORK.

gpshead removed the needs backport to 3.10 label

Apr 25, 2022

gpshead merged commit cd5726f into python:main

Apr 25, 2022

bedevere-bot removed the awaiting merge label

Apr 25, 2022

gpshead deleted the vfork-monsters branch

April 25, 2022 23:19

JelleZijlstra added a commit to python/typeshed that referenced this pull request

Apr 26, 2022


          subprocess: Add _USE_VFORK and _USE_POSIX_SPAWN

f527db7

Debatable whether to include these, but they are now documented: python/cpython#91490

JelleZijlstra mentioned this pull request

Apr 26, 2022

subprocess: Add _USE_VFORK and _USE_POSIX_SPAWN python/typeshed#7687

Merged

AlexWaygood pushed a commit to python/typeshed that referenced this pull request

Apr 26, 2022


          subprocess: Add _USE_VFORK and _USE_POSIX_SPAWN (#7687)

08ae9f3

These are now documented: python/cpython#91490

slateny mentioned this pull request

Nov 23, 2022

[Minor PR] Quotes in documentation changed into code blocks #99536

Merged

hugovk mentioned this pull request

Jun 21, 2023

Missing reference to issue in subprocess documentation #105975

Closed

mattip mentioned this pull request

Oct 7, 2024

signature of _posixsubprocess._fork_exec changed pypy/pypy#5069

Closed

Reviewers

vstinner

vstinner approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

gpshead

Labels

type-bug

An unexpected behavior, bug, or error

type-feature

A feature request or enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

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.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.