diff options
| -rw-r--r-- | lib/puppet/parser/parser_support.rb | 2 | ||||
| -rw-r--r-- | lib/puppet/parser/type_loader.rb | 98 | ||||
| -rw-r--r-- | spec/lib/puppet_spec/files.rb | 1 | ||||
| -rw-r--r-- | spec/unit/parser/type_loader_spec.rb | 19 |
4 files changed, 55 insertions, 65 deletions
diff --git a/lib/puppet/parser/parser_support.rb b/lib/puppet/parser/parser_support.rb index c0fd37178..4f3a4ddff 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 09aa636e1..bae560381 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 @@ -51,8 +82,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 +90,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 +119,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 +145,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 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 83006b37b..02d543b02 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. |