diff options
| -rw-r--r-- | lib/puppet/parser/parser_support.rb | 2 | ||||
| -rw-r--r-- | lib/puppet/parser/type_loader.rb | 116 | ||||
| -rw-r--r-- | spec/lib/puppet_spec/files.rb | 1 | ||||
| -rw-r--r-- | spec/unit/parser/type_loader_spec.rb | 18 |
4 files changed, 63 insertions, 74 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 diff --git a/spec/lib/puppet_spec/files.rb b/spec/lib/puppet_spec/files.rb index cab4a1e47..52ed903ec 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 58c386d96..cfa68871d 100644 --- a/spec/unit/parser/type_loader_spec.rb +++ b/spec/unit/parser/type_loader_spec.rb @@ -36,18 +36,13 @@ describe Puppet::Parser::TypeLoader do @loader.expects(:import).with("foo",nil).returns([]) @loader.try_load_fqname(:hostclass, "foo::bar") { |f| false } end - - it "should know when a given name has been loaded" do - @loader.expects(:import).with("file",nil).returns([]) - @loader.try_load_fqname(:hostclass, "file") { |f| true } - @loader.should be_loaded("file") - end end describe "when importing" do before do Puppet::Parser::Files.stubs(:find_manifests).returns ["modname", %w{file}] - @loader.stubs(:parse_file).returns(Puppet::Parser::AST::Hostclass.new('')) + Puppet::Parser::Parser.any_instance.stubs(:parse).returns(Puppet::Parser::AST::Hostclass.new('')) + Puppet::Parser::Parser.any_instance.stubs(:file=) end it "should return immediately when imports are being ignored" do @@ -88,16 +83,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.returns(Puppet::Parser::AST::Hostclass.new('')) + Puppet::Parser::Parser.any_instance.expects(:parse).once.returns(Puppet::Parser::AST::Hostclass.new('')) @loader.import("myfile") # This will fail if it tries to reimport the file. |