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 <ja...@puppetlabs.com> 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 <p...@puppetlabs.com>
> > ---
> > 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 puppet-...@googlegroups.com.
> > To unsubscribe from this group, send email to
> puppet-dev+unsubscr...@googlegroups.com<puppet-dev%2bunsubscr...@googlegroups.com>
> .
> > 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 puppet-...@googlegroups.com.
To unsubscribe from this group, send email to
puppet-dev+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en.
