Bugfix/facter 2/fact 150 process waitall

lib/facter/core/execution.rb

@@ -4,6 +4,20 @@ module Facter
   module Core
     module Execution
 
+      require 'facter/core/execution/base'
+      require 'facter/core/execution/ruby18'
+      require 'facter/core/execution/ruby19'
+
+      @@impl = if RUBY_VERSION.match /^1\.8/
+                 Facter::Core::Execution::Ruby18.new
+               else
+                 Facter::Core::Execution::Ruby19.new
+               end
+
+      def self.impl
+        @@impl
+      end
+
       module_function
 
       # Returns the locations to be searched when looking for a binary. This
@@ -13,14 +27,7 @@ module Execution
       # @return [Array<String>] the paths to be searched for binaries
       # @api private
       def search_paths
-        if Facter::Util::Config.is_windows?
-          ENV['PATH'].split(File::PATH_SEPARATOR)
-        else
-          # Make sure facter is usable even for non-root users. Most commands
-          # in /sbin (like ifconfig) can be run as non priviledged users as
-          # long as they do not modify anything - which we do not do with facter
-          ENV['PATH'].split(File::PATH_SEPARATOR) + [ '/sbin', '/usr/sbin' ]
-        end
+        @@impl.search_paths
       end
 
       # Determines the full path to a binary. If the supplied filename does not
@@ -36,44 +43,16 @@ def search_paths
       #
       # @api public
       def which(bin)
-        if absolute_path?(bin)
-          return bin if File.executable?(bin)
-        else
-          search_paths.each do |dir|
-            dest = File.join(dir, bin)
-            if Facter::Util::Config.is_windows?
-              dest.gsub!(File::SEPARATOR, File::ALT_SEPARATOR)
-              if File.extname(dest).empty?
-                exts = ENV['PATHEXT']
-                exts = exts ? exts.split(File::PATH_SEPARATOR) : %w[.COM .EXE .BAT .CMD]
-                exts.each do |ext|
-                  destext = dest + ext
-                  return destext if File.executable?(destext)
-                end
-              end
-            end
-            return dest if File.executable?(dest)
-          end
-        end
-        nil
+        @@impl.which(bin)
       end
 
       # Determine in a platform-specific way whether a path is absolute. This
       # defaults to the local platform if none is specified.
       #
       # @param path [String] the path to check
       # @param platform [:posix,:windows,nil] the platform logic to use
-      def absolute_path?(path, platform=nil)
-        # Escape once for the string literal, and once for the regex.
-        slash = '[\\\\/]'
-        name = '[^\\\\/]+'
-        regexes = {
-          :windows => %r!^(([A-Z]:#{slash})|(#{slash}#{slash}#{name}#{slash}#{name})|(#{slash}#{slash}\?#{slash}#{name}))!i,
-          :posix   => %r!^/!,
-        }
-        platform ||= Facter::Util::Config.is_windows? ? :windows : :posix
-
-        !! (path =~ regexes[platform])
+      def absolute_path?(path, platform = nil)
+        @@impl.absolute_path?(path, platform)
       end
 
       # Given a command line, this returns the command line with the
@@ -85,22 +64,7 @@ def absolute_path?(path, platform=nil)
       # @return [String, nil] the command line with the executable's path
       # expanded, or nil if the executable cannot be found.
       def expand_command(command)
-        if match = /^"(.+?)"(?:\s+(.*))?/.match(command)
-          exe, arguments = match.captures
-          exe = which(exe) and [ "\"#{exe}\"", arguments ].compact.join(" ")
-        elsif match = /^'(.+?)'(?:\s+(.*))?/.match(command) and not Facter::Util::Config.is_windows?
-          exe, arguments = match.captures
-          exe = which(exe) and [ "'#{exe}'", arguments ].compact.join(" ")
-        else
-          exe, arguments = command.split(/ /,2)
-          if exe = which(exe)
-            # the binary was not quoted which means it contains no spaces. But the
-            # full path to the binary may do so.
-            exe = "\"#{exe}\"" if exe =~ /\s/ and Facter::Util::Config.is_windows?
-            exe = "'#{exe}'" if exe =~ /\s/ and not Facter::Util::Config.is_windows?
-            [ exe, arguments ].compact.join(" ")
-          end
-        end
+        @@impl.expand_command(command)
       end
 
       # Overrides environment variables within a block of code.  The
@@ -115,31 +79,8 @@ def expand_command(command)
       # @return [void]
       #
       # @api public
-      def with_env(values)
-        old = {}
-        values.each do |var, value|
-          # save the old value if it exists
-          if old_val = ENV[var]
-            old[var] = old_val
-          end
-          # set the new (temporary) value for the environment variable
-          ENV[var] = value
-        end
-        # execute the caller's block, capture the return value
-        rv = yield
-      # use an ensure block to make absolutely sure we restore the variables
-      ensure
-        # restore the old values
-        values.each do |var, value|
-          if old.include?(var)
-            ENV[var] = old[var]
-          else
-            # if there was no old value, delete the key from the current environment variables hash
-            ENV.delete(var)
-          end
-        end
-        # return the captured return value
-        rv
+      def with_env(values, &block)
+        @@impl.with_env(values, &block)
       end
 
       # Executes a program and return the output of that program.
@@ -155,34 +96,8 @@ def with_env(values)
       # @note Since Facter 1.5.8 this strips trailing newlines from the
       #   returned value. If a fact will be used by versions of Facter older
       #   than 1.5.8 then you should call chomp the returned string.
-      def exec(code)
-
-        ## Set LANG to force i18n to C for the duration of this exec; this ensures that any code that parses the
-        ## output of the command can expect it to be in a consistent / predictable format / locale
-        with_env "LANG" => "C" do
-
-          if expanded_code = expand_command(code)
-            # if we can find the binary, we'll run the command with the expanded path to the binary
-            code = expanded_code
-          else
-            return nil
-          end
-
-          out = nil
-
-          begin
-            out = %x{#{code}}.chomp
-          rescue => detail
-            Facter.warn(detail)
-            return nil
-          end
-
-          if out == ""
-            return nil
-          else
-            return out
-          end
-        end
+      def exec(command)
+        @@impl.exec(command)
       end
     end
   end

lib/facter/core/execution/base.rb

@@ -0,0 +1,96 @@
+class Facter::Core::Execution::Base
+
+  def search_paths
+    if Facter::Util::Config.is_windows?
+      ENV['PATH'].split(File::PATH_SEPARATOR)
+    else
+      # Make sure facter is usable even for non-root users. Most commands
+      # in /sbin (like ifconfig) can be run as non priviledged users as
+      # long as they do not modify anything - which we do not do with facter
+      ENV['PATH'].split(File::PATH_SEPARATOR) + [ '/sbin', '/usr/sbin' ]
+    end
+  end
+
+  def which(bin)
+    if absolute_path?(bin)
+      return bin if File.executable?(bin)
+    else
+      search_paths.each do |dir|
+        dest = File.join(dir, bin)
+        if Facter::Util::Config.is_windows?
+          dest.gsub!(File::SEPARATOR, File::ALT_SEPARATOR)
+          if File.extname(dest).empty?
+            exts = ENV['PATHEXT']
+            exts = exts ? exts.split(File::PATH_SEPARATOR) : %w[.COM .EXE .BAT .CMD]
+            exts.each do |ext|
+              destext = dest + ext
+              return destext if File.executable?(destext)
+            end
+          end
+        end
+        return dest if File.executable?(dest)
+      end
+    end
+    nil
+  end
+
+  def absolute_path?(path, platform=nil)
+    # Escape once for the string literal, and once for the regex.
+    slash = '[\\\\/]'
+    name = '[^\\\\/]+'
+    regexes = {
+      :windows => %r!^(([A-Z]:#{slash})|(#{slash}#{slash}#{name}#{slash}#{name})|(#{slash}#{slash}\?#{slash}#{name}))!i,
+      :posix   => %r!^/!,
+    }
+    platform ||= Facter::Util::Config.is_windows? ? :windows : :posix
+
+    !! (path =~ regexes[platform])
+  end
+
+  def expand_command(command)
+    if match = /^"(.+?)"(?:\s+(.*))?/.match(command)
+      exe, arguments = match.captures
+      exe = which(exe) and [ "\"#{exe}\"", arguments ].compact.join(" ")
+    elsif match = /^'(.+?)'(?:\s+(.*))?/.match(command) and not Facter::Util::Config.is_windows?
+      exe, arguments = match.captures
+      exe = which(exe) and [ "'#{exe}'", arguments ].compact.join(" ")
+    else
+      exe, arguments = command.split(/ /,2)
+      if exe = which(exe)
+        # the binary was not quoted which means it contains no spaces. But the
+        # full path to the binary may do so.
+        exe = "\"#{exe}\"" if exe =~ /\s/ and Facter::Util::Config.is_windows?
+        exe = "'#{exe}'" if exe =~ /\s/ and not Facter::Util::Config.is_windows?
+        [ exe, arguments ].compact.join(" ")
+      end
+    end
+  end
+
+  def with_env(values)
+    old = {}
+    values.each do |var, value|
+      # save the old value if it exists
+      if old_val = ENV[var]
+        old[var] = old_val
+      end
+      # set the new (temporary) value for the environment variable
+      ENV[var] = value
+    end
+    # execute the caller's block, capture the return value
+    rv = yield
+  # use an ensure block to make absolutely sure we restore the variables
+  ensure
+    # restore the old values
+    values.each do |var, value|
+      if old.include?(var)
+        ENV[var] = old[var]
+      else
+        # if there was no old value, delete the key from the current environment variables hash
+        ENV.delete(var)
+      end
+    end
+    # return the captured return value
+    rv
+  end
+
+end

lib/facter/core/execution/ruby18.rb

@@ -0,0 +1,50 @@
+class Facter::Core::Execution::Ruby18 < Facter::Core::Execution::Base
+
+  def exec(code)
+
+    ## Set LANG to force i18n to C for the duration of this exec; this ensures that any code that parses the
+    ## output of the command can expect it to be in a consistent / predictable format / locale
+    with_env "LANG" => "C" do
+
+      if expanded_code = expand_command(code)
+        # if we can find the binary, we'll run the command with the expanded path to the binary
+        code = expanded_code
+      else
+        return nil
+      end
+
+      out = nil
+
+      begin
+        wait_for_child = true
+        out = %x{#{code}}.chomp
+        wait_for_child = false
+      rescue => detail
+        Facter.warn(detail.message)
+        return nil
+      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
+
+      if out == ""
+        return nil
+      else
+        return out
+      end
+    end
+  end
+end

lib/facter/core/execution/ruby19.rb

@@ -0,0 +1,45 @@
+class Facter::Core::Execution::Ruby19 < Facter::Core::Execution::Base
+
+  def exec(code)
+
+    ## Set LANG to force i18n to C for the duration of this exec; this ensures that any code that parses the
+    ## output of the command can expect it to be in a consistent / predictable format / locale
+    with_env "LANG" => "C" do
+
+      if expanded_code = expand_command(code)
+        # if we can find the binary, we'll run the command with the expanded path to the binary
+        code = expanded_code
+      else
+        return nil
+      end
+
+      out = nil
+
+      stdout_r, stdout_w = IO.pipe
+
+      begin
+        pid = Process.spawn(code, {:out => stdout_w.fileno, :err => :close})
+        stdout_w.close
+
+        Process.waitpid(pid)
+        pid = nil
+        out = stdout_r.read
+
+        out.chomp!
+      rescue => detail
+        Facter.warn(detail.message)
+        return nil
+      ensure
+        if pid
+          Process.detach(pid)
+        end
+      end
+
+      if out == ""
+        return nil
+      else
+        return out
+      end
+    end
+  end
+end

lib/facter/core/resolvable.rb

@@ -67,11 +67,6 @@ def value
     Facter::Util::Normalization.normalize(result)
   rescue Timeout::Error => detail
     Facter.warn "Timed out seeking value for #{self.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 #{self.name} resolved to an invalid value: #{e.message}"