processutils: ensure on_completion callback is always called

If the subprocess.Popen.communicate method raises an exception,
the on_completion callback is never invoked. If a caller is
trying to use on_execute + on_completion to track lifecycle
of a process this creates a problem, as they cannot reliably
detect completion.

Conflicts:
        oslo/concurrency/processutils.py
        tests/unit/test_processutils.py

Change-Id: I22b2d7bde8797276f7670bc289d915dab5122481
Closes-bug: #1470868
(cherry picked from commit ab78480659834ed07c84f8569df5d2c27965af00)

2 files changed, 30 insertions, 10 deletions

diff --git a/oslo/concurrency/processutils.py b/oslo/concurrency/processutils.py
index 240d296..174270c 100644
--- a/oslo/concurrency/processutils.py
+++ b/oslo/concurrency/processutils.py
@@ -223,16 +223,17 @@ def execute(*cmd, **kwargs):
             if on_execute:
                 on_execute(obj)
 
-            result = obj.communicate(process_input)
-
-            obj.stdin.close()  # pylint: disable=E1101
-            _returncode = obj.returncode  # pylint: disable=E1101
-            end_time = time.time() - start_time
-            LOG.log(loglevel, 'CMD "%s" returned: %s in %ss' %
-                    (sanitized_cmd, _returncode, end_time))
-
-            if on_completion:
-                on_completion(obj)
+            try:
+                result = obj.communicate(process_input)
+
+                obj.stdin.close()  # pylint: disable=E1101
+                _returncode = obj.returncode  # pylint: disable=E1101
+                end_time = time.time() - start_time
+                LOG.log(loglevel, 'CMD "%s" returned: %s in %ss' %
+                        (sanitized_cmd, _returncode, end_time))
+            finally:
+                if on_completion:
+                    on_completion(obj)
 
             if not ignore_exit_code and _returncode not in check_exit_code:
                 (stdout, stderr) = result
diff --git a/tests/unit/test_processutils.py b/tests/unit/test_processutils.py
index 903c2ed..d0e689e 100644
--- a/tests/unit/test_processutils.py
+++ b/tests/unit/test_processutils.py
@@ -20,6 +20,7 @@ import logging
 import multiprocessing
 import os
 import stat
+import subprocess
 import tempfile
 
 import fixtures
@@ -72,6 +73,24 @@ class UtilsTest(test_base.BaseTestCase):
         self.assertEqual(1, on_execute_callback.call_count)
         self.assertEqual(1, on_completion_callback.call_count)
 
+    @mock.patch.object(subprocess.Popen, "communicate")
+    def test_execute_with_callback_and_errors(self, mock_comm):
+        on_execute_callback = mock.Mock()
+        on_completion_callback = mock.Mock()
+
+        def fake_communicate(*args):
+            raise IOError("Broken pipe")
+
+        mock_comm.side_effect = fake_communicate
+
+        self.assertRaises(IOError,
+                          processutils.execute,
+                          "/bin/true",
+                          on_execute=on_execute_callback,
+                          on_completion=on_completion_callback)
+        self.assertEqual(1, on_execute_callback.call_count)
+        self.assertEqual(1, on_completion_callback.call_count)
+
 
 class ProcessExecutionErrorTest(test_base.BaseTestCase):