diff --git a/acceptance/tests/resource/exec/should_accept_large_output.rb b/acceptance/tests/resource/exec/should_accept_large_output.rb new file mode 100644 index 00000000000..28e8ee58e20 --- /dev/null +++ b/acceptance/tests/resource/exec/should_accept_large_output.rb @@ -0,0 +1,26 @@ +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 d5526f18853..6646d7d06d2 100644 --- a/lib/puppet/util/execution.rb +++ b/lib/puppet/util/execution.rb @@ -187,18 +187,31 @@ 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') : Puppet::FileSystem::Uniquefile.new('puppet') + stdout = options[:squelch] ? Puppet::FileSystem.open(null_file, nil, 'w') : writer 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 - return execution_stub.call(*exec_args) + child_pid = execution_stub.call(*exec_args) + [stdin, stdout, stderr].each {|io| io.close rescue nil} + return child_pid 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 @@ -216,6 +229,12 @@ 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) @@ -223,21 +242,15 @@ 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 - if !options[:squelch] && stdout - # if we opened a temp file for stdout, we need to clean it up. - stdout.close! + # 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 end end @@ -325,35 +338,4 @@ 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 0cb581d8f64..736f6d8f1e2 100644 --- a/spec/unit/util/execution_spec.rb +++ b/spec/unit/util/execution_spec.rb @@ -29,6 +29,7 @@ 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. :/ @@ -216,78 +217,49 @@ def stub_process_wait(exitstatus) end describe "when squelch is not set" do - it "should set stdout to a temporary output file" do - outfile = Puppet::FileSystem::Uniquefile.new('stdout') - Puppet::FileSystem::Uniquefile.stubs(:new).returns(outfile) - + it "should set stdout to a pipe" do Puppet::Util::Execution.expects(executor).with do |_,_,_,stdout,_| - stdout.path == outfile.path + stdout.class == IO 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.path == outfile.path and stderr.path == outfile.path + stdout == stderr 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.path == outfile.path and stderr.path == null_file + stdout.class == IO 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.path == outfile.path and stderr.path == outfile.path + stdout == stderr 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.path == outfile.path and stderr.path == null_file + stdout.class == IO 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.path == outfile.path and stderr.path == null_file + stdout.class == IO and stderr.path == null_file end.returns(rval) Puppet::Util::Execution.execute('test command', {}) @@ -536,9 +508,10 @@ def stub_process_wait(exitstatus) end it "should close the stdin/stdout/stderr files used by the child" do - stdin = mock 'file', :close - stdout = mock 'file', :close - stderr = mock 'file', :close + stdin = mock 'file' + stdout = mock 'file' + stderr = mock 'file' + [stdin, stdout, stderr].each {|io| io.expects(:close).at_least_once} File.expects(:open). times(3). @@ -550,37 +523,43 @@ def stub_process_wait(exitstatus) end it "should read and return the output if squelch is false" do - stdout = Puppet::FileSystem::Uniquefile.new('test') - Puppet::FileSystem::Uniquefile.stubs(:new).returns(stdout) - stdout.write("My expected command output") + r, w = IO.pipe + IO.expects(:pipe).returns([r, w]) + w.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 - stdout = Puppet::FileSystem::Uniquefile.new('test') - Puppet::FileSystem::Uniquefile.stubs(:new).returns(stdout) - stdout.write("My expected command output") + IO.expects(:pipe).never expect(Puppet::Util::Execution.execute('test command', :squelch => true)).to eq('') end - 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') + it "should close the pipe used for output if squelch is false" do + r, w = IO.pipe + IO.expects(:pipe).returns([r, w]) - expect(Puppet::FileSystem.exist?(path)).to be_falsey + expect(Puppet::Util::Execution.execute('test command')).to eq("") + expect(r.closed?) + expect(w.closed?) end - 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') + 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]) - Puppet::Util::Execution.execute('test command') + 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?) end it "should raise an error if failonfail is true and the child failed" do