Fixing #1812 (hopefully) - adding read and write locks to yaml.

It's obviously not really possible to test that this fixes it,
but I'm confident that the locks work, and now we're using them,
so it *should*.

Signed-off-by: Luke Kanies <luke@madstop.com>

2 files changed, 26 insertions, 10 deletions

diff --git a/lib/puppet/indirector/yaml.rb b/lib/puppet/indirector/yaml.rb
index 3f05ce618..5b53a7628 100644
--- a/lib/puppet/indirector/yaml.rb
+++ b/lib/puppet/indirector/yaml.rb
@@ -1,17 +1,26 @@
 require 'puppet/indirector/terminus'
+require 'puppet/util/file_locking'
 
 # The base class for YAML indirection termini.
 class Puppet::Indirector::Yaml < Puppet::Indirector::Terminus
+    include Puppet::Util::FileLocking
+
     # Read a given name's file in and convert it from YAML.
     def find(request)
         file = path(request.key)
         return nil unless FileTest.exist?(file)
 
+        yaml = nil
         begin
-            return from_yaml(File.read(file))
+            readlock(file) { |fh| yaml = fh.read }
         rescue => detail
             raise Puppet::Error, "Could not read YAML data for %s %s: %s" % [indirection.name, request.key, detail]
         end
+        begin
+            return from_yaml(yaml)
+        rescue => detail
+            raise Puppet::Error, "Could not parse YAML data for %s %s: %s" % [indirection.name, request.key, detail]
+        end
     end
 
     # Convert our object to YAML and store it to the disk.
@@ -28,7 +37,7 @@ class Puppet::Indirector::Yaml < Puppet::Indirector::Terminus
         end
 
         begin
-            File.open(file, "w", 0660) { |f| f.print to_yaml(request.instance) }
+            writelock(file, 0660) { |f| f.print to_yaml(request.instance) }
         rescue TypeError => detail
             Puppet.err "Could not save %s %s: %s" % [self.name, request.key, detail]
         end
@@ -36,7 +45,7 @@ class Puppet::Indirector::Yaml < Puppet::Indirector::Terminus
 
     # Get the yaml directory
     def base
-      (Puppet[:name] == "puppetmasterd") ? Puppet[:yamldir] : Puppet[:clientyamldir]
+        (Puppet[:name] == "puppetmasterd") ? Puppet[:yamldir] : Puppet[:clientyamldir]
     end
 
     # Return the path to a given node's file.
diff --git a/spec/unit/indirector/yaml.rb b/spec/unit/indirector/yaml.rb
index 081ae9666..7536fbc25 100755
--- a/spec/unit/indirector/yaml.rb
+++ b/spec/unit/indirector/yaml.rb
@@ -58,12 +58,12 @@ describe Puppet::Indirector::Yaml, " when choosing file location" do
             proc { @store.save(@request) }.should raise_error(ArgumentError)
         end
 
-        it "should convert Ruby objects to YAML and write them to disk" do
+        it "should convert Ruby objects to YAML and write them to disk using a write lock" do
             yaml = @subject.to_yaml
             file = mock 'file'
             path = @store.send(:path, @subject.name)
             FileTest.expects(:exist?).with(File.dirname(path)).returns(true)
-            File.expects(:open).with(path, "w", 0660).yields(file)
+            @store.expects(:writelock).with(path, 0660).yields(file)
             file.expects(:print).with(yaml)
 
             @store.save(@request)
@@ -74,9 +74,11 @@ describe Puppet::Indirector::Yaml, " when choosing file location" do
             file = mock 'file'
             path = @store.send(:path, @subject.name)
             dir = File.dirname(path)
+
             FileTest.expects(:exist?).with(dir).returns(false)
             Dir.expects(:mkdir).with(dir)
-            File.expects(:open).with(path, "w", 0660).yields(file)
+
+            @store.expects(:writelock).yields(file)
             file.expects(:print).with(yaml)
 
             @store.save(@request)
@@ -84,24 +86,29 @@ describe Puppet::Indirector::Yaml, " when choosing file location" do
     end
 
     describe Puppet::Indirector::Yaml, " when retrieving YAML" do
-        it "should read YAML in from disk and convert it to Ruby objects" do
+        it "should read YAML in from disk using a read lock and convert it to Ruby objects" do
             path = @store.send(:path, @subject.name)
 
             yaml = @subject.to_yaml
             FileTest.expects(:exist?).with(path).returns(true)
-            File.expects(:read).with(path).returns(yaml)
+
+            fh = mock 'filehandle'
+            @store.expects(:readlock).with(path).yields fh
+            fh.expects(:read).returns yaml
 
             @store.find(@request).instance_variable_get("@name").should == :me
         end
 
         it "should fail coherently when the stored YAML is invalid" do
             path = @store.send(:path, @subject.name)
+            FileTest.expects(:exist?).with(path).returns(true)
 
             # Something that will fail in yaml
             yaml = "--- !ruby/object:Hash"
 
-            FileTest.expects(:exist?).with(path).returns(true)
-            File.expects(:read).with(path).returns(yaml)
+            fh = mock 'filehandle'
+            @store.expects(:readlock).yields fh
+            fh.expects(:read).returns yaml
 
             proc { @store.find(@request) }.should raise_error(Puppet::Error)
         end