Fixed #2294 - Classes sometimes cannot be found

This patch should fix the race condition causing ticket 2294; it extends the loading logic so that:

    * initial load attempts are processed (as before),
    * recursive load attempts return immediately (as before),
    * but subsequent concurrent load attempts from different threads
      wait on a semaphore (condition variable) and then retry (e.g.
      use the now-valid results of the first thread).

This is a slight modification of the solution I'd originally proposed, to prevent a deadlock
that could have arisen if three or more threads simultaneously attempted to load the same item.
Though it solves the bug as reported, it has room for improvement:

    * Failures aren't cached, so repeated attempts will be made to
      import invalid items each time they are encountered
    * It doesn't address any of the underlying referential ambiguity
      (module vs. filename)
    * The threading logic should probably be refactored into a separate
      class (as a start I encapsulated it in an ad hoc singleton class,
      so at least it isn't cluttering up the load method)

Signed-off-by: Markus Roberts <Markus@reality.com>

2 files changed, 134 insertions, 53 deletions

diff --git a/lib/puppet/parser/parser_support.rb b/lib/puppet/parser/parser_support.rb
index f1c1da0b0..bc3444c2a 100644
--- a/lib/puppet/parser/parser_support.rb
+++ b/lib/puppet/parser/parser_support.rb
@@ -112,36 +112,22 @@ class Puppet::Parser::Parser
     end
 
     def find_or_load(namespace, name, type)
-        method = "find_" + type.to_s
-        if result = @loaded_code.send(method, namespace, name)
-            return result
-        end
-
+        method = "find_#{type}"
         fullname = (namespace + "::" + name).sub(/^::/, '')
-        self.load(fullname)
-
-        if result = @loaded_code.send(method, namespace, name)
-            return result
-        end
-
-        # Try to load the module init file if we're a qualified
-        # name
-        if fullname.include?("::")
-            module_name = fullname.split("::")[0]
+        names_to_try = [fullname]
 
-            self.load(module_name)
+        # Try to load the module init file if we're a qualified name
+        names_to_try << fullname.split("::")[0] if fullname.include?("::")
 
-            if result = @loaded_code.send(method, namespace, name)
-                return result
-            end
-        end
-
-        # Now try to load the bare name on its own.  This is
-        # appropriate if the class we're looking for is in a
+        # Otherwise try to load the bare name on its own.  This
+        # is appropriate if the class we're looking for is in a
         # module that's different from our namespace.
-        self.load(name)
+        names_to_try << name
 
-        @loaded_code.send(method, namespace, name)
+        until (result = @loaded_code.send(method, namespace, name)) or names_to_try.empty? do
+            self.load(names_to_try.shift)
+        end
+        return result
     end
 
     # Import our files.
@@ -207,42 +193,67 @@ class Puppet::Parser::Parser
         @lexer = Puppet::Parser::Lexer.new()
         @files = {}
         @loaded = []
-    end
-
-    # Try to load a class, since we could not find it.
-    def load(classname)
-        return false if classname == ""
-        filename = classname.gsub("::", File::SEPARATOR)
-
-        # First try to load the top-level module
-        mod = filename.scan(/^[\w-]+/).shift
-        unless @loaded.include?(mod)
-            @loaded << mod
-            begin
-                import(mod)
-                Puppet.info "Autoloaded module %s" % mod
-            rescue Puppet::ImportError => detail
-                # We couldn't load the module
+        @loading = {}
+        @loading.extend(MonitorMixin)
+        class << @loading
+            def done_with(item)
+                synchronize do 
+                    delete(item)[:busy].signal if self.has_key?(item) and self[item][:loader] == Thread.current
+                end
+            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
+                      else
+                        flag = self[item][:busy]
+                        flag.wait
+                        flag.signal
+                        :another_thread
+                    end
+                end
             end
         end
+    end
 
-        # We don't know whether we're looking for a class or definition, so we have
-        # to test for both.
-        return true if @loaded_code.hostclass(classname) || @loaded_code.definition(classname)
-
-        unless @loaded.include?(filename)
-            @loaded << filename
-            # Then the individual file
+    # Utility method factored out of load
+    def able_to_import?(classname,item,msg)
+        unless @loaded.include?(item)
             begin
-                import(filename)
-                Puppet.info "Autoloaded file %s from module %s" % [filename, mod]
+              case @loading.owner_of(item)
+              when :this_thread
+                  return
+              when :another_thread
+                  return able_to_import?(classname,item,msg)
+              when :nobody
+                  import(item)
+                  Puppet.info "Autoloaded #{msg}"
+                  @loaded << item
+              end
             rescue Puppet::ImportError => detail
-                # We couldn't load the file
+                # We couldn't load the item
+            ensure
+                @loading.done_with(item)
             end
         end
         # We don't know whether we're looking for a class or definition, so we have
         # to test for both.
-        return true if @loaded_code.hostclass(classname) || @loaded_code.definition(classname)
+        return @loaded_code.hostclass(classname) || @loaded_code.definition(classname)
+    end
+
+    # Try to load a class, since we could not find it.
+    def load(classname)
+        return false if classname == ""
+        filename = classname.gsub("::", File::SEPARATOR)
+        mod = filename.scan(/^[\w-]+/).shift
+
+        # First try to load the top-level module then the individual file
+        [[mod,     "module %s"              %            mod ],
+         [filename,"file %s from module %s" % [filename, mod]]
+        ].any? { |item,description| able_to_import?(classname,item,description) }
     end
 
     # Split an fq name into a namespace and name
diff --git a/spec/unit/parser/parser.rb b/spec/unit/parser/parser.rb
index bd12f7155..75d0c05a3 100755
--- a/spec/unit/parser/parser.rb
+++ b/spec/unit/parser/parser.rb
@@ -328,4 +328,74 @@ describe Puppet::Parser do
             @parser.version.should == "output"
         end
     end
- end
+
+    describe Puppet::Parser,"when looking up definitions" do
+        it "should check for them by name" do
+            @parser.stubs(:find_or_load).with("namespace","name",:definition).returns(:this_value)
+            @parser.find_definition("namespace","name").should == :this_value
+        end
+    end
+
+    describe Puppet::Parser,"when looking up hostclasses" do
+        it "should check for them by name" do
+            @parser.stubs(:find_or_load).with("namespace","name",:hostclass).returns(:this_value)
+            @parser.find_hostclass("namespace","name").should == :this_value
+        end
+    end
+
+    describe Puppet::Parser,"when looking up names" do
+        before :each do
+            @loaded_code = mock 'loaded code'
+            @loaded_code.stubs(:find_my_type).with('Loaded_namespace',  'Loaded_name').returns(true)
+            @loaded_code.stubs(:find_my_type).with('Bogus_namespace',   'Bogus_name' ).returns(false)
+            @parser = Puppet::Parser::Parser.new :environment => "development",:loaded_code => @loaded_code
+        end
+
+        describe "that are already loaded" do
+            it "should not try to load anything" do
+                @parser.expects(:load).never
+                @parser.find_or_load("Loaded_namespace","Loaded_name",:my_type)
+            end
+            it "should return true" do
+                @parser.find_or_load("Loaded_namespace","Loaded_name",:my_type).should == true
+            end
+        end
+
+        describe "that aren't already loaded" do
+            it "should first attempt to load them with the fully qualified name" do
+                @loaded_code.stubs(:find_my_type).with("Foo_namespace","Foo_name").returns(false,true,true)
+                @parser.expects(:load).with("Foo_namespace::Foo_name").returns(true).then.raises(Exception)
+                @parser.find_or_load("Foo_namespace","Foo_name",:my_type).should == true
+            end
+
+            it "should next attempt to load them with the namespace" do
+                @loaded_code.stubs(:find_my_type).with("Foo_namespace","Foo_name").returns(false,false,true,true)
+                @parser.expects(:load).with("Foo_namespace::Foo_name").returns(false).then.raises(Exception)
+                @parser.expects(:load).with("Foo_namespace").returns(true).then.raises(Exception)
+                @parser.find_or_load("Foo_namespace","Foo_name",:my_type).should == true
+            end
+
+            it "should return false if the name isn't found" do
+                @parser.stubs(:load).returns(false)
+                @parser.find_or_load("Bogus_namespace","Bogus_name",:my_type).should == false
+            end
+        end
+    end
+
+    describe Puppet::Parser,"when loading classnames" do
+        before :each do
+            @loaded_code = mock 'loaded code'
+            @parser = Puppet::Parser::Parser.new :environment => "development",:loaded_code => @loaded_code
+        end
+
+        it "should just return false if the classname is empty" do
+            @parser.expects(:import).never
+            @parser.load("").should == false
+        end
+
+        it "should just return true if the item is loaded" do
+            pending "Need to access internal state (@parser's @loaded) to force this"
+            @parser.load("").should == false
+        end
+    end
+end