◐ Shell
reader mode source ↗
Skip to content

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

Merged
vstinner merged 1 commit into
python:masterfrom
vstinner:subprocess_spawn2
Jan 23, 2019
Merged

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

Conversation

@vstinner

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

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

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

vstinner commented Jan 16, 2019

Copy link
Copy Markdown
Member Author

Follow-up of PR #11452.

@vstinner vstinner mentioned this pull request Jan 16, 2019
Merged
@vstinner

vstinner commented Jan 16, 2019

Copy link
Copy Markdown
Member Author

cc @izbyshev, @pablogsal, @nanjekyejoannah

@vstinner

vstinner commented Jan 16, 2019

Copy link
Copy Markdown
Member Author

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

@pablogsal pablogsal self-requested a review January 16, 2019 14:34
@vstinner
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.
@vstinner

vstinner commented Jan 16, 2019

Copy link
Copy Markdown
Member Author

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

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

@vstinner

vstinner commented Jan 16, 2019

Copy link
Copy Markdown
Member Author

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

@vstinner

vstinner commented Jan 22, 2019

Copy link
Copy Markdown
Member Author

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

@bedevere-bot

bedevere-bot commented Jan 23, 2019

Copy link
Copy Markdown

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

vstinner
vstinner commented Jan 23, 2019
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hide comment

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

giampaolo
giampaolo approved these changes Jan 23, 2019
View reviewed changes

@giampaolo giampaolo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

LGTM

@vstinner vstinner merged commit f6243ac into python:master Jan 23, 2019
@vstinner vstinner deleted the subprocess_spawn2 branch January 23, 2019 18:00
@vstinner

vstinner commented Jan 23, 2019

Copy link
Copy Markdown
Member Author

@giampaolo: "LGTM"

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

@vstinner

vstinner commented Jan 23, 2019

Copy link
Copy Markdown
Member Author

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

@giampaolo giampaolo mentioned this pull request Jan 27, 2019
Merged
@giampaolo

giampaolo commented Jan 27, 2019

Copy link
Copy Markdown
Contributor

@vstinner here it is: #11686

gpshead pushed a commit that referenced this pull request Jan 29, 2019
@giampaolo @gpshead
subprocess: close pipes/fds by using ExitStack (GH-11686) …
bafa848 
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@giampaolo giampaolo giampaolo approved these changes

@gpshead gpshead Awaiting requested review from gpshead

@pablogsal pablogsal Awaiting requested review from pablogsal

+1 more reviewer

@izbyshev izbyshev izbyshev left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@vstinner @bedevere-bot @giampaolo @izbyshev @the-knights-who-say-ni