+1. There's one thing I just noticed re-reading the code is that imported modules don't seem to get their module_name associated. It's not a big deal, but might be addressed by your patch, though.
On 16/07/10 04:56, Markus Roberts wrote: > The import function was calling type_loader#import directly so that it could > pass in the current file name, but by doing so it was thwarting the thread- > safety locking level. This patch rearanges things so that all imports go > through the same (thread safe) code path while retaining the current_file > passing, error handling, etc. from the old structure. > > Signed-off-by: Markus Roberts <[email protected]> > --- > lib/puppet/parser/parser_support.rb | 2 +- > lib/puppet/parser/type_loader.rb | 28 +++++++++++++--------------- > spec/unit/parser/type_loader_spec.rb | 26 +++++++++++++------------- > 3 files changed, 27 insertions(+), 29 deletions(-) > > diff --git a/lib/puppet/parser/parser_support.rb > b/lib/puppet/parser/parser_support.rb > index 4f3a4dd..c0fd371 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(file, @lexer.file) > + known_resource_types.loader.import_if_possible(file, @lexer.file) > end > > def initialize(env) > diff --git a/lib/puppet/parser/type_loader.rb > b/lib/puppet/parser/type_loader.rb > index cb8657f..c33f90d 100644 > --- a/lib/puppet/parser/type_loader.rb > +++ b/lib/puppet/parser/type_loader.rb > @@ -70,7 +70,7 @@ class Puppet::Parser::TypeLoader > > def initialize(env) > self.environment = env > - @loaded = [] > + @loaded = {} > @loading = Helper.new > > @imported = {} > @@ -79,10 +79,13 @@ class Puppet::Parser::TypeLoader > def load_until(namespaces, name) > return nil if name == "" # special-case main. > name2files(namespaces, name).each do |filename| > - modname = nil > - import_if_possible(filename) do > - modname = import(filename) > - @loaded << filename > + modname = begin > + import_if_possible(filename) > + 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. > + nil > end > if result = yield(filename) > Puppet.info "Automatically imported #{name} from #{filename}" > @@ -124,23 +127,18 @@ class Puppet::Parser::TypeLoader > parser.parse > end > > - private > - > # 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, &blk) > - return if @loaded.include?(file) > - begin > + def import_if_possible(file, current_file = nil) > + @loaded[file] || begin > case @loading.owner_of(file) > when :this_thread > - return > + nil > when :another_thread > - return import_if_possible(file, &blk) > + import_if_possible(file,current_file) > when :nobody > - blk.call > + @loaded[file] = import(file,current_file) > end > - rescue Puppet::ImportError => detail > - # We couldn't load the item > ensure > @loading.done_with(file) > end > diff --git a/spec/unit/parser/type_loader_spec.rb > b/spec/unit/parser/type_loader_spec.rb > index db72a23..8f005d5 100644 > --- a/spec/unit/parser/type_loader_spec.rb > +++ b/spec/unit/parser/type_loader_spec.rb > @@ -38,16 +38,16 @@ describe Puppet::Parser::TypeLoader do > > it "should attempt to import each generated name" do > @loader.expects(:name2files).returns %w{foo bar} > - @loader.expects(:import).with("foo") > - @loader.expects(:import).with("bar") > + @loader.expects(:import).with("foo",nil) > + @loader.expects(:import).with("bar",nil) > @loader.load_until(["foo"], "bar") { |f| false } > end > > it "should yield after each import" do > yielded = [] > @loader.expects(:name2files).returns %w{foo bar} > - @loader.expects(:import).with("foo") > - @loader.expects(:import).with("bar") > + @loader.expects(:import).with("foo",nil) > + @loader.expects(:import).with("bar",nil) > @loader.load_until(["foo"], "bar") { |f| yielded << f; false } > yielded.should == %w{foo bar} > end > @@ -55,31 +55,31 @@ describe Puppet::Parser::TypeLoader do > it "should stop importing when the yielded block returns true" do > yielded = [] > @loader.expects(:name2files).returns %w{foo bar baz} > - @loader.expects(:import).with("foo") > - @loader.expects(:import).with("bar") > - @loader.expects(:import).with("baz").never > + @loader.expects(:import).with("foo",nil) > + @loader.expects(:import).with("bar",nil) > + @loader.expects(:import).with("baz",nil).never > @loader.load_until(["foo"], "bar") { |f| true if f == "bar" } > end > > it "should return the result of the block" do > yielded = [] > @loader.expects(:name2files).returns %w{foo bar baz} > - @loader.expects(:import).with("foo") > - @loader.expects(:import).with("bar") > - @loader.expects(:import).with("baz").never > + @loader.expects(:import).with("foo",nil) > + @loader.expects(:import).with("bar",nil) > + @loader.expects(:import).with("baz",nil).never > @loader.load_until(["foo"], "bar") { |f| 10 if f == "bar" }.should == > 10 > end > > it "should return nil if the block never returns true" do > @loader.expects(:name2files).returns %w{foo bar} > - @loader.expects(:import).with("foo") > - @loader.expects(:import).with("bar") > + @loader.expects(:import).with("foo",nil) > + @loader.expects(:import).with("bar",nil) > @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") > + @loader.expects(:import).with("file",nil) > @loader.load_until(["foo"], "bar") { |f| true } > @loader.should be_loaded("file") > end -- Brice Figureau My Blog: http://www.masterzen.fr/ -- 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.
