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.