+1 On Dec 18, 2009, at 1:48 PM, Markus Roberts wrote:
> This appears to be regression introduced by threading changes. The > fix was > to rearrange things to keep the old behaviour (don't clear the > settings > until you know the config file parses) and the new (don't nest calls > to > synchronize) by: > > 1. Splitting clear into two parts--clear, which works as before, and > unsafe_clear which it calls and which expects synchronization to be > handled externally. > > 2. Rearranging the code to recover the previous calling order > > 3. Trapping syntax errors and turning them into logged messages and a > no-op effect. > > 4. Fixing reparse to not wrap a call to this code with a synchronize. > > 5. Tests. > > Signed-off-by: Markus Roberts <[email protected]> > --- > lib/puppet/util/settings.rb | 55 ++++++++++++++++++++++ > +------------------- > spec/unit/util/settings.rb | 19 +++++++++++++++ > 2 files changed, 49 insertions(+), 25 deletions(-) > > diff --git a/lib/puppet/util/settings.rb b/lib/puppet/util/settings.rb > index e80c7cc..df07d5c 100644 > --- a/lib/puppet/util/settings.rb > +++ b/lib/puppet/util/settings.rb > @@ -64,20 +64,25 @@ class Puppet::Util::Settings > # Remove all set values, potentially skipping cli values. > def clear(exceptcli = false) > @sync.synchronize do > - @values.each do |name, values| > - @values.delete(name) unless exceptcli and name > == :cli > - end > + unsafe_clear(exceptcli) > + end > + end > + > + # Remove all set values, potentially skipping cli values. > + def unsafe_clear(exceptcli = false) > + @values.each do |name, values| > + @values.delete(name) unless exceptcli and name == :cli > + end > > - # Don't clear the 'used' in this case, since it's a > config file reparse, > - # and we want to retain this info. > - unless exceptcli > - @used = [] > - end > + # Don't clear the 'used' in this case, since it's a config > file reparse, > + # and we want to retain this info. > + unless exceptcli > + @used = [] > + end > > - @cache.clear > + @cache.clear > > - @name = nil > - end > + @name = nil > end > > # This is mostly just used for testing. > @@ -317,23 +322,25 @@ class Puppet::Util::Settings > # and reparsed if necessary. > set_filetimeout_timer() > > - # Retrieve the value now, so that we don't lose it in the > 'clear' call. > - file = self[:config] > - > - return unless FileTest.exist?(file) > - > - # We have to clear outside of the sync, because it's > - # also using synchronize(). > - clear(true) > - > @sync.synchronize do > - unsafe_parse(file) > + unsafe_parse(self[:config]) > end > end > > # Unsafely parse the file -- this isn't thread-safe and causes > plenty of problems if used directly. > def unsafe_parse(file) > - parse_file(file).each do |area, values| > + return unless FileTest.exist?(file) > + begin > + data = parse_file(file) > + rescue => details > + puts details.backtrace if Puppet[:trace] > + Puppet.err "Could not parse #{file}: #{details}" > + return > + end > + > + unsafe_clear(true) > + > + data.each do |area, values| > @values[area] = values > end > > @@ -425,9 +432,7 @@ class Puppet::Util::Settings > def reparse > if file and file.changed? > Puppet.notice "Reparsing %s" % file.file > - @sync.synchronize do > - parse > - end > + parse > reuse() > end > end > diff --git a/spec/unit/util/settings.rb b/spec/unit/util/settings.rb > index aa2101f..5642813 100755 > --- a/spec/unit/util/settings.rb > +++ b/spec/unit/util/settings.rb > @@ -579,6 +579,25 @@ describe Puppet::Util::Settings do > # and we should now have the new value in memory > @settings[:two].should == "disk-replace" > end > + > + it "should retain in-memory values if the file has a syntax > error" do > + # Init the value > + text = "[main]\none = initial-value\n" > + @settings.expects(:read_file).returns(text) > + @settings.parse > + @settings[:one].should == "initial-value" > + > + # Now replace the value with something bogus > + text = "[main]\nkenny = killed-by-what-follows\n1 is 2, > blah blah florp\n" > + @settings.expects(:read_file).returns(text) > + @settings.parse > + > + # The originally-overridden value should not be > replaced with the default > + @settings[:one].should == "initial-value" > + > + # and we should not have the new value in memory > + @settings[:kenny].should be_nil > + end > end > > it "should provide a method for creating a catalog of resources > from its configuration" do > -- > 1.6.4 > > -- > > 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 > . > > -- It is well to remember that the entire universe, with one trifling exception, is composed of others. --John Andrew Holmes --------------------------------------------------------------------- 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.
