diff options
| -rw-r--r-- | .rspec | 2 | ||||
| -rw-r--r-- | .travis.yml | 10 | ||||
| -rw-r--r-- | CHANGELOG.md | 12 | ||||
| -rw-r--r-- | CONTRIBUTIONS.md | 10 | ||||
| -rw-r--r-- | Gemfile | 3 | ||||
| -rw-r--r-- | ci/jenkins_run_tests.bat | 15 | ||||
| -rw-r--r-- | lib/mixlib/shellout.rb | 26 | ||||
| -rw-r--r-- | lib/mixlib/shellout/unix.rb | 79 | ||||
| -rw-r--r-- | lib/mixlib/shellout/version.rb | 2 | ||||
| -rw-r--r-- | lib/mixlib/shellout/windows.rb | 8 | ||||
| -rw-r--r-- | lib/mixlib/shellout/windows/core_ext.rb | 41 | ||||
| -rw-r--r-- | spec/mixlib/shellout_spec.rb | 142 |
12 files changed, 288 insertions, 62 deletions
@@ -1 +1 @@ --cbfs +-cfs diff --git a/.travis.yml b/.travis.yml new file mode 100644 index 0000000..4cc49e0 --- /dev/null +++ b/.travis.yml @@ -0,0 +1,10 @@ +rvm: + - 1.8.7 + - 1.9.3 + - 2.0.0 + +branches: + only: + - master + +script: bundle exec rspec --color --format progress diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..f4c1b75 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,12 @@ +# mixlib-shellout Changelog + +## Unreleased + +* Improved process cleanup on timeouts. +* Enabled travis. +* Added error? to check if the command ran successfully. MIXLIB-18. +* Remove GC.disable hack for non-ruby 1.8.8 +* Handle ESRCH from getpgid of a zombie on OS X +* Fix "TypeError: no implicit conversion from nil to integer" due to nil "token" passed to CloseHandle. MIXLIB-25. +* $stderr of the command process is now reflected in the live_stream in addition to $stdout. (MIXLIB-19) +## Last Release: 1.3.0 (12/03/2013) diff --git a/CONTRIBUTIONS.md b/CONTRIBUTIONS.md new file mode 100644 index 0000000..9356c4e --- /dev/null +++ b/CONTRIBUTIONS.md @@ -0,0 +1,10 @@ +<!--- +This file is reset every time a new release is done. The contents of this file are for the currently unreleased version. + +Example Contribution: +* **kalistec**: Improved file resource greatly. +--> +# mixlib-shellout Contributions: + +* **carmstrong**: Added error? method to see if the command was successful. +* **akshaykarle**: Added the functionality to reflect $stderr when using live_stream. @@ -1,4 +1,5 @@ -source :rubygems +source 'https://rubygems.org' + gemspec :name => "mixlib-shellout" group(:test) do diff --git a/ci/jenkins_run_tests.bat b/ci/jenkins_run_tests.bat index 4d1b1d1..9483952 100644 --- a/ci/jenkins_run_tests.bat +++ b/ci/jenkins_run_tests.bat @@ -1,9 +1,14 @@ -set PATH=C:\Ruby192\bin;%PATH% - ruby -v -call bundle install --binstubs --without docgen --path vendor/bundle || ( call rm Gemfile.lock && call bundle install --binstubs --path vendor/bundle ) -ruby bin\rspec -r rspec_junit_formatter -f RspecJunitFormatter -o test.xml -f documentation spec/functional spec/unit spec/stress -set RSPEC_ERRORLVL=%ERRORLEVEL% +call bundle check + +if %ERRORLEVEL% NEQ 0 ( + call rm Gemfile.lock + call bundle install --without docgen --path vendor/bundle +) + +bundle exec rspec -r rspec_junit_formatter -f RspecJunitFormatter -o test.xml -f documentation spec + +set RSPEC_ERRORLVL=%ERRORLEVEL% REM Return the error level from rspec exit /B %RSPEC_ERRORLVL% diff --git a/lib/mixlib/shellout.rb b/lib/mixlib/shellout.rb index 66faa40..e446448 100644 --- a/lib/mixlib/shellout.rb +++ b/lib/mixlib/shellout.rb @@ -49,13 +49,13 @@ module Mixlib # Working directory for the subprocess. Normally set via options to new attr_accessor :cwd - # An Array of acceptable exit codes. #error! uses this list to determine if - # the command was successful. Normally set via options to new + # An Array of acceptable exit codes. #error? (and #error!) use this list + # to determine if the command was successful. Normally set via options to new attr_accessor :valid_exit_codes - # When live_stream is set, stdout of the subprocess will be copied to it as - # the subprocess is running. For example, if live_stream is set to STDOUT, - # the command's output will be echoed to STDOUT. + # When live_stream is set, stdout and stderr of the subprocess will be + # copied to it as the subprocess is running. For example, if live_stream is + # set to STDOUT, the command's output will be echoed to STDOUT. attr_accessor :live_stream # ShellOut will push data from :input down the stdin of the subprocss. @@ -227,17 +227,21 @@ module Mixlib super end - # Checks the +exitstatus+ against the set of +valid_exit_codes+. If - # +exitstatus+ is not in the list of +valid_exit_codes+, calls +invalid!+, - # which raises an Exception. + # Checks the +exitstatus+ against the set of +valid_exit_codes+. + # === Returns + # +true+ if +exitstatus+ is not in the list of +valid_exit_codes+, false + # otherwise. + def error? + !Array(valid_exit_codes).include?(exitstatus) + end + + # If #error? is true, calls +invalid!+, which raises an Exception. # === Returns # nil::: always returns nil when it does not raise # === Raises # ::ShellCommandFailed::: via +invalid!+ def error! - unless Array(valid_exit_codes).include?(exitstatus) - invalid!("Expected process to exit with #{valid_exit_codes.inspect}, but received '#{exitstatus}'") - end + invalid!("Expected process to exit with #{valid_exit_codes.inspect}, but received '#{exitstatus}'") if error? end # Raises a ShellCommandFailed exception, appending the diff --git a/lib/mixlib/shellout/unix.rb b/lib/mixlib/shellout/unix.rb index 15ead60..b8f4a17 100644 --- a/lib/mixlib/shellout/unix.rb +++ b/lib/mixlib/shellout/unix.rb @@ -20,6 +20,11 @@ module Mixlib class ShellOut module Unix + # "1.8.7" as a frozen string. We use this with a hack that disables GC to + # avoid segfaults on Ruby 1.8.7, so we need to allocate the fewest + # objects we possibly can. + ONE_DOT_EIGHT_DOT_SEVEN = "1.8.7".freeze + # Option validation that is unix specific def validate_options(opts) # No options to validate, raise exceptions here if needed @@ -29,13 +34,20 @@ module Mixlib # to +stdout+ and +stderr+, and saving its exit status object to +status+ # === Returns # returns +self+; +stdout+, +stderr+, +status+, and +exitstatus+ will be - # populated with results of the command + # populated with results of the command. # === Raises # * Errno::EACCES when you are not privileged to execute the command # * Errno::ENOENT when the command is not available on the system (or not # in the current $PATH) # * Chef::Exceptions::CommandTimeout when the command does not complete - # within +timeout+ seconds (default: 600s) + # within +timeout+ seconds (default: 600s). When this happens, ShellOut + # will send a TERM and then KILL to the entire process group to ensure + # that any grandchild processes are terminated. If the invocation of + # the child process spawned multiple child processes (which commonly + # happens if the command is passed as a single string to be interpreted + # by bin/sh, and bin/sh is not bash), the exit status object may not + # contain the correct exit code of the process (of course there is no + # exit code if the command is killed by SIGKILL, also). def run_command @child_pid = fork_subprocess @reaped = false @@ -43,14 +55,15 @@ module Mixlib configure_parent_process_file_descriptors # Ruby 1.8.7 and 1.8.6 from mid 2009 try to allocate objects during GC - # when calling IO.select and IO#read. Some OS Vendors are not interested - # in updating their ruby packages (Apple, *cough*) and we *have to* - # make it work. So I give you this epic hack: - GC.disable + # when calling IO.select and IO#read. Disabling GC works around the + # segfault, but obviously it's a bad workaround. We no longer support + # 1.8.6 so we only need this hack for 1.8.7. + GC.disable if RUBY_VERSION == ONE_DOT_EIGHT_DOT_SEVEN # CHEF-3390: Marshall.load on Ruby < 1.8.7p369 also has a GC bug related # to Marshall.load, so try disabling GC first. propagate_pre_exec_failure + get_child_pgid @result = nil @execution_time = 0 @@ -94,6 +107,18 @@ module Mixlib private + def get_child_pgid + # The behavior of Process.getpgid (see also getpgid(2) ) when the + # argument is the pid of a zombie isn't well specified. On Linux it + # works, on OS X it returns ESRCH (which ruby turns into Errno::ESRCH). + # + # If the child dies very quickly, @child_pid may be a zombie, so handle + # ESRCH here. + @child_pgid = -Process.getpgid(@child_pid) + rescue Errno::ESRCH + @child_pgid = nil + end + def set_user if user Process.euid = uid @@ -122,6 +147,16 @@ module Mixlib Dir.chdir(cwd) if cwd end + # Process group id of the child. Returned as a negative value so you can + # put it directly in arguments to kill, wait, etc. + # + # This may be nil if the child dies before the parent can query the + # system for its pgid (on some systems it is an error to get the pgid of + # a zombie). + def child_pgid + @child_pgid + end + def initialize_ipc @stdin_pipe, @stdout_pipe, @stderr_pipe, @process_status_pipe = IO.pipe, IO.pipe, IO.pipe, IO.pipe @process_status_pipe.last.fcntl(Fcntl::F_SETFD, Fcntl::FD_CLOEXEC) @@ -253,6 +288,7 @@ module Mixlib def read_stderr_to_buffer while chunk = child_stderr.read_nonblock(READ_SIZE) @stderr << chunk + @live_stream << chunk if @live_stream end rescue Errno::EAGAIN rescue EOFError @@ -263,6 +299,14 @@ module Mixlib initialize_ipc fork do + # Child processes may themselves fork off children. A common case + # is when the command is given as a single string (instead of + # command name plus Array of arguments) and /bin/sh does not + # support the "ONESHOT" optimization (where sh -c does exec without + # forking). To support cleaning up all the children, we need to + # ensure they're in a unique process group. + Process.setsid + configure_subprocess_file_descriptors clean_parent_file_descriptors @@ -302,14 +346,13 @@ module Mixlib def reap_errant_child return if attempt_reap - @terminate_reason = "Command execeded allowed execution time, killed by TERM signal." - logger.error("Command execeded allowed execution time, sending TERM") if logger - Process.kill(:TERM, @child_pid) + @terminate_reason = "Command exceeded allowed execution time, process terminated" + logger.error("Command exceeded allowed execution time, sending TERM") if logger + Process.kill(:TERM, child_pgid) sleep 3 - return if attempt_reap - @terminate_reason = "Command execeded allowed execution time, did not respond to TERM. Killed by KILL signal." - logger.error("Command did not exit from TERM, sending KILL") if logger - Process.kill(:KILL, @child_pid) + attempt_reap + logger.error("Command exceeded allowed execution time, sending KILL") if logger + Process.kill(:KILL, child_pgid) reap # Should not hit this but it's possible if something is calling waitall @@ -323,12 +366,22 @@ module Mixlib @child_pid && !@reaped end + # Unconditionally reap the child process. This is used in scenarios where + # we can be confident the child will exit quickly, and has not spawned + # and grandchild processes. def reap results = Process.waitpid2(@child_pid) @reaped = true @status = results.last + rescue Errno::ECHILD + # When cleaning up timed-out processes, we might send SIGKILL to the + # whole process group after we've cleaned up the direct child. In that + # case the grandchildren will have been adopted by init so we can't + # reap them even if we wanted to (we don't). + nil end + # Try to reap the child process but don't block if it isn't dead yet. def attempt_reap if results = Process.waitpid2(@child_pid, Process::WNOHANG) @reaped = true diff --git a/lib/mixlib/shellout/version.rb b/lib/mixlib/shellout/version.rb index aef17a3..6c71061 100644 --- a/lib/mixlib/shellout/version.rb +++ b/lib/mixlib/shellout/version.rb @@ -1,5 +1,5 @@ module Mixlib class ShellOut - VERSION = "1.3.0" + VERSION = "1.6.0.alpha.0" end end diff --git a/lib/mixlib/shellout/windows.rb b/lib/mixlib/shellout/windows.rb index ea809b5..832584a 100644 --- a/lib/mixlib/shellout/windows.rb +++ b/lib/mixlib/shellout/windows.rb @@ -122,8 +122,8 @@ module Mixlib end ensure - CloseHandle(process.thread_handle) - CloseHandle(process.process_handle) + CloseHandle(process.thread_handle) if process.thread_handle + CloseHandle(process.process_handle) if process.process_handle end ensure @@ -165,7 +165,9 @@ module Mixlib if ready.first.include?(stderr_read) begin - @stderr << stderr_read.readpartial(READ_SIZE) + next_chunk = stderr_read.readpartial(READ_SIZE) + @stderr << next_chunk + @live_stream << next_chunk if @live_stream rescue EOFError stderr_read.close open_streams.delete(stderr_read) diff --git a/lib/mixlib/shellout/windows/core_ext.rb b/lib/mixlib/shellout/windows/core_ext.rb index e692c7d..73b848f 100644 --- a/lib/mixlib/shellout/windows/core_ext.rb +++ b/lib/mixlib/shellout/windows/core_ext.rb @@ -288,19 +288,23 @@ module Process token = token.read_ulong - bool = CreateProcessAsUserW( - token, # User token handle - app, # App name - cmd, # Command line - process_security, # Process attributes - thread_security, # Thread attributes - inherit, # Inherit handles - hash['creation_flags'], # Creation Flags - env, # Environment - cwd, # Working directory - startinfo, # Startup Info - procinfo # Process Info - ) + begin + bool = CreateProcessAsUserW( + token, # User token handle + app, # App name + cmd, # Command line + process_security, # Process attributes + thread_security, # Thread attributes + inherit, # Inherit handles + hash['creation_flags'], # Creation Flags + env, # Environment + cwd, # Working directory + startinfo, # Startup Info + procinfo # Process Info + ) + ensure + CloseHandle(token) + end unless bool raise SystemCallError.new("CreateProcessAsUserW (You must hold the 'Replace a process level token' permission)", FFI.errno) @@ -346,9 +350,14 @@ module Process # Automatically close the process and thread handles in the # PROCESS_INFORMATION struct unless explicitly told not to. if hash['close_handles'] - CloseHandle(procinfo[:hProcess]) - CloseHandle(procinfo[:hThread]) - CloseHandle(token) + CloseHandle(procinfo[:hProcess]) if procinfo[:hProcess] + CloseHandle(procinfo[:hThread]) if procinfo[:hThread] + + # Set fields to nil so callers don't attempt to close the handle + # which can result in the wrong handle being closed or an + # exception in some circumstances + procinfo[:hProcess] = nil + procinfo[:hThread] = nil end ProcessInfo.new( diff --git a/spec/mixlib/shellout_spec.rb b/spec/mixlib/shellout_spec.rb index 44f29d7..5dcb6a7 100644 --- a/spec/mixlib/shellout_spec.rb +++ b/spec/mixlib/shellout_spec.rb @@ -449,12 +449,17 @@ describe Mixlib::ShellOut do context "with a live stream" do let(:stream) { StringIO.new } - let(:ruby_code) { 'puts "hello"' } + let(:ruby_code) { '$stdout.puts "hello"; $stderr.puts "world"' } let(:options) { { :live_stream => stream } } it "should copy the child's stdout to the live stream" do shell_cmd.run_command - stream.string.should eql("hello#{LINE_ENDING}") + stream.string.should include("hello#{LINE_ENDING}") + end + + it "should copy the child's stderr to the live stream" do + shell_cmd.run_command + stream.string.should include("world#{LINE_ENDING}") end end @@ -752,6 +757,24 @@ describe Mixlib::ShellOut do lambda { executed_cmd.invalid!("I expected this to exit 42, not 0") }.should raise_error(Mixlib::ShellOut::ShellCommandFailed) end end + + describe "#error?" do + context 'when exiting with invalid code' do + let(:exit_code) { 2 } + + it "should return true" do + executed_cmd.error?.should be_true + end + end + + context 'when exiting with valid code' do + let(:exit_code) { 0 } + + it "should return false" do + executed_cmd.error?.should be_false + end + end + end end context "when handling the subprocess" do @@ -779,7 +802,7 @@ describe Mixlib::ShellOut do end end - context "when running a command that doesn't exist" do + context "when running a command that doesn't exist", :unix_only do let(:cmd) { "/bin/this-is-not-a-real-command" } @@ -827,9 +850,35 @@ describe Mixlib::ShellOut do end end - context 'with subprocess that takes longer than timeout' do + context "when the child process dies immediately" do + let(:cmd) { [ 'exit' ] } + + it "handles ESRCH from getpgid of a zombie", :unix_only do + Process.stub(:setsid) { exit!(4) } + + # there is a small race condition here if the child doesn't get + # scheduled and call exit! before the parent can call getpgid, so run + # this a few times to make sure we've created the reproduction case + # correctly. + 5.times do + s = Mixlib::ShellOut.new(cmd) + s.run_command # should not raise Errno::ESRCH + end + + end + + end + + context 'with subprocess that takes longer than timeout', :unix_only do + def ruby_wo_shell(code) + parts = %w[ruby] + parts << "--disable-gems" if ruby_19? + parts << "-e" + parts << code + end + let(:cmd) do - ruby_eval.call(<<-CODE) + ruby_wo_shell(<<-CODE) STDOUT.sync = true trap(:TERM) { puts "got term"; exit!(123) } sleep 10 @@ -851,7 +900,7 @@ describe Mixlib::ShellOut do context "and the child is unresponsive" do let(:cmd) do - ruby_eval.call(<<-CODE) + ruby_wo_shell(<<-CODE) STDOUT.sync = true trap(:TERM) { puts "nanana cant hear you" } sleep 10 @@ -878,12 +927,83 @@ describe Mixlib::ShellOut do shell_cmd.stdout.should include("nanana cant hear you") shell_cmd.status.termsig.should == 9 - log_output.string.should include("Command execeded allowed execution time, sending TERM") - log_output.string.should include("Command did not exit from TERM, sending KILL") + log_output.string.should include("Command exceeded allowed execution time, sending TERM") + log_output.string.should include("Command exceeded allowed execution time, sending KILL") end end end + + context "and the child process forks grandchildren" do + let(:cmd) do + ruby_wo_shell(<<-CODE) + STDOUT.sync = true + trap(:TERM) { print "got term in child\n"; exit!(123) } + fork do + trap(:TERM) { print "got term in grandchild\n"; exit!(142) } + sleep 10 + end + sleep 10 + CODE + end + + it "should TERM the wayward child and grandchild" do + # 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 in child") + shell_cmd.stdout.should include("got term in grandchild") + end + + end + context "and the child process forks grandchildren that don't respond to TERM" do + let(:cmd) do + ruby_wo_shell(<<-CODE) + STDOUT.sync = true + + trap(:TERM) { print "got term in child\n"; exit!(123) } + fork do + trap(:TERM) { print "got term in grandchild\n" } + sleep 10 + end + sleep 10 + CODE + end + + + it "should TERM the wayward child and grandchild, then KILL whoever is left" do + # 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) + + begin + + # A little janky. We get the process group id out of the command + # object, then try to kill a process in it to make sure none + # exists. Trusting the system under test like this isn't great but + # it's difficult to test otherwise. + child_pgid = shell_cmd.send(:child_pgid) + initial_process_listing = `ps -j` + + shell_cmd.stdout.should include("got term in child") + shell_cmd.stdout.should include("got term in grandchild") + + Process.kill(:INT, child_pgid) # should raise ESRCH + + # Debug the failure: + puts "child pgid=#{child_pgid.inspect}" + Process.wait + puts "collected process: #{$?.inspect}" + puts "initial process listing:\n#{initial_process_listing}" + puts "current process listing:" + puts `ps -j` + raise "Failed to kill all expected processes" + rescue Errno::ESRCH + # this is what we want + end + end + + end end context 'with subprocess that exceeds buffersize' do @@ -910,7 +1030,7 @@ describe Mixlib::ShellOut do let(:ruby_code) { "STDIN.close; sleep 0.5; STDOUT.puts :win" } let(:options) { { :input => "Random data #{rand(100000)}" } } - it 'should not hang or lose outupt' do + it 'should not hang or lose output' do stdout.should eql("win#{LINE_ENDING}") end end @@ -918,7 +1038,7 @@ describe Mixlib::ShellOut do context 'with subprocess that closes stdout and continues writing to stderr' do let(:ruby_code) { "STDOUT.close; sleep 0.5; STDERR.puts :win" } - it 'should not hang or lose outupt' do + it 'should not hang or lose output' do stderr.should eql("win#{LINE_ENDING}") end end @@ -926,7 +1046,7 @@ describe Mixlib::ShellOut do context 'with subprocess that closes stderr and continues writing to stdout' do let(:ruby_code) { "STDERR.close; sleep 0.5; STDOUT.puts :win" } - it 'should not hang or lose outupt' do + it 'should not hang or lose output' do stdout.should eql("win#{LINE_ENDING}") end end |