diff --git a/acceptance/tests/resource/exec/should_accept_large_output.rb b/acceptance/tests/resource/exec/should_accept_large_output.rb deleted file mode 100644 index 28e8ee58e20..00000000000 --- a/acceptance/tests/resource/exec/should_accept_large_output.rb +++ /dev/null @@ -1,26 +0,0 @@ -test_name "tests that puppet correctly captures large and empty output." - -agents.each do |agent| - testfile = agent.tmpfile('should_accept_large_output') - - # Generate >64KB file to exceed pipe buffer. - lorem_ipsum = < ['/bin', '/usr/bin', 'C:/cygwin32/bin', 'C:/cygwin64/bin'], logoutput => true}") do - fail_test "didn't seem to run the command" unless - stdout.include? 'executed successfully' - fail_test "didn't print output correctly" unless - stdout.lines.select {|line| line =~ /\/returns:/}.count == 4097 - end - - apply_manifest_on(agent, "exec {'echo': path => ['/bin', '/usr/bin', 'C:/cygwin32/bin', 'C:/cygwin64/bin'], logoutput => true}") do - fail_test "didn't seem to run the command" unless - stdout.include? 'executed successfully' - end -end diff --git a/lib/puppet/util/execution.rb b/lib/puppet/util/execution.rb index 6646d7d06d2..d5526f18853 100644 --- a/lib/puppet/util/execution.rb +++ b/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,12 +216,6 @@ 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) @@ -242,15 +223,21 @@ def self.execute(command, options = NoOptionsSpecified) 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 diff --git a/spec/unit/util/execution_spec.rb b/spec/unit/util/execution_spec.rb index 736f6d8f1e2..0cb581d8f64 100644 --- a/spec/unit/util/execution_spec.rb +++ b/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