From 526d7a8bb47bd8ff58c829c30384cd70cc5d0747 Mon Sep 17 00:00:00 2001 From: Artur Jamro Date: Sat, 29 Nov 2025 03:04:52 +0100 Subject: [PATCH 1/3] gh-141473: Fix subprocess.Popen.communicate to send input to stdin upon a subsequent post-timeout call (GH-141477) * gh-141473: Fix subprocess.Popen.communicate to send input to stdin * Docs: Clarify that `input` is one time only on `communicate()` * NEWS entry * Add a regression test. --------- Co-authored-by: Gregory P. Smith --- Doc/library/subprocess.rst | 4 ++- Lib/subprocess.py | 2 +- Lib/test/test_subprocess.py | 34 +++++++++++++++++++ ...-11-27-20-16-38.gh-issue-141473.Wq4xVN.rst | 4 +++ 4 files changed, 42 insertions(+), 2 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2025-11-27-20-16-38.gh-issue-141473.Wq4xVN.rst diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst index 1aade881745f21..43da804b62beb1 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -831,7 +831,9 @@ Instances of the :class:`Popen` class have the following methods: If the process does not terminate after *timeout* seconds, a :exc:`TimeoutExpired` exception will be raised. Catching this exception and - retrying communication will not lose any output. + retrying communication will not lose any output. Supplying *input* to a + subsequent post-timeout :meth:`communicate` call is in undefined behavior + and may become an error in the future. The child process is not killed if the timeout expires, so in order to cleanup properly a well-behaved application should kill the child process and diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 79251bd5310223..4955f77b60381f 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -2105,7 +2105,7 @@ def _communicate(self, input, endtime, orig_timeout): input_view = memoryview(self._input) with _PopenSelector() as selector: - if self.stdin and input: + if self.stdin and not self.stdin.closed and self._input: selector.register(self.stdin, selectors.EVENT_WRITE) if self.stdout and not self.stdout.closed: selector.register(self.stdout, selectors.EVENT_READ) diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index f0e350c71f60ea..9792d7c8983cd3 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -1643,6 +1643,40 @@ def test_wait_negative_timeout(self): self.assertEqual(proc.wait(), 0) + def test_post_timeout_communicate_sends_input(self): + """GH-141473 regression test; the stdin pipe must close""" + with subprocess.Popen( + [sys.executable, "-uc", """\ +import sys +while c := sys.stdin.read(512): + sys.stdout.write(c) +print() +"""], + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + ) as proc: + try: + data = f"spam{'#'*4096}beans" + proc.communicate( + input=data, + timeout=0, + ) + except subprocess.TimeoutExpired as exc: + pass + # Prior to the bugfix, this would hang as the stdin + # pipe to the child had not been closed. + try: + stdout, stderr = proc.communicate(timeout=15) + except subprocess.TimeoutExpired as exc: + self.fail("communicate() hung waiting on child process that should have seen its stdin pipe close and exit") + self.assertEqual( + proc.returncode, 0, + msg=f"STDERR:\n{stderr}\nSTDOUT:\n{stdout}") + self.assertStartsWith(stdout, "spam") + self.assertIn("beans", stdout) + class RunFuncTestCase(BaseTestCase): def run_python(self, code, **kwargs): diff --git a/Misc/NEWS.d/next/Library/2025-11-27-20-16-38.gh-issue-141473.Wq4xVN.rst b/Misc/NEWS.d/next/Library/2025-11-27-20-16-38.gh-issue-141473.Wq4xVN.rst new file mode 100644 index 00000000000000..f6aa592cefda35 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2025-11-27-20-16-38.gh-issue-141473.Wq4xVN.rst @@ -0,0 +1,4 @@ +When :meth:`subprocess.Popen.communicate` was called with *input* and a +*timeout* and is called for a second time after a +:exc:`~subprocess.TimeoutExpired` exception before the process has died, it +should no longer hang. From cc6bc4c97f7be5b401a91119ba603e6c1a07c99b Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" <68491+gpshead@users.noreply.github.com> Date: Fri, 28 Nov 2025 20:25:06 -0800 Subject: [PATCH 2/3] GH-134453: Fix subprocess memoryview input handling on POSIX (GH-134949) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix inconsistent subprocess.Popen.communicate() behavior between Windows and POSIX when using memoryview objects with non-byte elements as input. On POSIX systems, the code was incorrectly comparing bytes written against element count instead of byte count, causing data truncation for large inputs with non-byte element types. Changes: - Cast memoryview inputs to byte view when input is already a memoryview - Fix progress tracking to use len(input_view) instead of len(self._input) - Add comprehensive test coverage for memoryview inputs 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude * old-man-yells-at-ReST * Update 2025-05-30-18-37-44.gh-issue-134453.kxkA-o.rst * assertIsNone review feedback * fix memoryview_nonbytes test to fail without our fix on main, and have a nicer error. Thanks to Peter Bierma @ZeroIntensity for the code review. --- Lib/subprocess.py | 7 +++- Lib/test/test_subprocess.py | 42 +++++++++++++++++++ ...-05-30-18-37-44.gh-issue-134453.kxkA-o.rst | 4 ++ 3 files changed, 51 insertions(+), 2 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2025-05-30-18-37-44.gh-issue-134453.kxkA-o.rst diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 4955f77b60381f..168703828fc50e 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -2102,7 +2102,10 @@ def _communicate(self, input, endtime, orig_timeout): self._save_input(input) if self._input: - input_view = memoryview(self._input) + if not isinstance(self._input, memoryview): + input_view = memoryview(self._input) + else: + input_view = self._input.cast("b") # byte input required with _PopenSelector() as selector: if self.stdin and not self.stdin.closed and self._input: @@ -2138,7 +2141,7 @@ def _communicate(self, input, endtime, orig_timeout): selector.unregister(key.fileobj) key.fileobj.close() else: - if self._input_offset >= len(self._input): + if self._input_offset >= len(input_view): selector.unregister(key.fileobj) key.fileobj.close() elif key.fileobj in (self.stdout, self.stderr): diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 9792d7c8983cd3..5e73810dcb7253 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -957,6 +957,48 @@ def test_communicate(self): self.assertEqual(stdout, b"banana") self.assertEqual(stderr, b"pineapple") + def test_communicate_memoryview_input(self): + # Test memoryview input with byte elements + test_data = b"Hello, memoryview!" + mv = memoryview(test_data) + p = subprocess.Popen([sys.executable, "-c", + 'import sys; sys.stdout.write(sys.stdin.read())'], + stdin=subprocess.PIPE, + stdout=subprocess.PIPE) + self.addCleanup(p.stdout.close) + self.addCleanup(p.stdin.close) + (stdout, stderr) = p.communicate(mv) + self.assertEqual(stdout, test_data) + self.assertIsNone(stderr) + + def test_communicate_memoryview_input_nonbyte(self): + # Test memoryview input with non-byte elements (e.g., int32) + # This tests the fix for gh-134453 where non-byte memoryviews + # had incorrect length tracking on POSIX + import array + # Create an array of 32-bit integers that's large enough to trigger + # the chunked writing behavior (> PIPE_BUF) + pipe_buf = getattr(select, 'PIPE_BUF', 512) + # Each 'i' element is 4 bytes, so we need more than pipe_buf/4 elements + # Add some extra to ensure we exceed the buffer size + num_elements = pipe_buf + 1 + test_array = array.array('i', [0x64306f66 for _ in range(num_elements)]) + expected_bytes = test_array.tobytes() + mv = memoryview(test_array) + + p = subprocess.Popen([sys.executable, "-c", + 'import sys; ' + 'data = sys.stdin.buffer.read(); ' + 'sys.stdout.buffer.write(data)'], + stdin=subprocess.PIPE, + stdout=subprocess.PIPE) + self.addCleanup(p.stdout.close) + self.addCleanup(p.stdin.close) + (stdout, stderr) = p.communicate(mv) + self.assertEqual(stdout, expected_bytes, + msg=f"{len(stdout)=} =? {len(expected_bytes)=}") + self.assertIsNone(stderr) + def test_communicate_timeout(self): p = subprocess.Popen([sys.executable, "-c", 'import sys,os,time;' diff --git a/Misc/NEWS.d/next/Library/2025-05-30-18-37-44.gh-issue-134453.kxkA-o.rst b/Misc/NEWS.d/next/Library/2025-05-30-18-37-44.gh-issue-134453.kxkA-o.rst new file mode 100644 index 00000000000000..fee975ea70230c --- /dev/null +++ b/Misc/NEWS.d/next/Library/2025-05-30-18-37-44.gh-issue-134453.kxkA-o.rst @@ -0,0 +1,4 @@ +Fixed :func:`subprocess.Popen.communicate` ``input=`` handling of :class:`memoryview` +instances that were non-byte shaped on POSIX platforms. Those are now properly +cast to a byte shaped view instead of truncating the input. Windows platforms +did not have this bug. From 923056b2d41c4c28ad9163901053cd3824d775c5 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" <68491+gpshead@users.noreply.github.com> Date: Fri, 28 Nov 2025 21:03:06 -0800 Subject: [PATCH 3/3] gh-74389: gh-70560: subprocess.Popen.communicate() now ignores stdin.flush error when closed (GH-142061) gh-70560: gh-74389: subprocess.Popen.communicate() now ignores stdin.flush error when closed with a unittest and news entry. --- Lib/subprocess.py | 4 ++++ Lib/test/test_subprocess.py | 13 +++++++++++++ .../2025-11-29-04-20-44.gh-issue-74389.pW3URj.rst | 3 +++ 3 files changed, 20 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2025-11-29-04-20-44.gh-issue-74389.pW3URj.rst diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 168703828fc50e..99e2d0755d7e46 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -2077,6 +2077,10 @@ def _communicate(self, input, endtime, orig_timeout): self.stdin.flush() except BrokenPipeError: pass # communicate() must ignore BrokenPipeError. + except ValueError: + # ignore ValueError: I/O operation on closed file. + if not self.stdin.closed: + raise if not input: try: self.stdin.close() diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 5e73810dcb7253..2d717f985a3ac3 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -1104,6 +1104,19 @@ def test_writes_before_communicate(self): self.assertEqual(stdout, b"bananasplit") self.assertEqual(stderr, b"") + def test_communicate_stdin_closed_before_call(self): + # gh-70560, gh-74389: stdin.close() before communicate() + # should not raise ValueError from stdin.flush() + with subprocess.Popen([sys.executable, "-c", + 'import sys; sys.exit(0)'], + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE) as p: + p.stdin.close() # Close stdin before communicate + # This should not raise ValueError + (stdout, stderr) = p.communicate() + self.assertEqual(p.returncode, 0) + def test_universal_newlines_and_text(self): args = [ sys.executable, "-c", diff --git a/Misc/NEWS.d/next/Library/2025-11-29-04-20-44.gh-issue-74389.pW3URj.rst b/Misc/NEWS.d/next/Library/2025-11-29-04-20-44.gh-issue-74389.pW3URj.rst new file mode 100644 index 00000000000000..a9bf5f80d80c9f --- /dev/null +++ b/Misc/NEWS.d/next/Library/2025-11-29-04-20-44.gh-issue-74389.pW3URj.rst @@ -0,0 +1,3 @@ +When the stdin being used by a :class:`subprocess.Popen` instance is closed, +this is now ignored in :meth:`subprocess.Popen.communicate` instead of +leaving the class in an inconsistent state.