diff --git a/lib/vagrant/action/builtin/box_add.rb b/lib/vagrant/action/builtin/box_add.rb index c9307beea..3b5624c4f 100644 --- a/lib/vagrant/action/builtin/box_add.rb +++ b/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) diff --git a/lib/vagrant/errors.rb b/lib/vagrant/errors.rb index 664da86e5..8e6bb8989 100644 --- a/lib/vagrant/errors.rb +++ b/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 diff --git a/lib/vagrant/util.rb b/lib/vagrant/util.rb index 4b3e0ff09..8e3cbd2af 100644 --- a/lib/vagrant/util.rb +++ b/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' diff --git a/lib/vagrant/util/file_mutex.rb b/lib/vagrant/util/file_mutex.rb new file mode 100644 index 000000000..0844726dd --- /dev/null +++ b/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 diff --git a/templates/locales/en.yml b/templates/locales/en.yml index 9e6d7ca16..4a18ff7f6 100644 --- a/templates/locales/en.yml +++ b/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`. diff --git a/test/unit/vagrant/action/builtin/box_add_test.rb b/test/unit/vagrant/action/builtin/box_add_test.rb index 67652d501..a28bcb16a 100644 --- a/test/unit/vagrant/action/builtin/box_add_test.rb +++ b/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) diff --git a/test/unit/vagrant/util/file_mutex_test.rb b/test/unit/vagrant/util/file_mutex_test.rb new file mode 100644 index 000000000..eacbd9ffd --- /dev/null +++ b/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