Skip to content

Commit eff75cc

Browse files
committed
gh-47798: run_pipeline: reject session/group kwargs and pipeline-level stderr=STDOUT
start_new_session and process_group: each command is spawned as a sibling child of the calling process, so applying these per command yields N separate sessions/groups rather than a single group spanning the pipeline. stderr=STDOUT at the pipeline level: would merge each non-final command's stderr into the next command's stdin. Per-command PipelineCommand(stderr=STDOUT) covers the legitimate use. Leave a note at Popen.__init__ reminding future kwarg additions to consider run_pipeline forwarding.
1 parent 84c9ec9 commit eff75cc

3 files changed

Lines changed: 64 additions & 18 deletions

File tree

Doc/library/subprocess.rst

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -316,6 +316,14 @@ underlying :class:`Popen` interface can be used directly.
316316
injection surface this function exists to avoid. When one command
317317
genuinely needs shell interpretation (a glob, or a shell builtin),
318318
wrap it in a :class:`PipelineCommand` with ``shell=True``.
319+
``start_new_session`` and ``process_group`` are also rejected: each
320+
command is spawned as a sibling child of the calling process, so
321+
applying these per command does not produce a single process group
322+
spanning the pipeline. ``stderr=STDOUT`` at the pipeline level is
323+
rejected because it would merge each non-final command's stderr into
324+
the next command's stdin; use a :class:`PipelineCommand` with
325+
``stderr=STDOUT`` for the one command that needs it, or
326+
``capture_output=True`` to capture stderr from every command.
319327

320328
.. rubric:: Standard error handling
321329

@@ -338,11 +346,8 @@ underlying :class:`Popen` interface can be used directly.
338346

339347
To exempt one command from the shared stderr pipe, wrap it in a
340348
:class:`PipelineCommand` with ``stderr=DEVNULL`` (discard) or
341-
``stderr=STDOUT`` (merge into that command's stdout). Note that
342-
passing ``stderr=STDOUT`` at the *pipeline* level redirects every
343-
command's stderr to its own stdout, which for non-final commands
344-
means stderr feeds the next command's stdin -- rarely what callers
345-
want.
349+
``stderr=STDOUT`` (merge into that command's stdout).
350+
``stderr=STDOUT`` at the *pipeline* level is rejected.
346351

347352
Examples::
348353

Lib/subprocess.py

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1108,6 +1108,26 @@ def run_pipeline(*commands, input=None, capture_output=False, timeout=None,
11081108
raise ValueError(
11091109
'executable= is not supported by run_pipeline')
11101110

1111+
if kwargs.get('stderr') is STDOUT:
1112+
raise ValueError(
1113+
'stderr=STDOUT at the run_pipeline level would merge each '
1114+
"non-final command's stderr into the next command's stdin. "
1115+
'Use PipelineCommand(cmd, stderr=STDOUT) for a single command, '
1116+
'or capture_output=True to capture stderr from every command.')
1117+
1118+
if kwargs.get('start_new_session') or kwargs.get('process_group') is not None:
1119+
# run_pipeline spawns each command as a sibling child of this
1120+
# process, so a per-command session/group does not give the shell
1121+
# "one process group per pipeline" semantic that callers passing
1122+
# these almost certainly want. Reject for now; a feature that
1123+
# places every command in a single new group is a possible
1124+
# follow-on.
1125+
raise ValueError(
1126+
'start_new_session and process_group are not supported by '
1127+
'run_pipeline; each command is spawned as a sibling child, '
1128+
'so a per-command session or group does not yield a single '
1129+
'process group for the pipeline')
1130+
11111131
commands = tuple(c if isinstance(c, PipelineCommand) else PipelineCommand(c)
11121132
for c in commands)
11131133

@@ -1581,6 +1601,9 @@ class Popen:
15811601
"""
15821602
_child_created = False # Set here since __del__ checks it
15831603

1604+
# When adding a new keyword here, consider whether forwarding it to
1605+
# every command in run_pipeline() makes sense; if not, reject or
1606+
# special-case it there.
15841607
def __init__(self, args, bufsize=-1, executable=None,
15851608
stdin=None, stdout=None, stderr=None,
15861609
preexec_fn=None, close_fds=True,

Lib/test/test_subprocess.py

Lines changed: 31 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2208,6 +2208,23 @@ def test_pipeline_capture_output_conflict(self):
22082208
)
22092209
self.assertIn('capture_output', str(cm.exception))
22102210

2211+
def test_pipeline_session_group_rejected(self):
2212+
"""start_new_session= and process_group= are rejected.
2213+
2214+
Each command is spawned as a sibling child of this process, so
2215+
per-command sessions/groups would not yield a single process
2216+
group spanning the pipeline.
2217+
"""
2218+
for kw in ({'start_new_session': True}, {'process_group': 0}):
2219+
with self.subTest(kw=kw):
2220+
with self.assertRaises(ValueError) as cm:
2221+
subprocess.run_pipeline(
2222+
[sys.executable, '-c', 'pass'],
2223+
[sys.executable, '-c', 'pass'],
2224+
**kw,
2225+
)
2226+
self.assertIn('process group', str(cm.exception))
2227+
22112228
def test_pipeline_close_fds_false_rejected(self):
22122229
"""Test that close_fds=False is rejected (would deadlock)"""
22132230
with self.assertRaises(ValueError) as cm:
@@ -2538,19 +2555,20 @@ def test_pipeline_check_true_success(self):
25382555
self.assertEqual(result.returncodes, (0, 0))
25392556
self.assertEqual(result.stdout.strip(), 'ok')
25402557

2541-
def test_pipeline_stderr_to_stdout(self):
2542-
"""stderr=STDOUT routes the final process's stderr to stdout"""
2543-
result = subprocess.run_pipeline(
2544-
[sys.executable, '-c', 'print("data")'],
2545-
[sys.executable, '-c',
2546-
'import sys; sys.stdout.write(sys.stdin.read()); '
2547-
'sys.stderr.write("ERR\\n")'],
2548-
stdout=subprocess.PIPE, stderr=subprocess.STDOUT
2549-
)
2550-
self.assertEqual(result.returncodes, [0, 0])
2551-
self.assertIn(b'data', result.stdout)
2552-
self.assertIn(b'ERR', result.stdout)
2553-
self.assertIsNone(result.stderr)
2558+
def test_pipeline_stderr_to_stdout_rejected(self):
2559+
"""stderr=STDOUT at the pipeline level is rejected.
2560+
2561+
It would merge each non-final command's stderr into the next
2562+
command's stdin; PipelineCommand(stderr=STDOUT) is the
2563+
per-command spelling.
2564+
"""
2565+
with self.assertRaises(ValueError) as cm:
2566+
subprocess.run_pipeline(
2567+
[sys.executable, '-c', 'pass'],
2568+
[sys.executable, '-c', 'pass'],
2569+
stdout=subprocess.PIPE, stderr=subprocess.STDOUT,
2570+
)
2571+
self.assertIn('PipelineCommand', str(cm.exception))
25542572

25552573
def test_pipeline_intermediate_stdout_closed_in_parent(self):
25562574
"""Parent closes intermediate stdout so an early-exiting consumer

0 commit comments

Comments
 (0)