Skip to content

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
merged 1 commit into from
Jan 23, 2019
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
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Doc/whatsnew/3.8.rst
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 Lib/subprocess.py
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)
Copy link
Contributor

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.

Copy link
Member Author

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?

Copy link
Contributor

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

Copy link
Member Author

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.

Copy link
Contributor

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.


# 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))
Copy link
Contributor

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.

Copy link
Member Author

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

Copy link
Member Author

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

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