The function import_if_possible, which was supposed to be responsible
for making sure that no two threads tried to import the same file at
the same time, was not making this decision based on the full pathname
of the file, since it was being invoked before pathnames were
resolved.  As a result, if we attempted to import two distinct files
with the same name at the same time (either in two threads or in a
single thread due to recursion), one of the files would not always get
imported.

Fixed this problem by moving the thread-safety logic to happen after
filenames are resolved to absolute paths.  This made it possible to
simplify the thread-safety logic significantly.

Signed-off-by: Paul Berry <p...@puppetlabs.com>
---
 lib/puppet/parser/parser_support.rb  |    2 +-
 lib/puppet/parser/type_loader.rb     |   90 ++++++++++++++++------------------
 spec/lib/puppet_spec/files.rb        |    1 +
 spec/unit/parser/type_loader_spec.rb |   19 +------
 4 files changed, 47 insertions(+), 65 deletions(-)

diff --git a/lib/puppet/parser/parser_support.rb 
b/lib/puppet/parser/parser_support.rb
index c0fd371..4f3a4dd 100644
--- a/lib/puppet/parser/parser_support.rb
+++ b/lib/puppet/parser/parser_support.rb
@@ -111,7 +111,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 09aa636..33c5304 100644
--- a/lib/puppet/parser/type_loader.rb
+++ b/lib/puppet/parser/type_loader.rb
@@ -3,25 +3,48 @@ 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.
+    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
+        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
@@ -51,8 +74,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
         parse_file(file)
       end
     end
@@ -60,27 +82,20 @@ class Puppet::Parser::TypeLoader
     modname
   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
 
   def load_until(namespaces, name)
     return nil if name == "" # special-case main.
     name2files(namespaces, name).each do |filename|
       modname = begin
-        import_if_possible(filename)
+        import(filename)
       rescue Puppet::ImportError => detail
         # We couldn't load the item
         # I'm not convienced we should just drop these errors, but this
@@ -96,10 +111,6 @@ class Puppet::Parser::TypeLoader
     nil
   end
 
-  def loaded?(name)
-    @loaded.include?(name)
-  end
-
   def name2files(namespaces, name)
     return [name.sub(/^::/, '').gsub("::", File::SEPARATOR)] if name =~ /^::/
 
@@ -126,21 +137,4 @@ class Puppet::Parser::TypeLoader
     parser.file = file
     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
 end
diff --git a/spec/lib/puppet_spec/files.rb b/spec/lib/puppet_spec/files.rb
index cab4a1e..52ed903 100644
--- a/spec/lib/puppet_spec/files.rb
+++ b/spec/lib/puppet_spec/files.rb
@@ -1,4 +1,5 @@
 require 'fileutils'
+require 'tempfile'
 
 # A support module for testing files.
 module PuppetSpec::Files
diff --git a/spec/unit/parser/type_loader_spec.rb 
b/spec/unit/parser/type_loader_spec.rb
index 83006b3..02d543b 100644
--- a/spec/unit/parser/type_loader_spec.rb
+++ b/spec/unit/parser/type_loader_spec.rb
@@ -77,13 +77,6 @@ describe Puppet::Parser::TypeLoader do
       @loader.load_until(["foo"], "bar") { |f| false }.should be_nil
     end
 
-    it "should know when a given name has been loaded" do
-      @loader.expects(:name2files).returns %w{file}
-      @loader.expects(:import).with("file",nil)
-      @loader.load_until(["foo"], "bar") { |f| true }
-      @loader.should be_loaded("file")
-    end
-
     it "should set the module name on any created resource types" do
       type = Puppet::Resource::Type.new(:hostclass, "mytype")
 
@@ -113,7 +106,8 @@ describe Puppet::Parser::TypeLoader do
   describe "when importing" do
     before do
       Puppet::Parser::Files.stubs(:find_manifests).returns ["modname", 
%w{file}]
-      @loader.stubs(:parse_file)
+      Puppet::Parser::Parser.any_instance.stubs(:parse)
+      Puppet::Parser::Parser.any_instance.stubs(:file=)
     end
 
     it "should return immediately when imports are being ignored" do
@@ -154,16 +148,9 @@ describe Puppet::Parser::TypeLoader do
       @loader.import("myfile", "/current/file")
     end
 
-    it "should know when a given file has been imported" do
-      Puppet::Parser::Files.expects(:find_manifests).returns ["modname", 
%w{/one}]
-      @loader.import("myfile")
-
-      @loader.should be_imported("/one")
-    end
-
     it "should not attempt to import files that have already been imported" do
       Puppet::Parser::Files.expects(:find_manifests).returns ["modname", 
%w{/one}]
-      @loader.expects(:parse_file).once
+      Puppet::Parser::Parser.any_instance.expects(:parse).once
       @loader.import("myfile")
 
       # This will fail if it tries to reimport the file.
-- 
1.7.2

-- 
You received this message because you are subscribed to the Google Groups 
"Puppet Developers" group.
To post to this group, send email to puppet-...@googlegroups.com.
To unsubscribe from this group, send email to 
puppet-dev+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/puppet-dev?hl=en.

Reply via email to