From 1ef8c7e886ea5260e5a6967ec2b8a4c32640f1a8 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith ext:(%20%5BGoogle%20Inc.%5D)" Date: Sat, 4 Jun 2016 00:22:17 +0000 Subject: Fixes Issue #26373: subprocess.Popen.communicate now correctly ignores BrokenPipeError when the child process dies before .communicate() is called in more (all?) circumstances. --- Lib/subprocess.py | 23 +++++++++++++++++----- Lib/test/test_subprocess.py | 47 +++++++++++++++++++++++++++++++++++++++++++++ Misc/NEWS | 4 ++++ 3 files changed, 69 insertions(+), 5 deletions(-) diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 4d316e6..d8d6ab2 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -1011,8 +1011,7 @@ class Popen(object): try: self.stdin.write(input) except BrokenPipeError: - # communicate() must ignore broken pipe error - pass + pass # communicate() must ignore broken pipe errors. except OSError as e: if e.errno == errno.EINVAL and self.poll() is not None: # Issue #19612: On Windows, stdin.write() fails with EINVAL @@ -1020,7 +1019,15 @@ class Popen(object): pass else: raise - self.stdin.close() + try: + self.stdin.close() + except BrokenPipeError: + pass # communicate() must ignore broken pipe errors. + except OSError as e: + if e.errno == errno.EINVAL and self.poll() is not None: + pass + else: + raise def communicate(self, input=None, timeout=None): """Interact with process: Send data to stdin. Read data from @@ -1661,9 +1668,15 @@ class Popen(object): if self.stdin and not self._communication_started: # Flush stdio buffer. This might block, if the user has # been writing to .stdin in an uncontrolled fashion. - self.stdin.flush() + try: + self.stdin.flush() + except BrokenPipeError: + pass # communicate() must ignore BrokenPipeError. if not input: - self.stdin.close() + try: + self.stdin.close() + except BrokenPipeError: + pass # communicate() must ignore BrokenPipeError. stdout = None stderr = None diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 9c7aa93..a5ecf67 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -1,4 +1,5 @@ import unittest +from unittest import mock from test.support import script_helper from test import support import subprocess @@ -1240,6 +1241,52 @@ class ProcessTestCase(BaseTestCase): fds_after_exception = os.listdir(fd_directory) self.assertEqual(fds_before_popen, fds_after_exception) + def test_communicate_BrokenPipeError_stdin_close(self): + # By not setting stdout or stderr or a timeout we force the fast path + # that just calls _stdin_write() internally due to our mock. + proc = subprocess.Popen([sys.executable, '-c', 'pass']) + with proc, mock.patch.object(proc, 'stdin') as mock_proc_stdin: + mock_proc_stdin.close.side_effect = BrokenPipeError + proc.communicate() # Should swallow BrokenPipeError from close. + mock_proc_stdin.close.assert_called_with() + + def test_communicate_BrokenPipeError_stdin_write(self): + # By not setting stdout or stderr or a timeout we force the fast path + # that just calls _stdin_write() internally due to our mock. + proc = subprocess.Popen([sys.executable, '-c', 'pass']) + with proc, mock.patch.object(proc, 'stdin') as mock_proc_stdin: + mock_proc_stdin.write.side_effect = BrokenPipeError + proc.communicate(b'stuff') # Should swallow the BrokenPipeError. + mock_proc_stdin.write.assert_called_once_with(b'stuff') + mock_proc_stdin.close.assert_called_once_with() + + def test_communicate_BrokenPipeError_stdin_flush(self): + # Setting stdin and stdout forces the ._communicate() code path. + # python -h exits faster than python -c pass (but spams stdout). + proc = subprocess.Popen([sys.executable, '-h'], + stdin=subprocess.PIPE, + stdout=subprocess.PIPE) + with proc, mock.patch.object(proc, 'stdin') as mock_proc_stdin, \ + open('/dev/null', 'wb') as dev_null: + mock_proc_stdin.flush.side_effect = BrokenPipeError + # because _communicate registers a selector using proc.stdin... + mock_proc_stdin.fileno.return_value = dev_null.fileno() + # _communicate() should swallow BrokenPipeError from flush. + proc.communicate(b'stuff') + mock_proc_stdin.flush.assert_called_once_with() + + def test_communicate_BrokenPipeError_stdin_close_with_timeout(self): + # Setting stdin and stdout forces the ._communicate() code path. + # python -h exits faster than python -c pass (but spams stdout). + proc = subprocess.Popen([sys.executable, '-h'], + stdin=subprocess.PIPE, + stdout=subprocess.PIPE) + with proc, mock.patch.object(proc, 'stdin') as mock_proc_stdin: + mock_proc_stdin.close.side_effect = BrokenPipeError + # _communicate() should swallow BrokenPipeError from close. + proc.communicate(timeout=999) + mock_proc_stdin.close.assert_called_once_with() + class RunFuncTestCase(BaseTestCase): def run_python(self, code, **kwargs): diff --git a/Misc/NEWS b/Misc/NEWS index 6e07c8c..d58485a 100644 --- a/Misc/NEWS +++ b/Misc/NEWS @@ -128,6 +128,10 @@ Core and Builtins Library ------- +- Issue #26373: subprocess.Popen.communicate now correctly ignores + BrokenPipeError when the child process dies before .communicate() + is called in more/all circumstances. + - Issue #21776: distutils.upload now correctly handles HTTPError. Initial patch by Claudiu Popa. -- cgit v0.12