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.
