bpo-35537: subprocess can use posix_spawn with pipes by vstinner · Pull Request #11575 · python/cpython · GitHub

subprocess.Popen can now also use os.posix_spawn() with pipes if
pipe file descriptors are greater than 2.
Fix Popen._posix_spawn(): set _child_created to True.
Add Popen._close_pipe_fds() helper function to factorize the code.

https://bugs.python.org/issue35537

Follow-up of PR #11452.

cc @izbyshev, @pablogsal, @nanjekyejoannah

PR rebased on top of PR #11579 (commit 0785889).

Technically, there is no need to explicitly close parent pipe ends because they have O_CLOEXEC. One small advantage of doing that is to reduce the number of descriptors before the following dup2, which could increase their number (in a corner case when any of descriptors 0, 1, 2 is closed in the parent process) and potentially hit resource limits.

Technically, there is no need, but _posixsubprocess does that. In case of doubt, I prefer to mimick _posixsubprocess behavior.

    /* Close parent's pipe ends. */
    if (p2cwrite != -1)
        POSIX_CALL(close(p2cwrite));
    if (c2pread != -1)
        POSIX_CALL(close(c2pread));
    if (errread != -1)
        POSIX_CALL(close(errread));
    POSIX_CALL(close(errpipe_read));

If we decide to skip the close(), I would prefer to also modify _posixsubprocess. But here I only want to use posix_spawn() in more cases, so I prefer to not modify "unrelated" changes :-)

* subprocess.Popen can now also use os.posix_spawn() with pipes if pipe file descriptors are greater than 2. * Fix Popen._posix_spawn(): set _child_created to True. * Add Popen._close_pipe_fds() helper function to factorize the code.

PR rebased on top of PR #11579 (commit 0785889).

Since PR #11579, I rebased this PR again :-)

@gpshead @giampaolo: would you mind to review this change? It allows to use posix_spawn() in subprocess in much more cases.

If I don't get any feedback before the end of the week, I will just merge my PR. I prefer to merge it early during the 3.8 development cycle, to get more time to fix it if something goes wrong :)

In case of premature failure on X.Close() or os.close(X) the remaining pipes/fds will remain "open". Perhaps it makes sense to use contextlib.ExitStack.

Is it a regression caused by my PR? Or a general remark on existing code? My PR just moves code, no?

Yep, it doesn't look like a regression introduced by you (was already there).

At least, I factorized the code so it should help to enhance the code ;-) Maybe open an issue if you want to work on that? I don't see an obvious pattern to ensure that all file descriptors are properly closed.

Yeah, makes sense. I bet there are many other places which can benefit from ExitStack in a similar manner.

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@giampaolo: So what do you think of the overall change? :-)

At least, I factorized the code so it should help to enhance the code ;-) Maybe open an issue if you want to work on that? I don't see an obvious pattern to ensure that all file descriptors are properly closed.

LGTM

@giampaolo: "LGTM"

Yay! Thanks for the approval! I merged my PR.

@giampaolo: As I wrote, please open a new issue if you would like to enhance the new _close_pipe_fds() helper method.

@vstinner here it is: #11686

Close pipes/fds in subprocess by using ExitStack. "In case of premature failure on X.Close() or os.close(X) the remaining pipes/fds will remain "open". Perhaps it makes sense to use contextlib.ExitStack." - Rationale: #11575 (comment)

vstinner requested a review from gpshead as a code owner January 16, 2019 00:20

the-knights-who-say-ni added the CLA signed label Jan 16, 2019

bedevere-bot added the awaiting merge label Jan 16, 2019

vstinner requested a review from giampaolo January 16, 2019 00:20

vstinner added the skip news label Jan 16, 2019

vstinner mentioned this pull request Jan 16, 2019

bpo-35537: subprocess uses os.posix_spawn in some cases #11452

Merged

pablogsal self-requested a review January 16, 2019 14:34

izbyshev reviewed Jan 16, 2019

View reviewed changes

Comment thread Lib/subprocess.py Outdated

bpo-35537 : subprocess can use posix_spawn with pipes

9bc8adf

* subprocess.Popen can now also use os.posix_spawn() with pipes if pipe file descriptors are greater than 2. * Fix Popen._posix_spawn(): set _child_created to True. * Add Popen._close_pipe_fds() helper function to factorize the code.

giampaolo requested changes Jan 23, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Jan 23, 2019

vstinner commented Jan 23, 2019

View reviewed changes

giampaolo approved these changes Jan 23, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Jan 23, 2019

vstinner merged commit f6243ac into python:master Jan 23, 2019

bedevere-bot removed the awaiting merge label Jan 23, 2019

vstinner deleted the subprocess_spawn2 branch January 23, 2019 18:00

giampaolo mentioned this pull request Jan 27, 2019

subprocess: close pipes/fds by using ExitStack #11686

Merged

+                              os.close(errwrite)
+                      if devnull_fd is not None:
+                          os.close(devnull_fd)

bpo-35537: subprocess can use posix_spawn with pipes by vstinner · Pull Request #11575 · python/cpython · GitHub

Uh oh!

Uh oh!

bpo-35537: subprocess can use posix_spawn with pipes - #11575

bpo-35537: subprocess can use posix_spawn with pipes#11575vstinner merged 1 commit intopython:masterpython/cpython:masterfrom vstinner:subprocess_spawn2Copy head branch name to clipboard

Conversation

vstinner commented Jan 16, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Jan 16, 2019

Uh oh!

vstinner commented Jan 16, 2019

Uh oh!

vstinner commented Jan 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vstinner commented Jan 16, 2019

Uh oh!

vstinner commented Jan 16, 2019

Uh oh!

vstinner commented Jan 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jan 23, 2019

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giampaolo left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Jan 23, 2019

Uh oh!

vstinner commented Jan 23, 2019

Uh oh!

giampaolo commented Jan 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bpo-35537: subprocess can use posix_spawn with pipes#11575
vstinner merged 1 commit into
python:masterfrom
vstinner:subprocess_spawn2

vstinner commented Jan 16, 2019 •
edited by bedevere-bot
Loading