Partial merge to 2.6.2rc1 : Merge commit '2b50f30' into next

Commit 2b50f30 simplified and fixed bugs in code that had already been
modified extensively by 4da88fb and 6b1dd81.  This merge resolution
commit is a manual replay of the changes from 2b50f30 onto next.

Manually Resolved Conflicts:
      lib/puppet/parser/type_loader.rb
      spec/unit/parser/type_loader_spec.rb

2 files changed, 59 insertions, 59 deletions

diff --git a/lib/puppet/parser/parser_support.rb b/lib/puppet/parser/parser_support.rb
index b1688cd22..41bebe420 100644
--- a/lib/puppet/parser/parser_support.rb
+++ b/lib/puppet/parser/parser_support.rb
@@ -102,7 +102,7 @@ class Puppet::Parser::Parser
   end
 
   def import(file)
-    known_resource_types.loader.import_if_possible(file, @lexer.file)
+    known_resource_types.loader.import(file, @lexer.file)
   end
 
   def initialize(env)
diff --git a/lib/puppet/parser/type_loader.rb b/lib/puppet/parser/type_loader.rb
index 8a183f493..140c9f2ca 100644
--- a/lib/puppet/parser/type_loader.rb
+++ b/lib/puppet/parser/type_loader.rb
@@ -3,25 +3,56 @@ require 'puppet/node/environment'
 class Puppet::Parser::TypeLoader
   include Puppet::Node::Environment::Helper
 
-  class Helper < Hash
+  # Helper class that makes sure we don't try to import the same file
+  # more than once from either the same thread or different threads.
+  class Helper
     include MonitorMixin
-    def done_with(item)
-      synchronize do
-        delete(item)[:busy].signal if self.has_key?(item) and self[item][:loader] == Thread.current
-      end
+    def initialize
+      super
+      # These hashes are indexed by filename
+      @state = {} # :doing or :done
+      @thread = {} # if :doing, thread that's doing the parsing
+      @cond_var = {} # if :doing, condition var that will be signaled when done.
     end
-    def owner_of(item)
-      synchronize do
-        if !self.has_key? item
-          self[item] = { :loader => Thread.current, :busy => self.new_cond}
-          :nobody
-        elsif self[item][:loader] == Thread.current
-          :this_thread
+
+    # Execute the supplied block exactly once per file, no matter how
+    # many threads have asked for it to run.  If another thread is
+    # already executing it, wait for it to finish.  If this thread is
+    # already executing it, return immediately without executing the
+    # block.
+    #
+    # Note: the reason for returning immediately if this thread is
+    # already executing the block is to handle the case of a circular
+    # import--when this happens, we attempt to recursively re-parse a
+    # file that we are already in the process of parsing.  To prevent
+    # an infinite regress we need to simply do nothing when the
+    # recursive import is attempted.
+    def do_once(file)
+      need_to_execute = synchronize do
+        case @state[file]
+        when :doing
+          if @thread[file] != Thread.current
+            @cond_var[file].wait
+          end
+          false
+        when :done
+          false
         else
-          flag = self[item][:busy]
-          flag.wait
-          flag.signal
-          :another_thread
+          @state[file] = :doing
+          @thread[file] = Thread.current
+          @cond_var[file] = new_cond
+          true
+        end
+      end
+      if need_to_execute
+        begin
+          yield
+        ensure
+          synchronize do
+            @state[file] = :done
+            @thread.delete(file)
+            @cond_var.delete(file).broadcast
+          end
         end
       end
     end
@@ -52,8 +83,7 @@ class Puppet::Parser::TypeLoader
       unless file =~ /^#{File::SEPARATOR}/
         file = File.join(dir, file)
       end
-      unless imported? file
-        @imported[file] = true
+      @loading_helper.do_once(file) do
         loaded_asts << parse_file(file)
       end
     end
@@ -62,48 +92,35 @@ class Puppet::Parser::TypeLoader
     end
   end
 
-  def imported?(file)
-    @imported.has_key?(file)
-  end
-
   def known_resource_types
     environment.known_resource_types
   end
 
   def initialize(env)
     self.environment = env
-    @loaded = {}
-    @loading = Helper.new
-
-    @imported = {}
+    @loading_helper = Helper.new
   end
 
   # Try to load the object with the given fully qualified name.
   def try_load_fqname(type, fqname)
     return nil if fqname == "" # special-case main.
     name2files(fqname).each do |filename|
-      if not loaded?(filename)
-        begin
-          imported_types = import_if_possible(filename)
-          if result = imported_types.find { |t| t.type == type and t.name == fqname }
-            Puppet.debug "Automatically imported #{fqname} from #{filename} into #{environment}"
-            return result
-          end
-        rescue Puppet::ImportError => detail
-          # We couldn't load the item
-          # I'm not convienced we should just drop these errors, but this
-          # preserves existing behaviours.
+      begin
+        imported_types = import(filename)
+        if result = imported_types.find { |t| t.type == type and t.name == fqname }
+          Puppet.debug "Automatically imported #{fqname} from #{filename} into #{environment}"
+          return result
         end
+      rescue Puppet::ImportError => detail
+        # We couldn't load the item
+        # I'm not convienced we should just drop these errors, but this
+        # preserves existing behaviours.
       end
     end
     # Nothing found.
     return nil
   end
 
-  def loaded?(name)
-    @loaded.include?(name)
-  end
-
   def parse_file(file)
     Puppet.debug("importing '#{file}' in environment #{environment}")
     parser = Puppet::Parser::Parser.new(environment)
@@ -111,23 +128,6 @@ class Puppet::Parser::TypeLoader
     return parser.parse
   end
 
-  # Utility method factored out of load for handling thread-safety.
-  # This isn't tested in the specs, because that's basically impossible.
-  def import_if_possible(file, current_file = nil)
-    @loaded[file] || begin
-      case @loading.owner_of(file)
-      when :this_thread
-        nil
-      when :another_thread
-        import_if_possible(file,current_file)
-      when :nobody
-        @loaded[file] = import(file,current_file)
-      end
-    ensure
-      @loading.done_with(file)
-    end
-  end
-
   private
 
   # Return a list of all file basenames that should be tried in order