Merge branch 'MIXLIB-16'

11 files changed, 124 insertions, 104 deletions

diff --git a/.rspec b/.rspec
index eca491f..7060880 100644
--- a/.rspec
+++ b/.rspec
@@ -1 +1 @@
--cbfp
+-cbfs
diff --git a/Gemfile b/Gemfile
index b1dd8e0..c1b379a 100644
--- a/Gemfile
+++ b/Gemfile
@@ -8,8 +8,3 @@ group(:test) do
 
 end
 
-group(:kitchen) do
-
-  gem 'test-kitchen', '>= 0.7.0.beta.1'
-
-end
diff --git a/lib/mixlib/shellout.rb b/lib/mixlib/shellout.rb
index a267162..66faa40 100644
--- a/lib/mixlib/shellout.rb
+++ b/lib/mixlib/shellout.rb
@@ -154,6 +154,7 @@ module Mixlib
       @environment = DEFAULT_ENVIRONMENT
       @cwd = nil
       @valid_exit_codes = [0]
+      @terminate_reason = nil
 
       if command_args.last.is_a?(Hash)
         parse_options(command_args.pop)
@@ -191,6 +192,7 @@ module Mixlib
     # results when the command exited with an unexpected status.
     def format_for_exception
       msg = ""
+      msg << "#{@terminate_reason}\n" if @terminate_reason
       msg << "---- Begin output of #{command} ----\n"
       msg << "STDOUT: #{stdout.strip}\n"
       msg << "STDERR: #{stderr.strip}\n"
diff --git a/lib/mixlib/shellout/unix.rb b/lib/mixlib/shellout/unix.rb
index d7b8b92..15ead60 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
 
@@ -57,44 +58,33 @@ module Mixlib
         write_to_child_stdin
 
         until @status
-          ready = IO.select(open_pipes, nil, nil, READ_WAIT_TIME)
-          unless ready
+          ready_buffers = attempt_buffer_read
+          unless ready_buffers
             @execution_time += READ_WAIT_TIME
             if @execution_time >= timeout && !@result
-              raise CommandTimeout, "command timed out:\n#{format_for_exception}"
+              # kill the bad proccess
+              reap_errant_child
+              # read anything it wrote when we killed it
+              attempt_buffer_read
+              # raise
+              raise CommandTimeout, "Command timed out after #{@execution_time.to_i}s:\n#{format_for_exception}"
             end
           end
 
-          if ready && ready.first.include?(child_stdout)
-            read_stdout_to_buffer
-          end
-          if ready && ready.first.include?(child_stderr)
-            read_stderr_to_buffer
-          end
-
-          unless @status
-            # make one more pass to get the last of the output after the
-            # child process dies
-            if results = Process.waitpid2(@child_pid, Process::WNOHANG)
-              @status = results.last
-              redo
-            end
-          end
+          attempt_reap
         end
+
         self
       rescue Errno::ENOENT
         # When ENOENT happens, we can be reasonably sure that the child process
         # is going to exit quickly, so we use the blocking variant of waitpid2
-        Process.waitpid2(@child_pid) rescue nil
-        raise
-      rescue Exception
-        # For exceptions other than ENOENT, such as timeout, we can't be sure
-        # how long the child process will live, so we use the non-blocking
-        # variant of waitpid2. This can result in zombie processes when the
-        # child later dies. See MIXLIB-16 for proposed enhancement.
-        Process.waitpid2(@child_pid, Process::WNOHANG) rescue nil
+        reap
         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.
@@ -239,6 +229,17 @@ module Mixlib
         child_stdin.close # Kick things off
       end
 
+      def attempt_buffer_read
+        ready = IO.select(open_pipes, nil, nil, READ_WAIT_TIME)
+        if ready && ready.first.include?(child_stdout)
+          read_stdout_to_buffer
+        end
+        if ready && ready.first.include?(child_stderr)
+          read_stderr_to_buffer
+        end
+        ready
+      end
+
       def read_stdout_to_buffer
         while chunk = child_stdout.read_nonblock(READ_SIZE)
           @stdout << chunk
@@ -299,6 +300,44 @@ module Mixlib
         end
       end
 
+      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)
+        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)
+        reap
+
+        # Should not hit this but it's possible if something is calling waitall
+        # in a separate thread.
+      rescue Errno::ESRCH
+        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
+        end
+      end
+
     end
   end
 end
diff --git a/mixlib-shellout.gemspec b/mixlib-shellout.gemspec
index cf72f12..f74f15f 100644
--- a/mixlib-shellout.gemspec
+++ b/mixlib-shellout.gemspec
@@ -13,7 +13,7 @@ Gem::Specification.new do |s|
   s.homepage = "http://wiki.opscode.com/"
 
 
-  %w(rspec).each { |gem| s.add_development_dependency gem }
+  s.add_development_dependency "rspec", "~> 2.0"
 
   s.bindir       = "bin"
   s.executables  = []
diff --git a/spec/mixlib/shellout_spec.rb b/spec/mixlib/shellout_spec.rb
index 084e1b2..71ecfef 100644
--- a/spec/mixlib/shellout_spec.rb
+++ b/spec/mixlib/shellout_spec.rb
@@ -1,4 +1,5 @@
 require 'spec_helper'
+require 'logger'
 
 describe Mixlib::ShellOut do
   let(:shell_cmd) { options ? shell_cmd_with_options : shell_cmd_without_options }
@@ -825,12 +826,62 @@ describe Mixlib::ShellOut do
       end
 
       context 'with subprocess that takes longer than timeout' do
-        let(:cmd) { ruby_eval.call('sleep 2') }
-        let(:options) { { :timeout => 0.1 } }
+        let(:cmd) do
+          ruby_eval.call(<<-CODE)
+            STDOUT.sync = true
+            trap(:TERM) { puts "got term"; exit!(123) }
+            sleep 10
+          CODE
+        end
+        let(:options) { { :timeout => 1 } }
 
         it "should raise CommandTimeout" do
           lambda { executed_cmd }.should raise_error(Mixlib::ShellOut::CommandTimeout)
         end
+
+        it "should ask the process nicely to exit" 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")
+          shell_cmd.exitstatus.should == 123
+        end
+
+        context "and the child is unresponsive" do
+          let(:cmd) do
+            ruby_eval.call(<<-CODE)
+              STDOUT.sync = true
+              trap(:TERM) { puts "nanana cant hear you" }
+              sleep 10
+            CODE
+          end
+
+          it "should KILL the wayward child" 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("nanana cant hear you")
+            shell_cmd.status.termsig.should == 9
+          end
+
+          context "and a logger is configured" do
+            let(:log_output) { StringIO.new }
+            let(:logger) { Logger.new(log_output) }
+            let(:options) { {:timeout => 1, :logger => logger} }
+
+            it "should log messages about killing the child process" 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("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")
+            end
+
+          end
+        end
       end
 
       context 'with subprocess that exceeds buffersize' do
@@ -1064,8 +1115,8 @@ describe Mixlib::ShellOut do
       # test for for_fd returning a valid File object, but close
       # throwing EBADF.
       it "should not throw an exception if fd.close throws EBADF" do
-        fd = mock('File')
-        fd.stub!(:close).at_least(:once).and_raise(Errno::EBADF)
+        fd = double('File')
+        fd.stub(:close).at_least(:once).and_raise(Errno::EBADF)
         File.should_receive(:for_fd).at_least(:once).and_return(fd)
         shellout = Mixlib::ShellOut.new()
         shellout.instance_variable_set(:@process_status_pipe, [ 98, 99 ])
diff --git a/test/kitchen/Kitchenfile b/test/kitchen/Kitchenfile
deleted file mode 100644
index 60afd49..0000000
--- a/test/kitchen/Kitchenfile
+++ /dev/null
@@ -1,26 +0,0 @@
-#
-# Author:: Seth Chisamore (<schisamo@opscode.com>)
-# Copyright:: Copyright (c) 2012, Opscode, Inc. (<legal@opscode.com>)
-#
-# License:: Apache License, Version 2.0
-#
-# Licensed under the Apache License, Version 2.0 (the "License");
-# you may not use this file except in compliance with the License.
-# You may obtain a copy of the License at
-#
-#    http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an "AS IS" BASIS,
-# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-# See the License for the specific language governing permissions and
-# limitations under the License.
-#
-
-integration_test "mixlib-shellout" do
-  language 'ruby'
-  runtimes ['1.9.3']
-  install 'bundle install --without kitchen'
-  script 'bundle exec rspec spec'
-  run_list_extras ['mixlib-shellout_test::default']
-end
diff --git a/test/kitchen/cookbooks/Cheffile b/test/kitchen/cookbooks/Cheffile
deleted file mode 100644
index 5313b1d..0000000
--- a/test/kitchen/cookbooks/Cheffile
+++ /dev/null
@@ -1,3 +0,0 @@
-cookbook 'rvm',
-:git => 'https://github.com/fnichol/chef-rvm.git',
-:ref => 'd6cdb2ab93c421ca6234cbaf5548c914eba4084f'
diff --git a/test/kitchen/cookbooks/mixlib-shellout_test/README.md b/test/kitchen/cookbooks/mixlib-shellout_test/README.md
deleted file mode 100644
index dd93d60..0000000
--- a/test/kitchen/cookbooks/mixlib-shellout_test/README.md
+++ /dev/null
@@ -1 +0,0 @@
-This cookbook is used with test-kitchen to test the parent project, mixlib-shellout.
diff --git a/test/kitchen/cookbooks/mixlib-shellout_test/metadata.rb b/test/kitchen/cookbooks/mixlib-shellout_test/metadata.rb
deleted file mode 100644
index b44b488..0000000
--- a/test/kitchen/cookbooks/mixlib-shellout_test/metadata.rb
+++ /dev/null
@@ -1,5 +0,0 @@
-maintainer       "Opscode, Inc."
-maintainer_email "cookbooks@opscode.com"
-license          "Apache 2.0"
-description      "This cookbook is used with test-kitchen to test the parent project, mixlib-shellout."
-version          "1.0.0"
diff --git a/test/kitchen/cookbooks/mixlib-shellout_test/recipes/default.rb b/test/kitchen/cookbooks/mixlib-shellout_test/recipes/default.rb
deleted file mode 100644
index fa78b4a..0000000
--- a/test/kitchen/cookbooks/mixlib-shellout_test/recipes/default.rb
+++ /dev/null
@@ -1,32 +0,0 @@
-#
-# Cookbook Name:: mixlib-shellout_test
-# Recipe:: default
-#
-# Copyright 2012, Opscode, Inc.
-#
-# Licensed under the Apache License, Version 2.0 (the "License");
-# you may not use this file except in compliance with the License.
-# You may obtain a copy of the License at
-#
-#    http://www.apache.org/licenses/LICENSE-2.0
-#
-# Unless required by applicable law or agreed to in writing, software
-# distributed under the License is distributed on an "AS IS" BASIS,
-# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-# See the License for the specific language governing permissions and
-# limitations under the License.
-#
-
-node.set['rvm']['user_installs'] = [
-  { 'user'          => 'vagrant',
-    'default_ruby'  => 'ruby-1.9.3-p327',
-    'rubies'        => ['1.9.3']
-  }
-]
-
-node.set['rvm']['gems'] = {
-  "ruby-1.9.3-p327" => [
-    { 'name' => 'bundler' }
-  ]
-}
-include_recipe "rvm::user"