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.

Sign up for GitHub

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

Jump to bottom

bpo-37951: Lift subprocess's fork() restriction (GH-15544) #15544

Merged
merged 2 commits into from
Aug 27, 2019
Merged

bpo-37951: Lift subprocess's fork() restriction (GH-15544) #15544

merged 2 commits into from
Aug 27, 2019

Conversation

tiran
Copy link
Member

@tiran tiran commented Aug 27, 2019 •
edited by bedevere-bot
Loading

@tiran tiran added type-bug An unexpected behavior, bug, or error needs backport to 3.8 only security fixes labels Aug 27, 2019
@tiran tiran requested a review from gpshead as a code owner August 27, 2019 08:15
@tiran
bpo-37951: Lift subprocess's fork() restriction
c91ab6c 
Signed-off-by: Christian Heimes <christian@python.org>
@tiran tiran force-pushed the bpo37951-subprocess-subinterpreter branch from b76ff65 to c91ab6c Compare August 27, 2019 09:14
vstinner
vstinner approved these changes Aug 27, 2019
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM. Valid approach according to https://bugs.python.org/issue34651#msg325302

I just added a minor comment.

Doc/library/subprocess.rst Outdated

The *preexec_fn* parameter is no longer supported in subinterpreters.
The new restriction may affect applications that are deployed in
mod_wsgi, uWSGI, and other environments.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that it's useful to document affected applications.

Copy link
Member Author

Choose a reason for hiding this comment

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

The list of affected applications makes it easier for affected users to find the culprit. In my experience it's mostly embedded WSGI servers and embedded applications that use subinterpreters. mod_wsgi and uWSGI are the most common ones.

Copy link
Member

Choose a reason for hiding this comment

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

When you say "no longer supported", here and in whatsnew, mention that use of it raises a RuntimeError.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW I do agree that most users probably are not aware that they're in a subinterpreter, so listing some common examples of such environments is not a bad thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

@gpshead The subinterpreter and whatsnew entries now mention RuntimeError.

@tiran
Copy link
Member Author

tiran commented Aug 27, 2019

I'll wait for ACK from either @gpshead or @ericsnowcurrently before I push the fix.

gpshead
gpshead reviewed Aug 27, 2019
View reviewed changes
Modules/_posixsubprocess.c Outdated
PyErr_SetString(PyExc_RuntimeError, "fork not supported for subinterpreters");
if ((preexec_fn != Py_None) &&
(_PyInterpreterState_Get() != PyInterpreterState_Main())) {
PyErr_SetString(PyExc_RuntimeError, "preexec_fn not supported for subinterpreters");
Copy link
Member

Choose a reason for hiding this comment

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

s/for/within/

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that makes sense. ETOOLAZY

@tiran
Copy link
Member Author

tiran commented Aug 27, 2019

@gpshead Please review again

@tiran tiran requested a review from gpshead August 27, 2019 20:58
@tiran tiran changed the title bpo-37951: Lift subprocess's fork() restriction bpo-37951: Lift subprocess's fork() restriction (GH-15544) Aug 27, 2019
@tiran tiran merged commit 98d90f7 into python:master Aug 27, 2019
@tiran tiran deleted the bpo37951-subprocess-subinterpreter branch August 27, 2019 21:36
@miss-islington
Copy link
Contributor

Thanks @tiran for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@miss-islington
Copy link
Contributor

I'm having trouble backporting to 3.8. Reason: 'Error 110 while writing to socket. Connection timed out.'. Please retry by removing and re-adding the needs backport to 3.8 label.

@tiran tiran added needs backport to 3.8 only security fixes and removed needs backport to 3.8 only security fixes labels Aug 27, 2019
@miss-islington
Copy link
Contributor

Thanks @tiran for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-15554 is a backport of this pull request to the 3.8 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.8 only security fixes label Aug 27, 2019
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 27, 2019
@tiran @miss-islington
bpo-37951: Lift subprocess's fork() restriction (pythonGH-15544)
bf7c96a 
(cherry picked from commit 98d90f7)

Co-authored-by: Christian Heimes <christian@python.org>
miss-islington added a commit that referenced this pull request Aug 27, 2019
@miss-islington @tiran
bpo-37951: Lift subprocess's fork() restriction (GH-15544)
03c52f2 
(cherry picked from commit 98d90f7)

Co-authored-by: Christian Heimes <christian@python.org>
@hroncok
Copy link
Contributor

hroncok commented Aug 28, 2019

@tiran @ambv Any chance this will be in 3.8.0b4?

@ambv
Copy link
Contributor

ambv commented Aug 28, 2019

It will be in b4, yes.

lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019
@tiran @lisroach
bpo-37951: Lift subprocess's fork() restriction (pythonGH-15544)
f08f279
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
@tiran @DinoV
bpo-37951: Lift subprocess's fork() restriction (pythonGH-15544)
e891d6f
websurfer5 pushed a commit to websurfer5/cpython that referenced this pull request Jul 20, 2020
@tiran @websurfer5
bpo-37951: Lift subprocess's fork() restriction (pythonGH-15544)
632721a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gpshead gpshead gpshead approved these changes

@vstinner vstinner vstinner approved these changes

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently

Assignees
No one assigned
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@tiran @miss-islington @bedevere-bot @hroncok @ambv @gpshead @vstinner @the-knights-who-say-ni