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 — bpo-35537: subprocess can use posix_spawn with pipes by vstinner · Pull Request #11575 · python/cpython

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

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

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

Uh oh!

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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
9bc8adf
bpo-35537: subprocess can use posix_spawn with pipes
vstinner Jan 16, 2019
File filter
Filter by extension
Clear filters
Conversations
Failed to load comments.
Loading

Uh oh!

There was an error while loading. Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading

Uh oh!

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

There are no files selected for viewing

4 changes: 2 additions & 2 deletions 4 Doc/whatsnew/3.8.rst
Show comments
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,8 +280,8 @@ Optimizations
and Linux (using glibc 2.24 or newer) if all these conditions are met:

* *close_fds* is false;
* *preexec_fn*, *pass_fds*, *cwd*, *stdin*, *stdout*, *stderr* and
*start_new_session* parameters are not set;
* *preexec_fn*, *pass_fds*, *cwd* and *start_new_session* parameters
are not set;
* the *executable* path contains a directory.

* :func:`shutil.copyfile`, :func:`shutil.copy`, :func:`shutil.copy2`,
Expand Down
92 changes: 64 additions & 28 deletions 92 Lib/subprocess.py
Show comments
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1066,6 +1066,34 @@ def wait(self, timeout=None):
pass
raise # resume the KeyboardInterrupt

def _close_pipe_fds(self,
p2cread, p2cwrite,
c2pread, c2pwrite,
errread, errwrite):
# self._devnull is not always defined.
devnull_fd = getattr(self, '_devnull', None)

if _mswindows:
if p2cread != -1:
p2cread.Close()
if c2pwrite != -1:
c2pwrite.Close()
if errwrite != -1:
errwrite.Close()
else:
if p2cread != -1 and p2cwrite != -1 and p2cread != devnull_fd:
os.close(p2cread)
if c2pwrite != -1 and c2pread != -1 and c2pwrite != devnull_fd:
os.close(c2pwrite)
if errwrite != -1 and errread != -1 and errwrite != devnull_fd:
os.close(errwrite)

if devnull_fd is not None:
os.close(devnull_fd)
Comment thread

@giampaolo giampaolo Jan 23, 2019

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner Jan 23, 2019

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

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

@giampaolo giampaolo Jan 23, 2019

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner Jan 23, 2019

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

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

@giampaolo giampaolo Jan 23, 2019

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

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

# Prevent a double close of these handles/fds from __init__ on error.
self._closed_child_pipe_fds = True


if _mswindows:
#
Expand Down Expand Up @@ -1244,17 +1272,9 @@ def _execute_child(self, args, executable, preexec_fn, close_fds,
# output pipe are maintained in this process or else the
# pipe will not close when the child process exits and the
# ReadFile will hang.
if p2cread != -1:
p2cread.Close()
if c2pwrite != -1:
c2pwrite.Close()
if errwrite != -1:
errwrite.Close()
if hasattr(self, '_devnull'):
os.close(self._devnull)
# Prevent a double close of these handles/fds from __init__
# on error.
self._closed_child_pipe_fds = True
self._close_pipe_fds(p2cread, p2cwrite,
c2pread, c2pwrite,
errread, errwrite)

# Retain the process handle, but close the thread handle
self._child_created = True
Expand Down Expand Up @@ -1441,7 +1461,10 @@ def _get_handles(self, stdin, stdout, stderr):
errread, errwrite)


def _posix_spawn(self, args, executable, env, restore_signals):
def _posix_spawn(self, args, executable, env, restore_signals,
p2cread, p2cwrite,
c2pread, c2pwrite,
errread, errwrite):
"""Execute program using os.posix_spawn()."""
if env is None:
env = os.environ
Expand All @@ -1456,7 +1479,26 @@ def _posix_spawn(self, args, executable, env, restore_signals):
sigset.append(signum)
kwargs['setsigdef'] = sigset

file_actions = []
for fd in (p2cwrite, c2pread, errread):
if fd != -1:
file_actions.append((os.POSIX_SPAWN_CLOSE, fd))
Comment thread Outdated

@izbyshev izbyshev Jan 16, 2019

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner Jan 16, 2019

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

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

@vstinner vstinner Jan 16, 2019

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
for fd, fd2 in (
(p2cread, 0),
(c2pwrite, 1),
(errwrite, 2),
):
if fd != -1:
file_actions.append((os.POSIX_SPAWN_DUP2, fd, fd2))
if file_actions:
kwargs['file_actions'] = file_actions

self.pid = os.posix_spawn(executable, args, env, **kwargs)
self._child_created = True

self._close_pipe_fds(p2cread, p2cwrite,
c2pread, c2pwrite,
errread, errwrite)

def _execute_child(self, args, executable, preexec_fn, close_fds,
pass_fds, cwd, env,
Expand Down Expand Up @@ -1489,11 +1531,14 @@ def _execute_child(self, args, executable, preexec_fn, close_fds,
and not close_fds
and not pass_fds
and cwd is None
and p2cread == p2cwrite == -1
and c2pread == c2pwrite == -1
and errread == errwrite == -1
and (p2cread == -1 or p2cread > 2)
and (c2pwrite == -1 or c2pwrite > 2)
and (errwrite == -1 or errwrite > 2)
Comment thread
vstinner marked this conversation as resolved.
Resolved

Uh oh!

There was an error while loading. Please reload this page.
and not start_new_session):
self._posix_spawn(args, executable, env, restore_signals)
self._posix_spawn(args, executable, env, restore_signals,
p2cread, p2cwrite,
c2pread, c2pwrite,
errread, errwrite)
return

orig_executable = executable
Expand Down Expand Up @@ -1548,18 +1593,9 @@ def _execute_child(self, args, executable, preexec_fn, close_fds,
# be sure the FD is closed no matter what
os.close(errpipe_write)

# self._devnull is not always defined.
devnull_fd = getattr(self, '_devnull', None)
if p2cread != -1 and p2cwrite != -1 and p2cread != devnull_fd:
os.close(p2cread)
if c2pwrite != -1 and c2pread != -1 and c2pwrite != devnull_fd:
os.close(c2pwrite)
if errwrite != -1 and errread != -1 and errwrite != devnull_fd:
os.close(errwrite)
if devnull_fd is not None:
os.close(devnull_fd)
# Prevent a double close of these fds from __init__ on error.
self._closed_child_pipe_fds = True
self._close_pipe_fds(p2cread, p2cwrite,
c2pread, c2pwrite,
errread, errwrite)

# Wait for exec to fail or succeed; possibly raising an
# exception (limited in size)
Expand Down
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.