[PATCH 05/22] Simplify iutil.execReadlines.

David Shea dshea at redhat.com
Wed Jun 3 16:03:29 UTC 2015


Remove the thread and queue between execReadlines and the process it
starts. Instead of reading child process output into an unbounded
buffer, have the child block until we are ready to read input, and kill
the child when we are done reading input. Return an iterator object
instead of a generator so that we can kill the child on __del__.

Add a test for whether the child is killed when the iterator is
unreferenced.

(cherry picked from commit 92765220a8e08e779908944f06e465797f638cdd)

Related: rhbz#1188287
---
 pyanaconda/iutil.py                  |  61 ++++++++++++---------
 tests/pyanaconda_tests/iutil_test.py | 101 +++++++++++++++++++++++++++++++++++
 2 files changed, 137 insertions(+), 25 deletions(-)

diff --git a/pyanaconda/iutil.py b/pyanaconda/iutil.py
index fcba391..192bf6e 100644
--- a/pyanaconda/iutil.py
+++ b/pyanaconda/iutil.py
@@ -31,8 +31,6 @@ import string
 import tempfile
 import types
 import re
-from threading import Thread
-from Queue import Queue, Empty
 from urllib import quote, unquote
 import signal
 
@@ -265,13 +263,42 @@ def execReadlines(command, argv, stdin=None, root='/', env_prune=None):
         :param env_prune: environment variable to remove before execution
 
         Output from the file is not logged to program.log
-        This returns a generator with the lines from the command until it has finished
+        This returns an iterator with the lines from the command until it has finished
     """
-    # Return the lines from stdout via a Queue
-    def queue_lines(out, queue):
-        for line in iter(out.readline, b''):
-            queue.put(line.strip())
-        out.close()
+
+    class ExecLineReader(object):
+        """Iterator class for returning lines from a process and cleaning
+           up the process when the output is no longer needed.
+        """
+
+        def __init__(self, proc, argv):
+            self._proc = proc
+            self._argv = argv
+
+        def __iter__(self):
+            return self
+
+        def __del__(self):
+            # See if the process is still running
+            if self._proc.poll() is None:
+                # Stop the process and ignore any problems that might arise
+                try:
+                    self._proc.terminate()
+                except OSError:
+                    pass
+
+        def next(self):
+            # 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 os.WIFSIGNALED(self._proc.returncode):
+                        raise OSError("process '%s' was killed" % self._argv)
+                raise StopIteration
+
+            return line.strip()
 
     argv = [command] + argv
 
@@ -282,23 +309,7 @@ def execReadlines(command, argv, stdin=None, root='/', env_prune=None):
             program_log.error("Error running %s: %s", argv[0], e.strerror)
         raise
 
-    q = Queue()
-    t = Thread(target=queue_lines, args=(proc.stdout, q))
-    t.daemon = True # thread dies with the program
-    t.start()
-
-    while True:
-        try:
-            line = q.get(timeout=.1)
-            yield line
-            q.task_done()
-        except Empty:
-            if proc.poll() is not None:
-                if os.WIFSIGNALED(proc.returncode):
-                    raise OSError("process '%s' was killed" % argv)
-                break
-    q.join()
-
+    return ExecLineReader(proc, argv)
 
 ## Run a shell.
 def execConsole():
diff --git a/tests/pyanaconda_tests/iutil_test.py b/tests/pyanaconda_tests/iutil_test.py
index 970d49f..08c9404 100644
--- a/tests/pyanaconda_tests/iutil_test.py
+++ b/tests/pyanaconda_tests/iutil_test.py
@@ -144,6 +144,78 @@ exit 0
                 self.assertEqual(rl_iterator.next(), "three")
                 self.assertRaises(StopIteration, rl_iterator.next)
 
+    def exec_readlines_test_exits(self):
+        """Test execReadlines in different child exit situations."""
+
+        # These tests raise OSError once output has been consumed
+
+        # Test a normal, non-0 exit
+        with tempfile.NamedTemporaryFile() as testscript:
+            testscript.write("""#!/bin/sh
+echo "one"
+echo "two"
+echo "three"
+exit 1
+""")
+            testscript.flush()
+
+            with timer(5):
+                rl_iterator = iutil.execReadlines("/bin/sh", [testscript.name])
+                self.assertEqual(rl_iterator.next(), "one")
+                self.assertEqual(rl_iterator.next(), "two")
+                self.assertEqual(rl_iterator.next(), "three")
+                self.assertRaises(OSError, rl_iterator.next)
+
+        # Test exit on signal
+        with tempfile.NamedTemporaryFile() as testscript:
+            testscript.write("""#!/bin/sh
+echo "one"
+echo "two"
+echo "three"
+kill -TERM $$
+""")
+            testscript.flush()
+
+            with timer(5):
+                rl_iterator = iutil.execReadlines("/bin/sh", [testscript.name])
+                self.assertEqual(rl_iterator.next(), "one")
+                self.assertEqual(rl_iterator.next(), "two")
+                self.assertEqual(rl_iterator.next(), "three")
+                self.assertRaises(OSError, rl_iterator.next)
+
+        # Repeat the above two tests, but exit before a final newline
+        with tempfile.NamedTemporaryFile() as testscript:
+            testscript.write("""#!/bin/sh
+echo "one"
+echo "two"
+echo -n "three"
+exit 1
+""")
+            testscript.flush()
+
+            with timer(5):
+                rl_iterator = iutil.execReadlines("/bin/sh", [testscript.name])
+                self.assertEqual(rl_iterator.next(), "one")
+                self.assertEqual(rl_iterator.next(), "two")
+                self.assertEqual(rl_iterator.next(), "three")
+                self.assertRaises(OSError, rl_iterator.next)
+
+        with tempfile.NamedTemporaryFile() as testscript:
+            testscript.write("""#!/bin/sh
+echo "one"
+echo "two"
+echo -n "three"
+kill -TERM $$
+""")
+            testscript.flush()
+
+            with timer(5):
+                rl_iterator = iutil.execReadlines("/bin/sh", [testscript.name])
+                self.assertEqual(rl_iterator.next(), "one")
+                self.assertEqual(rl_iterator.next(), "two")
+                self.assertEqual(rl_iterator.next(), "three")
+                self.assertRaises(OSError, rl_iterator.next)
+
     def exec_readlines_test_signals(self):
         """Test execReadlines and signal receipt."""
 
@@ -264,6 +336,35 @@ while true ; do sleep 1 ; done
                 proc.communicate()
                 self.assertEqual(proc.returncode, -(signal.SIGTERM))
 
+    def exec_readlines_auto_kill_test(self):
+        """Test execReadlines with reading only part of the output"""
+
+        with tempfile.NamedTemporaryFile() as testscript:
+            testscript.write("""#!/bin/sh
+# Output forever
+while true; do
+echo hey
+done
+""")
+            testscript.flush()
+
+            with timer(5):
+                rl_iterator = iutil.execReadlines("/bin/sh", [testscript.name])
+
+                # Save the process context
+                proc = rl_iterator._proc
+
+                # Read two lines worth
+                self.assertEqual(rl_iterator.next(), "hey")
+                self.assertEqual(rl_iterator.next(), "hey")
+
+                # Delete the iterator and wait for the process to be killed
+                del rl_iterator
+                proc.communicate()
+
+            # Check that the process is gone
+            self.assertIsNotNone(proc.poll())
+
 class MiscTests(unittest.TestCase):
     def get_dir_size_test(self):
         """Test the getDirSize."""
-- 
2.1.0



More information about the anaconda-patches mailing list