From e9a0cb7a28a34fd04db4bfe1db347da5d774f2e8 Mon Sep 17 00:00:00 2001
From: Markus Roberts <Markus@reality.com>
Date: Fri, 18 Dec 2009 13:40:29 -0800
Subject: Fix for #2657 (retain old setting if config has syntax error)

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 <Markus@reality.com>
---
 lib/puppet/util/settings.rb | 55 ++++++++++++++++++++++++---------------------
 1 file changed, 30 insertions(+), 25 deletions(-)

(limited to 'lib/puppet')

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
-- 
cgit