[PATCH 12/17] Simplify iutil.execReadlines.

David Shea dshea at redhat.com
Sun Sep 21 19:37:04 UTC 2014


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.
---
 pyanaconda/iutil.py                  |  61 ++++++++++--------
 tests/pyanaconda_tests/iutil_test.py | 118 +++++++++++++++++++++--------------
 2 files changed, 108 insertions(+), 71 deletions(-)

diff --git a/pyanaconda/iutil.py b/pyanaconda/iutil.py
index 999b4ec..97b9f35 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 gettext
 import signal
@@ -266,13 +264,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
 
@@ -283,23 +310,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 b048f9c..4d28606 100644
--- a/tests/pyanaconda_tests/iutil_test.py
+++ b/tests/pyanaconda_tests/iutil_test.py
@@ -21,7 +21,6 @@
 
 from pyanaconda import iutil
 import unittest
-import types
 import os
 import tempfile
 import signal
@@ -103,12 +102,10 @@ class RunProgramTests(unittest.TestCase):
         # test some lines are returned
         self.assertGreater(len(list(iutil.execReadlines("ls", ["--help"]))), 0)
 
-        # check that it always returns a generator for both
+        # check that it always returns an iterator for both
         # if there is some output and if there isn't any
-        self.assertIsInstance(iutil.execReadlines("ls", ["--help"]),
-                              types.GeneratorType)
-        self.assertIsInstance(iutil.execReadlines("true", []),
-                              types.GeneratorType)
+        self.assertTrue(hasattr(iutil.execReadlines("ls", ["--help"]), "__iter__"))
+        self.assertTrue(hasattr(iutil.execReadlines("true", []), "__iter__"))
 
     def exec_readlines_test_normal_output(self):
         """Test the output of execReadlines."""
@@ -124,11 +121,11 @@ exit 0
             testscript.flush()
 
             with timer(5):
-                rl_generator = iutil.execReadlines("/bin/sh", [testscript.name])
-                self.assertEqual(rl_generator.next(), "one")
-                self.assertEqual(rl_generator.next(), "two")
-                self.assertEqual(rl_generator.next(), "three")
-                self.assertRaises(StopIteration, rl_generator.next)
+                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(StopIteration, rl_iterator.next)
 
         # Test output with no end of line
         with tempfile.NamedTemporaryFile() as testscript:
@@ -141,11 +138,11 @@ exit 0
             testscript.flush()
 
             with timer(5):
-                rl_generator = iutil.execReadlines("/bin/sh", [testscript.name])
-                self.assertEqual(rl_generator.next(), "one")
-                self.assertEqual(rl_generator.next(), "two")
-                self.assertEqual(rl_generator.next(), "three")
-                self.assertRaises(StopIteration, rl_generator.next)
+                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(StopIteration, rl_iterator.next)
 
     def exec_readlines_test_exits(self):
         """Test execReadlines in different child exit situations."""
@@ -163,11 +160,11 @@ exit 1
             testscript.flush()
 
             with timer(5):
-                rl_generator = iutil.execReadlines("/bin/sh", [testscript.name])
-                self.assertEqual(rl_generator.next(), "one")
-                self.assertEqual(rl_generator.next(), "two")
-                self.assertEqual(rl_generator.next(), "three")
-                self.assertRaises(OSError, rl_generator.next)
+                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:
@@ -180,11 +177,11 @@ kill -TERM $$
             testscript.flush()
 
             with timer(5):
-                rl_generator = iutil.execReadlines("/bin/sh", [testscript.name])
-                self.assertEqual(rl_generator.next(), "one")
-                self.assertEqual(rl_generator.next(), "two")
-                self.assertEqual(rl_generator.next(), "three")
-                self.assertRaises(OSError, rl_generator.next)
+                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:
@@ -197,11 +194,11 @@ exit 1
             testscript.flush()
 
             with timer(5):
-                rl_generator = iutil.execReadlines("/bin/sh", [testscript.name])
-                self.assertEqual(rl_generator.next(), "one")
-                self.assertEqual(rl_generator.next(), "two")
-                self.assertEqual(rl_generator.next(), "three")
-                self.assertRaises(OSError, rl_generator.next)
+                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
@@ -213,11 +210,11 @@ kill -TERM $$
             testscript.flush()
 
             with timer(5):
-                rl_generator = iutil.execReadlines("/bin/sh", [testscript.name])
-                self.assertEqual(rl_generator.next(), "one")
-                self.assertEqual(rl_generator.next(), "two")
-                self.assertEqual(rl_generator.next(), "three")
-                self.assertRaises(OSError, rl_generator.next)
+                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."""
@@ -236,11 +233,11 @@ exit 0
                 testscript.flush()
 
                 with timer(5):
-                    rl_generator = iutil.execReadlines("/bin/sh", [testscript.name])
-                    self.assertEqual(rl_generator.next(), "one")
-                    self.assertEqual(rl_generator.next(), "two")
-                    self.assertEqual(rl_generator.next(), "three")
-                    self.assertRaises(StopIteration, rl_generator.next)
+                    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(StopIteration, rl_iterator.next)
         finally:
             signal.signal(signal.SIGHUP, old_HUP_handler)
 
@@ -260,11 +257,11 @@ exit 0
                 testscript.flush()
 
                 with timer(5):
-                    rl_generator = iutil.execReadlines("/bin/sh", [testscript.name])
-                    self.assertEqual(rl_generator.next(), "one")
-                    self.assertEqual(rl_generator.next(), "two")
-                    self.assertEqual(rl_generator.next(), "three")
-                    self.assertRaises(StopIteration, rl_generator.next)
+                    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(StopIteration, rl_iterator.next)
         finally:
             signal.signal(signal.SIGHUP, old_HUP_handler)
 
@@ -339,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."""
-- 
1.9.3



More information about the anaconda-patches mailing list