Merge pull request #13057 from soapy1/box-race-condition

Add a file mutex when downloading box files.

lib/vagrant/action/builtin/box_add.rb

@@ -6,6 +6,7 @@ require "uri"
 require "vagrant/box_metadata"
 require "vagrant/util/downloader"
 require "vagrant/util/file_checksum"
+require "vagrant/util/file_mutex"
 require "vagrant/util/platform"
 
 module Vagrant
@@ -479,12 +480,21 @@ module Vagrant
           end
 
           begin
-            d.download!
-          rescue Errors::DownloaderInterrupted
-            # The downloader was interrupted, so just return, because that
-            # means we were interrupted as well.
-            @download_interrupted = true
-            env[:ui].info(I18n.t("vagrant.actions.box.download.interrupted"))
+            mutex_path = d.destination + ".lock"
+            Util::FileMutex.new(mutex_path).with_lock do
+              begin
+                d.download!
+              rescue Errors::DownloaderInterrupted
+                # The downloader was interrupted, so just return, because that
+                # means we were interrupted as well.
+                @download_interrupted = true
+                env[:ui].info(I18n.t("vagrant.actions.box.download.interrupted"))
+              end
+            end
+          rescue Errors::VagrantLocked
+            raise Errors::DownloadAlreadyInProgress,
+              dest_path: d.destination,
+              lock_file_path: mutex_path
           end
 
           Pathname.new(d.destination)

lib/vagrant/errors.rb

@@ -376,6 +376,10 @@ module Vagrant
       error_key(:dotfile_upgrade_json_error)
     end
 
+    class DownloadAlreadyInProgress < VagrantError
+      error_key(:download_already_in_progress_error)
+    end
+
     class DownloaderError < VagrantError
       error_key(:downloader_error)
     end
@@ -920,6 +924,10 @@ module Vagrant
       error_key(:uploader_interrupted)
     end
 
+    class VagrantLocked < VagrantError
+      error_key(:vagrant_locked)
+    end
+
     class VagrantInterrupt < VagrantError
       error_key(:interrupted)
     end

lib/vagrant/util.rb

@@ -20,6 +20,7 @@ module Vagrant
     autoload :Experimental,              'vagrant/util/experimental'
     autoload :FileChecksum,              'vagrant/util/file_checksum'
     autoload :FileMode,                  'vagrant/util/file_mode'
+    autoload :FileMutex,                 'vagrant/util/file_mutex'
     autoload :GuestHosts,                'vagrant/util/guest_hosts'
     autoload :GuestInspection,           'vagrant/util/guest_inspection'
     autoload :HashWithIndifferentAccess, 'vagrant/util/hash_with_indifferent_access'

lib/vagrant/util/file_mutex.rb

@@ -0,0 +1,31 @@
+module Vagrant
+  module Util
+    class FileMutex
+      def initialize(mutex_path)
+        @mutex_path = mutex_path
+      end
+
+      def with_lock(&block)
+        lock
+        begin
+          block.call
+        rescue => e
+          raise e
+        ensure
+          unlock
+        end
+      end
+
+      def lock
+        f = File.open(@mutex_path, "w+") 
+        if f.flock(File::LOCK_EX|File::LOCK_NB) === false
+          raise Errors::VagrantLocked, lock_file_path: @mutex_path
+        end
+      end
+
+      def unlock
+        File.delete(@mutex_path) if File.file?(@mutex_path)
+      end
+    end
+  end
+end

templates/locales/en.yml

@@ -895,6 +895,13 @@ en:
         support.
 
         State file path: %{state_file}
+      download_already_in_progress_error: |-
+        Download to global Vagrant location already in progress. This
+        may be caused by other Vagrant processes attempting to download
+        a file to the same location.
+
+        Download path: %{dest_path}
+        Lock file path: %{lock_file_path}
       downloader_error: |-
         An error occurred while downloading the remote file. The error
         message, if any, is reproduced below. Please fix this error and try
@@ -1668,6 +1675,11 @@ en:
       uploader_interrupted: |-
         The upload was interrupted by an external signal. It did not
         complete.
+      vagrant_locked: |-
+        The requested Vagrant action is locked. This may be caused
+        by other Vagrant processes attempting to do a similar action.
+
+        Lock file path: %{lock_file_path}
       vagrantfile_exists: |-
         `Vagrantfile` already exists in this directory. Remove it before
         running `vagrant init`.

test/unit/vagrant/action/builtin/box_add_test.rb

@@ -84,6 +84,27 @@ describe Vagrant::Action::Builtin::BoxAdd, :skip_windows, :bsdtar do
     allow(box_collection).to receive(:find).and_return(nil)
   end
 
+  context "the download location is locked" do
+    let(:box_path) { iso_env.box2_file(:virtualbox) }
+    let(:mutex_path) {  env[:tmp_path].join("box" + Digest::SHA1.hexdigest("file://" + box_path.to_s) + ".lock").to_s }
+
+    before do
+      # Lock file
+      @f = File.open(mutex_path, "w+") 
+      @f.flock(File::LOCK_EX|File::LOCK_NB) 
+    end
+  
+    after { @f.close }
+    
+    it "raises a download error" do
+      env[:box_name] = "foo"
+      env[:box_url] = box_path.to_s
+
+      expect { subject.call(env) }.
+        to raise_error(Vagrant::Errors::DownloadAlreadyInProgress)
+    end
+  end
+
   context "with box file directly" do
     it "adds it" do
       box_path = iso_env.box2_file(:virtualbox)

test/unit/vagrant/util/file_mutex_test.rb

@@ -0,0 +1,81 @@
+require File.expand_path("../../../base", __FILE__)
+require 'vagrant/util/file_mutex'
+
+describe Vagrant::Util::FileMutex do
+  include_context "unit"
+
+  let(:temp_dir) { Dir.mktmpdir("vagrant-test-util-mutex_test") }
+
+  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
+    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
+    expect(File).to exist(mutex_path)
+    instance.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
+    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 }
+    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
+    # create a new lock that will run a function
+    instance2 = described_class.new(mutex_path)
+    # lock should persist for multiple runs
+    expect {instance2.with_lock { true }}.
+      to raise_error(Vagrant::Errors::VagrantLocked)
+    expect {instance2.with_lock { true }}.
+      to raise_error(Vagrant::Errors::VagrantLocked)
+    # mutex should exist until its unlocked
+    expect(File).to exist(mutex_path)
+    instance.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)
+    expect {
+      instance.with_lock { instance2.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 }
+    }.to raise_error(Vagrant::Errors::VagrantError)
+    expect(File).to_not exist(mutex_path)
+  end
+end