On Wed, 22 Sep 2010 14:51:45 -0700, Paul Berry wrote: > > You're right on all counts, Jacob. I've squashed the following changes into > my existing commit (which is available at > http://github.com/stereotype441/puppet/tree/ticket/2.6.x/4771): > > diff --git a/lib/puppet/parser/type_loader.rb > b/lib/puppet/parser/type_loader.rb > index 33c5304..bae5603 100644 > --- a/lib/puppet/parser/type_loader.rb > +++ b/lib/puppet/parser/type_loader.rb > @@ -20,13 +20,20 @@ class Puppet::Parser::TypeLoader > # 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 > + end > false > when :done > false > @@ -34,6 +41,7 @@ class Puppet::Parser::TypeLoader > @state[file] = :doing > @thread[file] = Thread.current > @cond_var[file] = new_cond > + true > end > end > if need_to_execute > > > On Wed, Sep 22, 2010 at 11:57 AM, Jacob Helwig <[email protected]> wrote: > > > On Mon, 20 Sep 2010 15:59:19 -0700, Paul Berry wrote: > > > > > > 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 <[email protected]> > > > --- > > > 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. > > > > I was a bit confused by the "this thread is already executing it" part, > > until it was brought to my attention that there might be circular > > imports. Might be worth explicitly mentioning that to save people a few > > brain cycles in the future? > > > > > + def do_once(file) > > > + need_to_execute = synchronize do > > > + case @state[file] > > > + when :doing > > > + if @thread[file] != Thread.current > > > + @cond_var[file].wait > > > + end > > > > Indentation on '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 > > > > Should this return new_cond in the "true" case, or explicitly return > > true? Do we care? > > > > > + 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 [email protected]. > > > To unsubscribe from this group, send email to > > [email protected]<puppet-dev%[email protected]> > > . > > > For more options, visit this group at > > http://groups.google.com/group/puppet-dev?hl=en. > > > > > > > -- > > Jacob Helwig > > > > -----BEGIN PGP SIGNATURE----- > > Version: GnuPG v1.4.10 (GNU/Linux) > > > > iQGcBAEBAgAGBQJMmlF6AAoJEHJabXWGiqEB3qYL/1OXRqPuasD7rLUwBbWOQcth > > kgpnM6oPKG63xgIThi9DnUwUDTOtRRGZi8bCtfHHWvEf789ZK7MKMEGcQwPZRB2r > > R1t7hDkSnZ/cBPN2ashqGRaqWkHN9eCGWFpSoMNPsXTynXDFjWLn0FUDgZ2WNI8u > > /BC0tX8F9sBP2g9uC0zUEyrVculCeXXi13guVDanOTllGk/UkKMg+kU017IrmhHV > > 4Xdrt+0JBO68G4Bu1J1uxsHhNla7wjN1LC7MgMbezSV3VwDze/XZ1H6A6+uVRehL > > HIrYKqUiylGg7EsQJ56YBSc0/Hl0/AMJaaxSMCBwTycpVvwGXOxw3XdB7JjA1XZX > > nkdp1497IBYMolyLC/n/Pj0GRiiiz/9CeSZFa59t7yLiJAVBrzbViUhq+tSU09e7 > > w9GpXWV7lx4RjfTxIjHqzNRJ6V7co5iGW2slxQLnb1b/c+3wR3OOuqSTCmPZ74yt > > 9kFYf1KXcwMm9MmW8gbZfSCo6JHKOh5LMLaZ3r1kkQ== > > =Ojg0 > > -----END PGP SIGNATURE----- > > > > > > -- > You received this message because you are subscribed to the Google Groups > "Puppet Developers" group. > To post to this group, send email to [email protected]. > To unsubscribe from this group, send email to > [email protected]. > For more options, visit this group at > http://groups.google.com/group/puppet-dev?hl=en. >
+1 with the squashed in changes. -- Jacob Helwig
signature.asc
Description: Digital signature
