From 2b50f30c703aca5c4f3e89961d64a94d886296bd Mon Sep 17 00:00:00 2001 From: Paul Berry Date: Fri, 17 Sep 2010 17:59:56 -0700 Subject: [#4771] Import of manifests with the same name only happens once 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. --- lib/puppet/parser/parser_support.rb | 2 +- lib/puppet/parser/type_loader.rb | 98 ++++++++++++++++++------------------ spec/lib/puppet_spec/files.rb | 1 + 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. -- cgit