[PATCH] Really fix unexpected exits in execReadlines

David Shea dshea at redhat.com
Thu Sep 25 20:31:25 UTC 2014


Due to the asynchronous nature of signal delivery, it's possible to
check on the execReadlines child process at a point where output has
been closed but the process is still, technically, alive. This situation
appeared in particular when running the nosetests on a system with one
CPU. To make the behavior of reading a line from an exiting process
deterministic, wait for the process to finish when output is complete.
The consequence of this is that processes that runs forever will cause
execReadlines to block forever, and the answer is to not run such a
process with execReadlines.

Also revert to and make intentional the old, accidental behavior of
raising OSError any time the process exits with a non-0 status, since
that actually seems more useful than just looking for signal exits.
---
 pyanaconda/iutil.py                  | 19 ++++++++++++++-----
 tests/pyanaconda_tests/iutil_test.py |  4 ++--
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/pyanaconda/iutil.py b/pyanaconda/iutil.py
index bf4928d..f51c9ae 100644
--- a/pyanaconda/iutil.py
+++ b/pyanaconda/iutil.py
@@ -326,6 +326,10 @@ def execReadlines(command, argv, stdin=None, root='/', env_prune=None):
     """ Execute an external command and return the line output of the command
         in real-time.
 
+        This method assumes that there is a reasonably low delay between the
+        end of output and the process exiting. If the child process closes
+        stdout and then keeps on truckin' there will be problems.
+
         :param command: The command to run
         :param argv: The argument list
         :param stdin: The file object to read stdin from.
@@ -363,11 +367,16 @@ def execReadlines(command, argv, stdin=None, root='/', env_prune=None):
             # Read the next line, blocking if a line is not yet available
             line = self._proc.stdout.readline()
             if line == '':
-                # Output finished, check for the process dying unexpectedly
-                # and stop the iteration
-                if self._proc.poll() is not None:
-                    if self._proc.returncode < 0:
-                        raise OSError("process '%s' was killed" % self._argv)
+                # Output finished, wait for the process to end
+                self._proc.communicate()
+
+                # Check for successful exit
+                if self._proc.returncode < 0:
+                    raise OSError("process '%s' was killed by signal %s" %
+                            (self._argv, -self._proc.returncode))
+                elif self._proc.returncode > 0:
+                    raise OSError("process '%s' exited with status %s" %
+                            (self._argv, self._proc.returncode))
                 raise StopIteration
 
             return line.strip()
diff --git a/tests/pyanaconda_tests/iutil_test.py b/tests/pyanaconda_tests/iutil_test.py
index da33d8e..82148d5 100644
--- a/tests/pyanaconda_tests/iutil_test.py
+++ b/tests/pyanaconda_tests/iutil_test.py
@@ -184,7 +184,7 @@ exit 1
                 self.assertEqual(rl_iterator.next(), "one")
                 self.assertEqual(rl_iterator.next(), "two")
                 self.assertEqual(rl_iterator.next(), "three")
-                self.assertRaises(StopIteration, rl_iterator.next)
+                self.assertRaises(OSError, rl_iterator.next)
 
         # Test exit on signal
         with tempfile.NamedTemporaryFile() as testscript:
@@ -218,7 +218,7 @@ exit 1
                 self.assertEqual(rl_iterator.next(), "one")
                 self.assertEqual(rl_iterator.next(), "two")
                 self.assertEqual(rl_iterator.next(), "three")
-                self.assertRaises(StopIteration, rl_iterator.next)
+                self.assertRaises(OSError, rl_iterator.next)
 
         with tempfile.NamedTemporaryFile() as testscript:
             testscript.write("""#!/bin/sh
-- 
2.1.0



More information about the anaconda-patches mailing list