Merge pull request #13159 from chrisroberts/unlock-fix

Release file lock before file deletion

lib/vagrant/util/file_mutex.rb

@@ -1,10 +1,16 @@
 module Vagrant
   module Util
+    # Utility to provide a simple mutex via file lock
     class FileMutex
+      # Create a new FileMutex instance
+      #
+      # @param mutex_path [String] path for file
       def initialize(mutex_path)
         @mutex_path = mutex_path
       end
 
+      # Execute provided block within lock and unlock
+      # when completed
       def with_lock(&block)
         lock
         begin
@@ -16,16 +22,26 @@ def with_lock(&block)
         end
       end
 
+      # Attempt to acquire the lock
       def lock
-        f = File.open(@mutex_path, "w+") 
-        if f.flock(File::LOCK_EX|File::LOCK_NB) === false
+        if lock_file.flock(File::LOCK_EX|File::LOCK_NB) === false
           raise Errors::VagrantLocked, lock_file_path: @mutex_path
         end
       end
 
+      # Unlock the file
       def unlock
+        lock_file.flock(File::LOCK_UN)
+        lock_file.close
         File.delete(@mutex_path) if File.file?(@mutex_path)
       end
+
+      protected
+
+      def lock_file
+        return @lock_file if @lock_file && !@lock_file.closed?
+        @lock_file = File.open(@mutex_path, "w+")
+      end
     end
   end
 end

test/unit/vagrant/util/file_mutex_test.rb

@@ -5,77 +5,75 @@
   include_context "unit"
 
   let(:temp_dir) { Dir.mktmpdir("vagrant-test-util-mutex_test") }
+  let(:mutex_path) { File.join(temp_dir, "test.lock") }
+  let(:subject) { described_class.new(mutex_path) }
 
   after do
     FileUtils.rm_rf(temp_dir)
   end
 
   it "should create a lock file" do
-    mutex_path = temp_dir + "test.lock"
-    instance = described_class.new(mutex_path)
-    instance.lock
+    subject.lock
     expect(File).to exist(mutex_path)
   end
 
   it "should create and delete lock file" do
-    mutex_path = temp_dir + "test.lock"
-    instance = described_class.new(mutex_path)
-    instance.lock
+    subject.lock
     expect(File).to exist(mutex_path)
-    instance.unlock
+    subject.unlock
     expect(File).to_not exist(mutex_path)
   end
 
   it "should not raise an error if the lock file does not exist" do
-    mutex_path = temp_dir + "test.lock"
-    instance = described_class.new(mutex_path)
-    instance.unlock
+    subject.unlock
     expect(File).to_not exist(mutex_path)
   end
 
   it "should run a function with a lock" do
-    mutex_path = temp_dir + "test.lock"
-    instance = described_class.new(mutex_path)
-    instance.with_lock { true }
+    subject.with_lock { true }
     expect(File).to_not exist(mutex_path)
   end
 
   it "should fail running a function when locked" do
-    mutex_path = temp_dir + "test.lock"
     # create a lock
-    instance = described_class.new(mutex_path)
-    instance.lock
+    subject.lock
     # create a new lock that will run a function
-    instance2 = described_class.new(mutex_path)
+    instance = described_class.new(mutex_path)
     # lock should persist for multiple runs
-    expect {instance2.with_lock { true }}.
+    expect {instance.with_lock { true }}.
       to raise_error(Vagrant::Errors::VagrantLocked)
-    expect {instance2.with_lock { true }}.
+    expect {instance.with_lock { true }}.
       to raise_error(Vagrant::Errors::VagrantLocked)
     # mutex should exist until its unlocked
     expect(File).to exist(mutex_path)
-    instance.unlock
+    subject.unlock
     expect(File).to_not exist(mutex_path)
   end
 
   it "should fail running a function within a locked" do
-    mutex_path = temp_dir + "test.lock"
-    # create a lock
-    instance = described_class.new(mutex_path)
     # create a new lock that will run a function
-    instance2 = described_class.new(mutex_path)
+    instance = described_class.new(mutex_path)
     expect {
-      instance.with_lock { instance2.with_lock{true} }
+      subject.with_lock { instance.with_lock{true} }
     }.to raise_error(Vagrant::Errors::VagrantLocked)
     expect(File).to_not exist(mutex_path)
   end
 
   it "should delete the lock even when the function fails" do
-    mutex_path = temp_dir + "test.lock"
-    instance = described_class.new(mutex_path)
     expect {
-      instance.with_lock { raise Vagrant::Errors::VagrantError.new }
+      subject.with_lock { raise Vagrant::Errors::VagrantError.new }
     }.to raise_error(Vagrant::Errors::VagrantError)
     expect(File).to_not exist(mutex_path)
   end
+
+  it "should unlock file before deletion" do
+    lock_file = double(:lock_file)
+    allow(subject).to receive(:lock_file).and_return(lock_file)
+    allow(lock_file).to receive(:flock).and_return(true)
+
+    expect(lock_file).to receive(:flock).with(File::LOCK_UN)
+    expect(lock_file).to receive(:close)
+
+    subject.with_lock { true }
+  end
 end