2 files changed, 49 insertions, 25 deletions

diff --git a/lib/puppet/util/settings.rb b/lib/puppet/util/settings.rb
index e80c7cc8f..df07d5c51 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 aa2101f6e..56428132d 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