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.
> For more options, visit this group at
> http://groups.google.com/group/puppet-dev?hl=en.
>
--
Jacob Helwig
signature.asc
Description: Digital signature
