Skip to content

Commit e22d1da

Browse files
gpsheadclaude
andcommitted
Simplify _communicate_streams() to only accept file objects
Remove support for raw file descriptors in _communicate_streams(), requiring all streams to be file objects. This simplifies both the Windows and POSIX implementations by removing isinstance() checks and fd-wrapping logic. The run_pipeline() function now wraps the stderr pipe's read end with os.fdopen() immediately after creation. This change makes _communicate_streams() more compatible with Popen.communicate() which already uses file objects, enabling potential future refactoring to share the multiplexed I/O logic. Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
1 parent 2470e14 commit e22d1da

File tree

1 file changed

+31
-49
lines changed

1 file changed

+31
-49
lines changed

Lib/subprocess.py

Lines changed: 31 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -333,18 +333,19 @@ def _communicate_streams(stdin=None, input_data=None, read_streams=None,
333333
"""
334334
Multiplex I/O: write input_data to stdin, read from read_streams.
335335
336-
Works with both file objects and raw file descriptors.
336+
All streams must be file objects (not raw file descriptors).
337337
All I/O is done in binary mode; caller handles text encoding.
338338
339339
Args:
340-
stdin: Writable file object for input, or None
340+
stdin: Writable binary file object for input, or None
341341
input_data: Bytes to write to stdin, or None
342-
read_streams: List of readable file objects or raw fds to read from
342+
read_streams: List of readable binary file objects to read from
343343
timeout: Timeout in seconds, or None for no timeout
344344
cmd_for_timeout: Value to use for TimeoutExpired.cmd
345345
346346
Returns:
347-
Dict mapping each item in read_streams to its bytes data
347+
Dict mapping each file object in read_streams to its bytes data.
348+
All file objects in read_streams will be closed.
348349
349350
Raises:
350351
TimeoutExpired: If timeout expires (with partial data)
@@ -377,22 +378,15 @@ def _communicate_streams_windows(stdin, input_data, read_streams,
377378
"""Windows implementation using threads."""
378379
threads = []
379380
buffers = {}
380-
fds_to_close = []
381381

382-
# Start reader threads
382+
# Start reader threads for each stream
383383
for stream in read_streams:
384384
buf = []
385385
buffers[stream] = buf
386-
# Wrap raw fds in file objects
387-
if isinstance(stream, int):
388-
fobj = os.fdopen(os.dup(stream), 'rb')
389-
fds_to_close.append(stream)
390-
else:
391-
fobj = stream
392-
t = threading.Thread(target=_reader_thread_func, args=(fobj, buf))
386+
t = threading.Thread(target=_reader_thread_func, args=(stream, buf))
393387
t.daemon = True
394388
t.start()
395-
threads.append((stream, t, fobj))
389+
threads.append((stream, t))
396390

397391
# Write stdin
398392
if stdin and input_data:
@@ -413,7 +407,7 @@ def _communicate_streams_windows(stdin, input_data, read_streams,
413407
raise
414408

415409
# Join threads with timeout
416-
for stream, t, fobj in threads:
410+
for stream, t in threads:
417411
remaining = _remaining_time_helper(endtime)
418412
if remaining is not None and remaining < 0:
419413
remaining = 0
@@ -425,28 +419,17 @@ def _communicate_streams_windows(stdin, input_data, read_streams,
425419
cmd_for_timeout, orig_timeout,
426420
output=results.get(read_streams[0]) if read_streams else None)
427421

428-
# Close any raw fds we duped
429-
for fd in fds_to_close:
430-
try:
431-
os.close(fd)
432-
except OSError:
433-
pass
434-
435422
# Collect results
436423
return {stream: (buf[0] if buf else b'') for stream, buf in buffers.items()}
437424

438425
else:
439426
def _communicate_streams_posix(stdin, input_data, read_streams,
440427
endtime, orig_timeout, cmd_for_timeout):
441428
"""POSIX implementation using selectors."""
442-
# Normalize read_streams: build mapping of fd -> (original_key, chunks)
443-
fd_info = {} # fd -> (original_stream, chunks_list)
429+
# Build mapping of fd -> (file_object, chunks_list)
430+
fd_info = {}
444431
for stream in read_streams:
445-
if isinstance(stream, int):
446-
fd = stream
447-
else:
448-
fd = stream.fileno()
449-
fd_info[fd] = (stream, [])
432+
fd_info[stream.fileno()] = (stream, [])
450433

451434
# Prepare stdin
452435
stdin_fd = None
@@ -477,8 +460,8 @@ def _communicate_streams_posix(stdin, input_data, read_streams,
477460
remaining = _remaining_time_helper(endtime)
478461
if remaining is not None and remaining < 0:
479462
# Timed out - collect partial results
480-
results = {orig: b''.join(chunks)
481-
for fd, (orig, chunks) in fd_info.items()}
463+
results = {stream: b''.join(chunks)
464+
for fd, (stream, chunks) in fd_info.items()}
482465
raise TimeoutExpired(
483466
cmd_for_timeout, orig_timeout,
484467
output=results.get(read_streams[0]) if read_streams else None)
@@ -487,8 +470,8 @@ def _communicate_streams_posix(stdin, input_data, read_streams,
487470

488471
# Check timeout after select
489472
if endtime is not None and _time() > endtime:
490-
results = {orig: b''.join(chunks)
491-
for fd, (orig, chunks) in fd_info.items()}
473+
results = {stream: b''.join(chunks)
474+
for fd, (stream, chunks) in fd_info.items()}
492475
raise TimeoutExpired(
493476
cmd_for_timeout, orig_timeout,
494477
output=results.get(read_streams[0]) if read_streams else None)
@@ -520,16 +503,14 @@ def _communicate_streams_posix(stdin, input_data, read_streams,
520503
else:
521504
fd_info[key.fd][1].append(data)
522505

523-
# Build results: map original stream keys to joined data
506+
# Build results and close all file objects
524507
results = {}
525-
for fd, (orig_stream, chunks) in fd_info.items():
526-
results[orig_stream] = b''.join(chunks)
527-
# Close file objects (but not raw fds - caller manages those)
528-
if not isinstance(orig_stream, int):
529-
try:
530-
orig_stream.close()
531-
except OSError:
532-
pass
508+
for fd, (stream, chunks) in fd_info.items():
509+
results[stream] = b''.join(chunks)
510+
try:
511+
stream.close()
512+
except OSError:
513+
pass
533514

534515
return results
535516

@@ -942,13 +923,14 @@ def run_pipeline(*commands, input=None, capture_output=False, timeout=None,
942923
stdout_arg = kwargs.pop('stdout', None)
943924

944925
processes = []
945-
stderr_read_fd = None # Read end of shared stderr pipe (for parent)
926+
stderr_reader = None # File object for reading shared stderr (for parent)
946927
stderr_write_fd = None # Write end of shared stderr pipe (for children)
947928

948929
try:
949930
# Create a single stderr pipe that all processes will share
950931
if capture_stderr:
951932
stderr_read_fd, stderr_write_fd = os.pipe()
933+
stderr_reader = os.fdopen(stderr_read_fd, 'rb')
952934

953935
for i, cmd in enumerate(commands):
954936
is_first = (i == 0)
@@ -1017,8 +999,8 @@ def run_pipeline(*commands, input=None, capture_output=False, timeout=None,
1017999
read_streams = []
10181000
if last_proc.stdout is not None:
10191001
read_streams.append(last_proc.stdout)
1020-
if stderr_read_fd is not None:
1021-
read_streams.append(stderr_read_fd)
1002+
if stderr_reader is not None:
1003+
read_streams.append(stderr_reader)
10221004

10231005
# Use multiplexed I/O to handle stdin/stdout/stderr concurrently
10241006
# This avoids deadlocks from pipe buffer limits
@@ -1043,7 +1025,7 @@ def run_pipeline(*commands, input=None, capture_output=False, timeout=None,
10431025

10441026
# Extract results
10451027
stdout = results.get(last_proc.stdout)
1046-
stderr = results.get(stderr_read_fd)
1028+
stderr = results.get(stderr_reader)
10471029

10481030
# Decode stdout if in text mode (Popen text mode only applies to
10491031
# streams it creates, but we read via _communicate_streams which
@@ -1087,10 +1069,10 @@ def run_pipeline(*commands, input=None, capture_output=False, timeout=None,
10871069
proc.stdin.close()
10881070
if proc.stdout and not proc.stdout.closed:
10891071
proc.stdout.close()
1090-
# Close stderr pipe file descriptor
1091-
if stderr_read_fd is not None:
1072+
# Close stderr pipe (reader is a file object, writer is a raw fd)
1073+
if stderr_reader is not None and not stderr_reader.closed:
10921074
try:
1093-
os.close(stderr_read_fd)
1075+
stderr_reader.close()
10941076
except OSError:
10951077
pass
10961078
if stderr_write_fd is not None:

0 commit comments

Comments
 (0)