Skip to content

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

Merged
tiran merged 2 commits into
python:masterfrom
tiran:bpo37951-subprocess-subinterpreter
Aug 27, 2019
Merged

bpo-37951: Lift subprocess's fork() restriction (GH-15544)#15544
tiran merged 2 commits into
python:masterfrom
tiran:bpo37951-subprocess-subinterpreter

Conversation

@tiran
Copy link
Copy Markdown
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 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
Copy Markdown
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.

Comment thread 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
Copy Markdown
Member

@vstinner vstinner Aug 27, 2019

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
Copy Markdown
Member Author

@tiran tiran Aug 27, 2019

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
Copy Markdown
Member

@gpshead gpshead Aug 27, 2019

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
Copy Markdown
Member

@gpshead gpshead Aug 27, 2019

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
Copy Markdown
Member Author

@tiran tiran Aug 27, 2019

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
Copy Markdown
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
Comment thread 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
Copy Markdown
Member

@gpshead gpshead Aug 27, 2019

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
Copy Markdown
Member Author

@tiran tiran Aug 27, 2019

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
Copy Markdown
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
Copy Markdown
Contributor

miss-islington commented Aug 27, 2019

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
Copy Markdown
Contributor

miss-islington commented Aug 27, 2019

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.

@miss-islington
Copy link
Copy Markdown
Contributor

miss-islington commented Aug 27, 2019

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

@bedevere-bot
Copy link
Copy Markdown

bedevere-bot commented Aug 27, 2019

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

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
Copy Markdown
Contributor

hroncok commented Aug 28, 2019

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

@ambv
Copy link
Copy Markdown
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