Skip to content

bpo-35537: subprocess can now use os.posix_spawnp#11579

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 from
Jan 16, 2019
Merged

bpo-35537: subprocess can now use os.posix_spawnp #11579

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
3 changes: 1 addition & 2 deletions Doc/whatsnew/3.8.rst
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -281,8 +281,7 @@ Optimizations

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

* :func:`shutil.copyfile`, :func:`shutil.copy`, :func:`shutil.copy2`,
:func:`shutil.copytree` and :func:`shutil.move` use platform-specific
Expand Down
13 changes: 10 additions & 3 deletions Lib/subprocess.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -655,6 +655,7 @@ def _use_posix_spawn():


_USE_POSIX_SPAWN = _use_posix_spawn()
_HAVE_POSIX_SPAWNP = hasattr(os, 'posix_spawnp')


class Popen(object):
Expand DownExpand Up@@ -1442,7 +1443,10 @@ def _get_handles(self, stdin, stdout, stderr):


def _posix_spawn(self, args, executable, env, restore_signals):
"""Execute program using os.posix_spawn()."""
"""Execute program using os.posix_spawnp().

Or use os.posix_spawn() if os.posix_spawnp() is not available.
"""
if env is None:
env = os.environ

Expand All@@ -1456,7 +1460,10 @@ def _posix_spawn(self, args, executable, env, restore_signals):
sigset.append(signum)
kwargs['setsigdef'] = sigset

self.pid = os.posix_spawn(executable, args, env, **kwargs)
if _HAVE_POSIX_SPAWNP:
self.pid = os.posix_spawnp(executable, args, env, **kwargs)

@izbyshevizbyshevJan 16, 2019

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.

Please keep in mind that _posixsubprocess is likely to use a different algorithm for searching in PATH compared to posix_spawnp implementations (and, moreover, posix_spawnp implementations differ between libcs and even between versions of the same libc). For example, musl uses execvpe as a "backend" for posix_spawnp, which skips only 3 kinds of errors, while _posixsubprocess skips all errors. Also, EACCES is treated in a special way by both glibc and musl.

Also, the default PATH if the environment variable is not set is likely to differ (e.g., glibc uses /bin:/usr/bin, musl uses /usr/local/bin:/bin:/usr/bin, and Python uses :/bin:/usr/bin).

@vstinnervstinnerJan 16, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

"... glibc uses /bin:/usr/bin ... and Python uses :/bin:/usr/bin"

Oh. ":" at the start of posixpath.defpath is surprising. Is it a bug? What's the point of putting an empty string in the list of default search paths? It doesn't help to locate an executable...

@izbyshevizbyshevJan 16, 2019

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.

It's not a bug: empty entries in PATH mean "the current directory". See also NOTES in man execvp about glibc dropping this empty entry.

@vstinnervstinnerJan 16, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I created https://bugs.python.org/issue35755 to propose to remove it :-)

else:
self.pid = os.posix_spawn(executable, args, env, **kwargs)

def _execute_child(self, args, executable, preexec_fn, close_fds,
pass_fds, cwd, env,
Expand DownExpand Up@@ -1484,7 +1491,7 @@ def _execute_child(self, args, executable, preexec_fn, close_fds,
executable = args[0]

if (_USE_POSIX_SPAWN
and os.path.dirname(executable)
and (_HAVE_POSIX_SPAWNP or os.path.dirname(executable))
and preexec_fn is None
and not close_fds
and not pass_fds
Expand Down
3 changes: 2 additions & 1 deletion Lib/test/pythoninfo.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -612,7 +612,8 @@ def collect_get_config(info_add):

def collect_subprocess(info_add):
import subprocess
copy_attributes(info_add, subprocess, 'subprocess.%s', ('_USE_POSIX_SPAWN',))
attrs = ('_USE_POSIX_SPAWN', '_HAVE_POSIX_SPAWNP')
copy_attributes(info_add, subprocess, 'subprocess.%s', attrs)


def collect_info(info):
Expand Down