[Pkg-puppet-devel] [facter] 266/352: (FACT-327) Move Process.waitall to Execution.exec

[Stig Sandbeck Mathisen]

This is an automated email from the git hooks/post-receive script.

ssm pushed a commit to branch master
in repository facter.

commit a7f4cb6da3bb866c53b9d33b3ae8ec6430b2723c
Author: Adrien Thebo <git at somethingsinistral.net>
Date:   Mon Feb 10 11:59:42 2014 -0800

    (FACT-327) Move Process.waitall to Execution.exec
    
    Commit d24504e8 added functionality for waiting on child processes if we
    aborted command execution early, but this only really applies to command
    execution. This commit relocates the waitall functionality to
    Execution.exec so it's clearer what we're waiting on and why.
---
 lib/facter/core/execution.rb      | 20 +++++++++++++++-
 lib/facter/core/resolvable.rb     |  5 ----
 spec/unit/core/execution_spec.rb  | 48 +++++++++++++++++++++++++++++++++++++++
 spec/unit/core/resolvable_spec.rb | 11 ---------
 4 files changed, 67 insertions(+), 17 deletions(-)

diff --git a/lib/facter/core/execution.rb b/lib/facter/core/execution.rb
index bac2be9..87b6cf6 100644
--- a/lib/facter/core/execution.rb
+++ b/lib/facter/core/execution.rb
@@ -171,10 +171,28 @@ module Facter
           out = ''
 
           begin
+            wait_for_child = true
             out = %x{#{code}}.chomp
+            wait_for_child = false
           rescue => detail
-            Facter.warn(detail)
+            Facter.warn(detail.message)
             return ''
+          ensure
+            if wait_for_child
+              # We need to ensure that if this code exits early then any spawned
+              # children will be reaped. Process execution is frequently
+              # terminated using Timeout.timeout but since the timeout isn't in
+              # this scope we can't rescue the raised exception. The best that
+              # we can do is determine if the child has exited, and if it hasn't
+              # then we need to spawn a thread to wait for the child.
+              #
+              # Due to the limitations of Ruby 1.8 there aren't good ways to
+              # asynchronously run a command and grab the PID of that command
+              # using the standard library. The best we can do is blindly wait
+              # on all processes and hope for the best. This issue is described
+              # at https://tickets.puppetlabs.com/browse/FACT-150
+              Thread.new { Process.waitall }
+            end
           end
 
           out
diff --git a/lib/facter/core/resolvable.rb b/lib/facter/core/resolvable.rb
index 84459a5..46bbad4 100644
--- a/lib/facter/core/resolvable.rb
+++ b/lib/facter/core/resolvable.rb
@@ -67,11 +67,6 @@ module Facter::Core::Resolvable
     Facter::Util::Normalization.normalize(result)
   rescue Timeout::Error => detail
     Facter.warn "Timed out after #{limit} seconds while resolving #{qualified_name}"
-
-    # This call avoids zombies -- basically, create a thread that will
-    # dezombify all of the child processes that we're ignoring because
-    # of the timeout.
-    Thread.new { Process.waitall }
     return nil
   rescue Facter::Util::Normalization::NormalizationError => e
     Facter.warn "Fact resolution #{qualified_name} resolved to an invalid value: #{e.message}"
diff --git a/spec/unit/core/execution_spec.rb b/spec/unit/core/execution_spec.rb
index c90df48..bf8d925 100644
--- a/spec/unit/core/execution_spec.rb
+++ b/spec/unit/core/execution_spec.rb
@@ -255,4 +255,52 @@ describe Facter::Core::Execution do
     end
   end
 
+
+  describe "#exec" do
+
+    it "switches LANG to C when executing the command" do
+      described_class.expects(:with_env).with('LANG' => 'C')
+      described_class.exec('foo')
+    end
+
+    it "switches LC_ALL to C when executing the command"
+
+    it "expands the command before running it" do
+      described_class.stubs(:`).returns ''
+      described_class.expects(:expand_command).with('foo').returns '/bin/foo'
+      described_class.exec('foo')
+    end
+
+    it "returns an empty string when the command could not be expanded" do
+      described_class.expects(:expand_command).with('foo').returns nil
+      expect(described_class.exec('foo')).to be_empty
+    end
+
+    it "logs a warning and returns an empty string when the command execution fails" do
+      described_class.expects(:`).with("/bin/foo").raises "kaboom!"
+      Facter.expects(:warn).with("kaboom!")
+
+      described_class.expects(:expand_command).with('foo').returns '/bin/foo'
+
+      expect(described_class.exec("foo")).to be_empty
+    end
+
+    it "launches a thread to wait on children if the command was interrupted" do
+      described_class.expects(:`).with("/bin/foo").raises "kaboom!"
+      described_class.expects(:expand_command).with('foo').returns '/bin/foo'
+
+      Facter.stubs(:warn)
+      Thread.expects(:new).yields
+      Process.expects(:waitall).once
+
+      described_class.exec("foo")
+    end
+
+    it "returns the output of the command" do
+      described_class.expects(:`).with("/bin/foo").returns 'hi'
+      described_class.expects(:expand_command).with('foo').returns '/bin/foo'
+
+      expect(described_class.exec("foo")).to eq 'hi'
+    end
+  end
 end
diff --git a/spec/unit/core/resolvable_spec.rb b/spec/unit/core/resolvable_spec.rb
index 1b877f0..b7a2e0c 100644
--- a/spec/unit/core/resolvable_spec.rb
+++ b/spec/unit/core/resolvable_spec.rb
@@ -60,17 +60,6 @@ describe Facter::Core::Resolvable do
 
       expect(subject.value).to be_nil
     end
-
-
-    it "starts a thread to wait on all child processes if the timeout was reached" do
-      Thread.expects(:new).yields
-      Process.expects(:waitall)
-
-      Facter.stubs(:warn)
-      Timeout.expects(:timeout).raises Timeout::Error
-
-      subject.value
-    end
   end
 
   describe 'callbacks when flushing facts' do

-- 
Alioth's /usr/local/bin/git-commit-notice on /srv/git.debian.org/git/pkg-puppet/facter.git