On Jul 24, 2009, at 10:21 PM, MarkusQ wrote: I dunno if it's on the dev lifecycle page, but can you reformat this comment just a bit, so the first line is short and the second is blank?
You can follow the guidelines here: http://www.tpope.net/node/106 > * initial load attempts are processed (as before), > * recursive load attempts return immediately (as before), > * but subsequent concurrent load attempts from different threads > wait on a semaphore (condition variable) and then retry (e.g. > use the now-valid results of the first thread). > > This is a slight modification of the solution I'd originally > proposed, to prevent a deadlock > that could have arisen if three or more threads simultaneously > attempted to load the same item. > Though it solves the bug as reported, it has room for improvement: > > * Failures aren't cached, so repeated attempts will be made to > import invalid items each time they are encountered > * It doesn't address any of the underlying referential ambiguity > (module vs. filename) > * The threading logic should probably be refactored into a separate > class (as a start I encapsulated it in an ad hoc singleton class, > so at least it isn't cluttering up the load method) > > Signed-off-by: MarkusQ <[email protected]> > --- > lib/puppet/parser/parser_support.rb | 116 ++++++++++++++++++ > +---------------- > 1 files changed, 64 insertions(+), 52 deletions(-) > > diff --git a/lib/puppet/parser/parser_support.rb b/lib/puppet/parser/ > parser_support.rb > index e1af2fe..224c2db 100644 > --- a/lib/puppet/parser/parser_support.rb > +++ b/lib/puppet/parser/parser_support.rb > @@ -112,36 +112,22 @@ class Puppet::Parser::Parser > end > > def find_or_load(namespace, name, type) > - method = "find_" + type.to_s > - if result = @loaded_code.send(method, namespace, name) > - return result > - end > - > + method = "find_#{type}" > fullname = (namespace + "::" + name).sub(/^::/, '') > - self.load(fullname) > - > - if result = @loaded_code.send(method, namespace, name) > - return result > - end > - > - # Try to load the module init file if we're a qualified > - # name > - if fullname.include?("::") > - module_name = fullname.split("::")[0] > + things_to_try = [fullname] > > - self.load(module_name) > + # Try to load the module init file if we're a qualified name > + things_to_try << fullname.split("::")[0] if > fullname.include?("::") > > - if result = @loaded_code.send(method, namespace, name) > - return result > - end > - end > - > - # Now try to load the bare name on its own. This is > - # appropriate if the class we're looking for is in a > + # Otherwise try to load the bare name on its own. This > + # is appropriate if the class we're looking for is in a > # module that's different from our namespace. > - self.load(name) > + things_to_try << name > > - @loaded_code.send(method, namespace, name) > + until (result = @loaded_code.send(method, namespace, name)) > or things_to_try.empty? do > + self.load(things_to_try.shift) > + end > + return result Nice. > > end > > # Import our files. > @@ -207,42 +193,68 @@ class Puppet::Parser::Parser > @lexer = Puppet::Parser::Lexer.new() > @files = {} > @loaded = [] > - end > - > - # Try to load a class, since we could not find it. > - def load(classname) > - return false if classname == "" > - filename = classname.gsub("::", File::SEPARATOR) > - > - # First try to load the top-level module > - mod = filename.scan(/^[\w-]+/).shift > - unless @loaded.include?(mod) > - @loaded << mod > - begin > - import(mod) > - Puppet.info "Autoloaded module %s" % mod > - rescue Puppet::ImportError => detail > - # We couldn't load the module > + @loading = {} > + @loading.extend(MonitorMixin) > + class << @loading > + def done_with(item) > + synchronize do > + delete(item)[:busy].signal if self.has_key? > (item) and self[item][:loader] == Thread.current > + end > + 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 > + else > + flag = self[item][:busy] > + flag.wait > + flag.signal > + :another_thread > + end > + end Not so sure about the live patch, but we can give it a try. It'd be nice to have a bit of commentary here around why this is necessary. It's probably actually worth pulling ths code into a separate method; maybe a 'loading' accessor method that initializes @loading this way if it's nil: def loading return @loading if defined?(@loading) .... end Then you just s/@loading/loading/g to use the accessor, basically. > > end > end > + end > > - # We don't know whether we're looking for a class or > definition, so we have > - # to test for both. > - return true if @loaded_code.hostclass(classname) || > @loaded_code.definition(classname) > - > - unless @loaded.include?(filename) > - @loaded << filename > - # Then the individual file > + # Utility method factored out of load > + def able_to_import?(classname,item,msg) > + unless @loaded.include?(item) > begin > - import(filename) > - Puppet.info "Autoloaded file %s from module %s" % > [filename, mod] > + case @loading.owner_of(item) > + when :this_thread > + return > + when :another_thread > + return able_to_import?(classname,item,msg) > + when :nobody > + import(item) > + Puppet.info "Autoloaded #{msg}" > + @loaded << item > + end > rescue Puppet::ImportError => detail > - # We couldn't load the file > + # We couldn't load the item > + ensure > + @loading.done_with(item) > end > end > # We don't know whether we're looking for a class or > definition, so we have > # to test for both. > - return true if @loaded_code.hostclass(classname) || > @loaded_code.definition(classname) > + return @loaded_code.hostclass(classname) || > @loaded_code.definition(classname) > + end > + > + # Try to load a class, since we could not find it. > + def load(classname) > + return false if classname == "" > + filename = classname.gsub("::", File::SEPARATOR) > + mod = filename.scan(/^[\w-]+/).shift > + > + # First try to load the top-level module then the > individual file > + # First try to load the top-level module then the > individual file > + [[mod, "module %s" % mod ], What's with this whitespace? > > + [filename,"file %s from module %s" % [filename, mod]] > + ].any? { |item,description| able_to_import? > (classname,item,description) } > end Nice. > > # Split an fq name into a namespace and name > -- > 1.6.0.4 > > > > -- The chief lesson I have learned in a long life is that the only way to make a man trustworthy is to trust him; and the surest way to make him untrustworthy is to distrust him and show your distrust. -- Henry L. Stimson --------------------------------------------------------------------- Luke Kanies | http://reductivelabs.com | http://madstop.com --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
