Fix for #4955 -- Race condition & memory leak in Puppet::Util

The Puppet::Util.sync method was not thread safe and also leaked memory. I'm
not certain, but I believe the first is ironic and the second is merely a bug.

This patch addresses the problem by 1) refactoring so the sync objects
are never returned (and thus no one can cache a reference to one) 2) adding
reference counting 3) deleting them when they are no longer needed 4) doing
the thread safty dance.

It wasn't the first (or even second) solution considered, but it's the one
that I was able to make work in a way that I'm convinced is correct.  Its
main advantage is that it puts all the tricky bits in one place.

1 files changed, 6 insertions, 20 deletions

diff --git a/spec/unit/util/file_locking_spec.rb b/spec/unit/util/file_locking_spec.rb
index 476aa2caa..8fafb1d52 100755
--- a/spec/unit/util/file_locking_spec.rb
+++ b/spec/unit/util/file_locking_spec.rb
@@ -32,17 +32,12 @@ describe Puppet::Util::FileLocking do
     end
 
     it "should use a global shared mutex" do
-      @sync = mock 'sync'
-      @sync.expects(:synchronize).with(Sync::SH).once
-      Puppet::Util.expects(:sync).with('/file').returns @sync
-
+      Puppet::Util.expects(:synchronize_on).with('/file',Sync::SH).once
       Puppet::Util::FileLocking.readlock '/file'
     end
 
     it "should use a shared lock on the file" do
-      @sync = mock 'sync'
-      @sync.stubs(:synchronize).yields
-      Puppet::Util.expects(:sync).with('/file').returns @sync
+      Puppet::Util.expects(:synchronize_on).with('/file',Sync::SH).yields
 
       fh = mock 'filehandle'
       File.expects(:open).with("/file").yields fh
@@ -59,9 +54,7 @@ describe Puppet::Util::FileLocking do
     end
 
     it "should create missing files" do
-      @sync = mock 'sync'
-      @sync.stubs(:synchronize).yields
-      Puppet::Util.expects(:sync).with('/file').returns @sync
+      Puppet::Util.expects(:synchronize_on).with('/file',Sync::SH).yields
 
       File.expects(:exists?).with('/file').returns false
       File.expects(:open).with('/file').once
@@ -72,9 +65,7 @@ describe Puppet::Util::FileLocking do
 
   describe "when acquiring a write lock" do
     before do
-      @sync = mock 'sync'
-      Puppet::Util.stubs(:sync).returns @sync
-      @sync.stubs(:synchronize).yields
+      Puppet::Util.stubs(:synchronize_on).yields
       File.stubs(:file?).with('/file').returns true
       File.stubs(:exists?).with('/file').returns true
     end
@@ -88,10 +79,7 @@ describe Puppet::Util::FileLocking do
     end
 
     it "should use a global exclusive mutex" do
-      sync = mock 'sync'
-      sync.expects(:synchronize).with(Sync::EX)
-      Puppet::Util.expects(:sync).with("/file").returns sync
-
+      Puppet::Util.expects(:synchronize_on).with("/file",Sync::EX)
       Puppet::Util::FileLocking.writelock '/file'
     end
 
@@ -161,9 +149,7 @@ describe Puppet::Util::FileLocking do
     end
 
     it "should create missing files" do
-      @sync = mock 'sync'
-      @sync.stubs(:synchronize).yields
-      Puppet::Util.expects(:sync).with('/file').returns @sync
+      Puppet::Util.expects(:synchronize_on).with('/file',Sync::EX).yields
 
       File.expects(:exists?).with('/file').returns false
       File.expects(:open).with('/file', File::Constants::CREAT | File::Constants::WRONLY, 0600).once