Merge pull request #5868 from puppetlabs/revert-5844-PUP-6675

Revert "(PUP-6675) Use pipes instead of temporary files for Puppet exec"

lib/puppet/util/execution.rb

@@ -187,31 +187,18 @@ def self.execute(command, options = NoOptionsSpecified)
     null_file = Puppet.features.microsoft_windows? ? 'NUL' : '/dev/null'
 
     begin
-      # We close stdin/stdout/stderr immediately after execution as there no longer needed.
-      # In most cases they could be closed later, but when stdout is the writer pipe we
-      # must close it or we'll never reach eof on the reader.
-      reader, writer = IO.pipe unless options[:squelch]
       stdin = Puppet::FileSystem.open(options[:stdinfile] || null_file, nil, 'r')
-      stdout = options[:squelch] ? Puppet::FileSystem.open(null_file, nil, 'w') : writer
+      stdout = options[:squelch] ? Puppet::FileSystem.open(null_file, nil, 'w') : Puppet::FileSystem::Uniquefile.new('puppet')
       stderr = options[:combine] ? stdout : Puppet::FileSystem.open(null_file, nil, 'w')
 
       exec_args = [command, options, stdin, stdout, stderr]
-      output = ''
 
       if execution_stub = Puppet::Util::ExecutionStub.current_value
-        child_pid = execution_stub.call(*exec_args)
-        [stdin, stdout, stderr].each {|io| io.close rescue nil}
-        return child_pid
+        return execution_stub.call(*exec_args)
       elsif Puppet.features.posix?
         child_pid = nil
         begin
           child_pid = execute_posix(*exec_args)
-          [stdin, stdout, stderr].each {|io| io.close rescue nil}
-          unless options[:squelch]
-            while !reader.eof?
-              output << reader.read
-            end
-          end
           exit_status = Process.waitpid2(child_pid).last.exitstatus
           child_pid = nil
         rescue Timeout::Error => e
@@ -229,28 +216,28 @@ def self.execute(command, options = NoOptionsSpecified)
       elsif Puppet.features.microsoft_windows?
         process_info = execute_windows(*exec_args)
         begin
-          [stdin, stdout, stderr].each {|io| io.close rescue nil}
-          unless options[:squelch]
-            while !reader.eof?
-              output << reader.read
-            end
-          end
           exit_status = Puppet::Util::Windows::Process.wait_process(process_info.process_handle)
         ensure
           FFI::WIN32.CloseHandle(process_info.process_handle)
           FFI::WIN32.CloseHandle(process_info.thread_handle)
         end
       end
 
+      [stdin, stdout, stderr].each {|io| io.close rescue nil}
+
+      # read output in if required
+      unless options[:squelch]
+        output = wait_for_output(stdout)
+        Puppet.warning _("Could not get output") unless output
+      end
+
       if options[:failonfail] and exit_status != 0
         raise Puppet::ExecutionFailure, _("Execution of '%{str}' returned %{exit_status}: %{output}") % { str: command_str, exit_status: exit_status, output: output.strip }
       end
     ensure
-      # Make sure all handles are closed in case an exception was thrown attempting to execute.
-      [stdin, stdout, stderr].each {|io| io.close rescue nil}
-      if !options[:squelch] && reader
-        # if we opened a pipe, we need to clean it up.
-        reader.close
+      if !options[:squelch] && stdout
+        # if we opened a temp file for stdout, we need to clean it up.
+        stdout.close!
       end
     end
 
@@ -338,4 +325,35 @@ def self.execute_windows(command, options, stdin, stdout, stderr)
     end
   end
   private_class_method :execute_windows
+
+
+  # This is private method.
+  # @comment see call to private_class_method after method definition
+  # @api private
+  #
+  def self.wait_for_output(stdout)
+    # Make sure the file's actually been written.  This is basically a race
+    # condition, and is probably a horrible way to handle it, but, well, oh
+    # well.
+    # (If this method were treated as private / inaccessible from outside of this file, we shouldn't have to worry
+    #  about a race condition because all of the places that we call this from are preceded by a call to "waitpid2",
+    #  meaning that the processes responsible for writing the file have completed before we get here.)
+    2.times do |try|
+      if Puppet::FileSystem.exist?(stdout.path)
+        stdout.open
+        begin
+          return stdout.read
+        ensure
+          stdout.close
+          stdout.unlink
+        end
+      else
+        time_to_sleep = try / 2.0
+        Puppet.warning _("Waiting for output; will sleep %{time_to_sleep} seconds") % { time_to_sleep: time_to_sleep }
+        sleep(time_to_sleep)
+      end
+    end
+    nil
+  end
+  private_class_method :wait_for_output
 end

spec/unit/util/execution_spec.rb

@@ -29,7 +29,6 @@ def stub_process_wait(exitstatus)
       end
     end
 
-
     describe "#execute_posix (stubs)", :unless => Puppet.features.microsoft_windows? do
       before :each do
         # Most of the things this method does are bad to do during specs. :/
@@ -217,49 +216,78 @@ def stub_process_wait(exitstatus)
         end
 
         describe "when squelch is not set" do
-          it "should set stdout to a pipe" do
+          it "should set stdout to a temporary output file" do
+            outfile = Puppet::FileSystem::Uniquefile.new('stdout')
+            Puppet::FileSystem::Uniquefile.stubs(:new).returns(outfile)
+
             Puppet::Util::Execution.expects(executor).with do |_,_,_,stdout,_|
-              stdout.class == IO
+              stdout.path == outfile.path
             end.returns(rval)
 
             Puppet::Util::Execution.execute('test command', :squelch => false)
           end
 
           it "should set stderr to the same file as stdout if combine is true" do
+            outfile = Puppet::FileSystem::Uniquefile.new('stdout')
+            Puppet::FileSystem::Uniquefile.stubs(:new).returns(outfile)
+
             Puppet::Util::Execution.expects(executor).with do |_,_,_,stdout,stderr|
-              stdout == stderr
+              stdout.path == outfile.path and stderr.path == outfile.path
             end.returns(rval)
 
             Puppet::Util::Execution.execute('test command', :squelch => false, :combine => true)
           end
 
           it "should set stderr to the null device if combine is false" do
+            outfile = Puppet::FileSystem::Uniquefile.new('stdout')
+            Puppet::FileSystem::Uniquefile.stubs(:new).returns(outfile)
+
             Puppet::Util::Execution.expects(executor).with do |_,_,_,stdout,stderr|
-              stdout.class == IO and stderr.path == null_file
+              stdout.path == outfile.path and stderr.path == null_file
             end.returns(rval)
 
             Puppet::Util::Execution.execute('test command', :squelch => false, :combine => false)
           end
 
+          it "should combine stdout and stderr if combine is true" do
+            outfile = Puppet::FileSystem::Uniquefile.new('stdout')
+            Puppet::FileSystem::Uniquefile.stubs(:new).returns(outfile)
+
+            Puppet::Util::Execution.expects(executor).with do |_,_,_,stdout,stderr|
+              stdout.path == outfile.path and stderr.path == outfile.path
+            end.returns(rval)
+
+            Puppet::Util::Execution.execute('test command', :combine => true)
+          end
+
           it "should default combine to true when no options are specified" do
+            outfile = Puppet::FileSystem::Uniquefile.new('stdout')
+            Puppet::FileSystem::Uniquefile.stubs(:new).returns(outfile)
+
             Puppet::Util::Execution.expects(executor).with do |_,_,_,stdout,stderr|
-              stdout == stderr
+              stdout.path == outfile.path and stderr.path == outfile.path
             end.returns(rval)
 
             Puppet::Util::Execution.execute('test command')
           end
 
           it "should default combine to false when options are specified, but combine is not" do
+            outfile = Puppet::FileSystem::Uniquefile.new('stdout')
+            Puppet::FileSystem::Uniquefile.stubs(:new).returns(outfile)
+
             Puppet::Util::Execution.expects(executor).with do |_,_,_,stdout,stderr|
-              stdout.class == IO and stderr.path == null_file
+              stdout.path == outfile.path and stderr.path == null_file
             end.returns(rval)
 
             Puppet::Util::Execution.execute('test command', :failonfail => false)
           end
 
           it "should default combine to false when an empty hash of options is specified" do
+            outfile = Puppet::FileSystem::Uniquefile.new('stdout')
+            Puppet::FileSystem::Uniquefile.stubs(:new).returns(outfile)
+
             Puppet::Util::Execution.expects(executor).with do |_,_,_,stdout,stderr|
-              stdout.class == IO and stderr.path == null_file
+              stdout.path == outfile.path and stderr.path == null_file
             end.returns(rval)
 
             Puppet::Util::Execution.execute('test command', {})
@@ -508,10 +536,9 @@ def stub_process_wait(exitstatus)
       end
 
       it "should close the stdin/stdout/stderr files used by the child" do
-        stdin = mock 'file'
-        stdout = mock 'file'
-        stderr = mock 'file'
-        [stdin, stdout, stderr].each {|io| io.expects(:close).at_least_once}
+        stdin = mock 'file', :close
+        stdout = mock 'file', :close
+        stderr = mock 'file', :close
 
         File.expects(:open).
             times(3).
@@ -523,43 +550,37 @@ def stub_process_wait(exitstatus)
       end
 
       it "should read and return the output if squelch is false" do
-        r, w = IO.pipe
-        IO.expects(:pipe).returns([r, w])
-        w.write("My expected command output")
+        stdout = Puppet::FileSystem::Uniquefile.new('test')
+        Puppet::FileSystem::Uniquefile.stubs(:new).returns(stdout)
+        stdout.write("My expected command output")
 
         expect(Puppet::Util::Execution.execute('test command')).to eq("My expected command output")
       end
 
       it "should not read the output if squelch is true" do
-        IO.expects(:pipe).never
+        stdout = Puppet::FileSystem::Uniquefile.new('test')
+        Puppet::FileSystem::Uniquefile.stubs(:new).returns(stdout)
+        stdout.write("My expected command output")
 
         expect(Puppet::Util::Execution.execute('test command', :squelch => true)).to eq('')
       end
 
-      it "should close the pipe used for output if squelch is false" do
-        r, w = IO.pipe
-        IO.expects(:pipe).returns([r, w])
+      it "should delete the file used for output if squelch is false" do
+        stdout = Puppet::FileSystem::Uniquefile.new('test')
+        path = stdout.path
+        Puppet::FileSystem::Uniquefile.stubs(:new).returns(stdout)
+
+        Puppet::Util::Execution.execute('test command')
 
-        expect(Puppet::Util::Execution.execute('test command')).to eq("")
-        expect(r.closed?)
-        expect(w.closed?)
+        expect(Puppet::FileSystem.exist?(path)).to be_falsey
       end
 
-      it "should close the pipe used for output if squelch is false and an error is raised" do
-        r, w = IO.pipe
-        IO.expects(:pipe).returns([r, w])
+      it "should not raise an error if the file is open" do
+        stdout = Puppet::FileSystem::Uniquefile.new('test')
+        Puppet::FileSystem::Uniquefile.stubs(:new).returns(stdout)
+        file = File.new(stdout.path, 'r')
 
-        if Puppet.features.microsoft_windows?
-          Puppet::Util::Execution.expects(:execute_windows).raises(Exception, 'execution failed')
-        else
-          Puppet::Util::Execution.expects(:execute_posix).raises(Exception, 'execution failed')
-        end
-
-        expect {
-          subject.execute('fail command')
-        }.to raise_error(Exception, 'execution failed')
-        expect(r.closed?)
-        expect(w.closed?)
+        Puppet::Util::Execution.execute('test command')
       end
 
       it "should raise an error if failonfail is true and the child failed" do