GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — subprocess: close pipes/fds by using ExitStack by giampaolo · Pull Request #11686 · python/cpython

Conversation

@giampaolo

@giampaolo giampaolo commented Jan 27, 2019 •
edited by gpshead
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

"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."

This is a follow up of bpo-35537. Rationale of this change originated during code inspection in #11575 (comment).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@giampaolo
subprocess: close pipes/fds by using ExitStack
a294842 
Rationale: #11575 (comment)
@giampaolo
giampaolo requested review from ncoghlan and vstinner January 27, 2019 14:53
@giampaolo
giampaolo requested a review from gpshead as a code owner January 27, 2019 14:53
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Jan 27, 2019
@bedevere-bot bedevere-bot added the awaiting merge label Jan 27, 2019
@giampaolo giampaolo mentioned this pull request Jan 27, 2019
bpo-35537: subprocess can use posix_spawn with pipes #11575
Merged
izbyshev
izbyshev reviewed Jan 27, 2019
View reviewed changes
Comment thread Lib/subprocess.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
giampaolo added 2 commits January 27, 2019 16:37
@giampaolo
avoid try/finally block around CloseHandle()
93ac155
@giampaolo
fix accidental removal of CloseHandle() calls
685cef2
@gpshead gpshead added skip issue and removed skip issue labels Jan 27, 2019
@gpshead

gpshead commented Jan 27, 2019 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

I suggest adding the following as a NEWS entry:

"An ExitStack is now used internally within subprocess.POpen to clean up pipe file handles. No behavior change in normal operation. But if closing one handle were ever to cause an exception, the others will now be closed instead of leaked."

I tried, but the blurbit web UI wouldn't let me add it to this PR.
reuse the bpo-35537 issue number, no good reason to file an issue specifically for this minor fix.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gpshead
gpshead approved these changes Jan 27, 2019
View reviewed changes
@giampaolo
add NEWS entry
65374c6
gpshead
gpshead approved these changes Jan 29, 2019
View reviewed changes
@gpshead gpshead added the skip issue label Jan 29, 2019
@gpshead
gpshead merged commit bafa848 into python:master Jan 29, 2019
@bedevere-bot bedevere-bot removed the awaiting merge label Jan 29, 2019
@giampaolo
giampaolo deleted the subprocess_spawn_exitstack branch February 3, 2019 22:27
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 Awaiting requested review from vstinner
@ncoghlan ncoghlan Awaiting requested review from ncoghlan
+1 more reviewer
@izbyshev izbyshev izbyshev left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

No one assigned

Labels

skip issue

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.

6 participants

@giampaolo @gpshead @ncoghlan @izbyshev @the-knights-who-say-ni @bedevere-bot
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.