Cleanup control flow in main loop

removes some complexity that was introduced with the fix to kill
timed-out child processes

2 files changed, 23 insertions, 19 deletions

diff --git a/lib/mixlib/shellout/unix.rb b/lib/mixlib/shellout/unix.rb
index f884832..546d3ee 100644
--- a/lib/mixlib/shellout/unix.rb
+++ b/lib/mixlib/shellout/unix.rb
@@ -38,6 +38,7 @@ module Mixlib
       #   within +timeout+ seconds (default: 600s)
       def run_command
         @child_pid = fork_subprocess
+        @reaped = false
 
         configure_parent_process_file_descriptors
 
@@ -70,13 +71,7 @@ module Mixlib
             end
           end
 
-          unless @status
-            # make one more pass to get the last of the output after the
-            # child process dies
-            if attempt_reap
-              redo
-            end
-          end
+          attempt_reap
         end
 
         self
@@ -85,12 +80,11 @@ module Mixlib
         # is going to exit quickly, so we use the blocking variant of waitpid2
         reap
         raise
-      rescue CommandTimeout
-        raise
-      rescue Exception
-        reap_errant_child
-        raise
       ensure
+        reap_errant_child if should_reap?
+        # make one more pass to get the last of the output after the
+        # child process dies
+        attempt_buffer_read
         # no matter what happens, turn the GC back on, and hope whatever busted
         # version of ruby we're on doesn't allocate some objects during the next
         # GC run.
@@ -320,14 +314,20 @@ module Mixlib
         nil
       end
 
+      def should_reap?
+        # if we fail to fork, no child pid so nothing to reap
+        @child_pid && !@reaped
+      end
+
       def reap
         results = Process.waitpid2(@child_pid)
+        @reaped = true
         @status = results.last
       end
 
-
       def attempt_reap
         if results = Process.waitpid2(@child_pid, Process::WNOHANG)
+          @reaped = true
           @status = results.last
         else
           nil
diff --git a/spec/mixlib/shellout_spec.rb b/spec/mixlib/shellout_spec.rb
index d068dfc..ec2df8f 100644
--- a/spec/mixlib/shellout_spec.rb
+++ b/spec/mixlib/shellout_spec.rb
@@ -839,9 +839,11 @@ describe Mixlib::ShellOut do
         end
 
         it "should ask the process nicely to exit" do
-          lambda { executed_cmd }.should raise_error(Mixlib::ShellOut::CommandTimeout)
-          executed_cmd.stdout.should include("got term")
-          executed_cmd.exitstatus.should == 123
+          # note: let blocks don't correctly memoize if an exception is raised,
+          # so can't use executed_cmd
+          lambda { shell_cmd.run_command }.should raise_error(Mixlib::ShellOut::CommandTimeout)
+          shell_cmd.stdout.should include("got term")
+          shell_cmd.exitstatus.should == 123
         end
 
         context "and the child is unresponsive" do
@@ -854,9 +856,11 @@ describe Mixlib::ShellOut do
           end
 
           it "should KILL the wayward child" do
-            lambda { executed_cmd }.should raise_error(Mixlib::ShellOut::CommandTimeout)
-            executed_cmd.stdout.should include("nanana cant hear you")
-            executed_cmd.status.termsig.should == 9
+            # note: let blocks don't correctly memoize if an exception is raised,
+            # so can't use executed_cmd
+            lambda { shell_cmd.run_command}.should raise_error(Mixlib::ShellOut::CommandTimeout)
+            shell_cmd.stdout.should include("nanana cant hear you")
+            shell_cmd.status.termsig.should == 9
           end
         end
       end