core: MachineIndex has precision locking for entries

lib/vagrant/errors.rb

@@ -408,6 +408,10 @@ module Vagrant
       error_key(:machine_guest_not_ready)
     end
 
+    class MachineLocked < VagrantError
+      error_key(:machine_locked)
+    end
+
     class MachineNotFound < VagrantError
       error_key(:machine_not_found)
     end

lib/vagrant/machine_index.rb

@@ -1,6 +1,7 @@
 require "json"
 require "pathname"
 require "securerandom"
+require "thread"
 
 module Vagrant
   # MachineIndex is able to manage the index of created Vagrant environments
@@ -35,44 +36,81 @@ module Vagrant
   class MachineIndex
     # Initializes a MachineIndex at the given file location.
     #
-    # @param [Pathname] data_file Path to the file that should be used
-    #   to maintain the machine index. This file doesn't have to exist
-    #   but this location must be writable.
-    def initialize(data_file)
-      @data_file = data_file
+    # @param [Pathname] data_dir Path to the directory where data for the
+    #   index can be stored. This folder should exist and must be writable.
+    def initialize(data_dir)
+      @data_dir   = data_dir
+      @index_file = data_dir.join("index")
+      @lock       = Mutex.new
       @machines  = {}
+      @machine_locks = {}
 
-      if @data_file.file?
-        data = nil
-        begin
-          data = JSON.load(@data_file.read)
-        rescue JSON::ParserError
-          raise Errors::CorruptMachineIndex, path: data_file.to_s
-        end
-
-        if data
-          if !data["version"] || data["version"].to_i != 1
-            raise Errors::CorruptMachineIndex, path: data_file.to_s
-          end
-
-          @machines = data["machines"] || {}
-        end
+      with_index_lock do
+        unlocked_reload
       end
     end
 
     # Accesses a machine by UUID and returns a {MachineIndex::Entry}
     #
+    # The entry returned is locked and can't be read again or updated by
+    # this process or any other. To unlock the machine, call {#release}
+    # with the entry.
+    #
+    # You can only {#set} an entry (update) when the lock is held.
+    #
     # @param [String] uuid UUID for the machine to access.
     # @return [MachineIndex::Entry]
-    def [](uuid)
+    def get(uuid)
+      entry = nil
+
+      @lock.synchronize do
+        with_index_lock do
           return nil if !@machines[uuid]
-      Entry.new(uuid, @machines[uuid].merge("id" => uuid))
+
+          entry = Entry.new(uuid, @machines[uuid].merge("id" => uuid))
+
+          # Lock this machine
+          lock_file = lock_machine(uuid)
+          if !lock_file
+            raise Errors::MachineLocked,
+              name: entry.name,
+              provider: entry.provider
+          end
+
+          @machine_locks[uuid] = lock_file
+        end
+      end
+
+      entry
+    end
+
+    # Releases an entry, unlocking it.
+    #
+    # This is an idempotent operation. It is safe to call this even if you're
+    # unsure if an entry is locked or not.
+    #
+    # After calling this, the previous entry should no longer be used.
+    #
+    # @param [Entry] entry
+    def release(entry)
+      @lock.synchronize do
+        lock_file = @machine_locks[entry.id]
+        if lock_file
+          lock_file.close
+          @machine_locks.delete(entry.id)
+        end
+      end
     end
 
     # Creates/updates an entry object and returns the resulting entry.
     #
     # If the entry was new (no UUID), then the UUID will be set on the
-    # resulting entry and can be used.
+    # resulting entry and can be used. Additionally, the a lock will
+    # be created for the resulting entry, so you must {#release} it
+    # if you want others to be able to access it.
+    #
+    # If the entry isn't new (has a UUID). then this process must hold
+    # that entry's lock or else this set will fail.
     #
     # @param [Entry] entry
     # @return [Entry]
@@ -82,21 +120,82 @@ module Vagrant
 
       # Set an ID if there isn't one already set
       id     = entry.id
-      id     ||= SecureRandom.uuid
 
-      # Store the data
+      @lock.synchronize do
+        # Verify the machine is locked so we can safely write
+        # to it.
+        if !id
+          id = SecureRandom.uuid
+          lock_file = lock_machine(id)
+          if !lock_file
+            raise "Failed to lock new machine: #{entry.name}"
+          end
+
+          @machine_locks[id] = lock_file
+        end
+
+        if !@machine_locks[id]
+          raise "Unlocked write on machine: #{id}"
+        end
+
+        with_index_lock do
+          # Reload so we have the latest machine data, then update
+          # this particular machine, then write. This allows other processes
+          # to update their own machines without conflicting with our own.
+          unlocked_reload
           @machines[id] = struct
-      save
+          unlocked_save
+        end
+      end
 
       Entry.new(id, struct)
     end
 
-    # Saves the index.
+    protected
+
+    # Locks a machine exclusively to us, returning the file handle
+    # that holds the lock.
     #
-    # This doesn't usually need to be called because {#set} will
-    # automatically save as well.
-    def save
-      @data_file.open("w") do |f|
+    # If the lock cannot be acquired, then nil is returned.
+    #
+    # @return [File]
+    def lock_machine(uuid)
+      lock_path = @data_dir.join("#{uuid}.lock")
+      lock_file = lock_path.open("w+")
+      if lock_file.flock(File::LOCK_EX | File::LOCK_NB) === false
+        lock_file.close
+        lock_file = nil
+      end
+
+      lock_file
+    end
+
+    # This will reload the data without locking the index. It is assumed
+    # the caller with lock the index outside of this call.
+    #
+    # @param [File] f
+    def unlocked_reload
+      return if !@index_file.file?
+
+      data = nil
+      begin
+        data = JSON.load(@index_file.read)
+      rescue JSON::ParserError
+        raise Errors::CorruptMachineIndex, path: @index_file.to_s
+      end
+
+      if data
+        if !data["version"] || data["version"].to_i != 1
+          raise Errors::CorruptMachineIndex, path: @index_file.to_s
+        end
+
+        @machines = data["machines"] || {}
+      end
+    end
+
+    # Saves the index.
+    def unlocked_save
+      @index_file.open("w") do |f|
         f.write(JSON.dump({
           "version"  => 1,
           "machines" => @machines,
@@ -104,6 +203,16 @@ module Vagrant
       end
     end
 
+
+    # This will hold a lock to the index so it can be read or updated.
+    def with_index_lock
+      lock_path = "#{@index_file}.lock"
+      File.open(lock_path, "w+") do |f|
+        f.flock(File::LOCK_EX)
+        yield
+      end
+    end
+
     # An entry in the MachineIndex.
     class Entry
       # The unique ID for this entry. This is _not_ the ID for the

templates/locales/en.yml

@@ -729,6 +729,14 @@ en:
         Guest-specific operations were attempted on a machine that is not
         ready for guest communication. This should not happen and a bug
         should be reported.
+      machine_locked: |-
+        Vagrant can't use the requested machine because it is locekd! This
+        means that another Vagrant process is currently reading or modifying
+        the machine. Please wait for that Vagrant process to end and try
+        again. Details about the machine are shown below:
+
+        Name: %{name}
+        Provider: %{provider}
       machine_not_found: |-
         The machine with the name '%{name}' was not found configured for
         this Vagrant environment.

test/unit/vagrant/machine_directory_test.rb

@@ -9,12 +9,12 @@ require "vagrant/machine_index"
 describe Vagrant::MachineIndex do
   include_context "unit"
 
-  let(:data_file) { temporary_file }
+  let(:data_dir) { temporary_dir }
 
-  subject { described_class.new(data_file) }
+  subject { described_class.new(data_dir) }
 
   it "raises an exception if the data file is corrupt" do
-    data_file.open("w") do |f|
+    data_dir.join("index").open("w") do |f|
       f.write(JSON.dump({}))
     end
 
@@ -23,7 +23,7 @@ describe Vagrant::MachineIndex do
   end
 
   it "raises an exception if the JSON is invalid" do
-    data_file.open("w") do |f|
+    data_dir.join("index").open("w") do |f|
       f.write("foo")
     end
 
@@ -31,7 +31,7 @@ describe Vagrant::MachineIndex do
       to raise_error(Vagrant::Errors::CorruptMachineIndex)
   end
 
-  describe "#[]" do
+  describe "#get and #release" do
     before do
       data = {
         "version" => 1,
@@ -46,17 +46,17 @@ describe Vagrant::MachineIndex do
         }
       }
 
-      data_file.open("w") do |f|
+      data_dir.join("index").open("w") do |f|
         f.write(JSON.dump(data))
       end
     end
 
     it "returns nil if the machine doesn't exist" do
-      expect(subject["foo"]).to be_nil
+      expect(subject.get("foo")).to be_nil
     end
 
     it "returns a valid entry if the machine exists" do
-      result = subject["bar"]
+      result = subject.get("bar")
 
       expect(result.id).to eq("bar")
       expect(result.name).to eq("default")
@@ -65,22 +65,47 @@ describe Vagrant::MachineIndex do
       expect(result.state).to eq("running")
       expect(result.updated_at).to eq("foo")
     end
+
+    it "locks the entry so subsequent gets fail" do
+      result = subject.get("bar")
+      expect(result).to_not be_nil
+
+      expect { subject.get("bar") }.
+        to raise_error(Vagrant::Errors::MachineLocked)
     end
 
-  describe "#set and #[]" do
+    it "can unlock a machine" do
+      result = subject.get("bar")
+      expect(result).to_not be_nil
+      subject.release(result)
+
+      result = subject.get("bar")
+      expect(result).to_not be_nil
+    end
+  end
+
+  describe "#set and #get" do
     let(:entry_klass) { Vagrant::MachineIndex::Entry }
 
-    it "adds a new entry" do
-      entry = entry_klass.new
-      entry.name = "foo"
-      entry.vagrantfile_path = "/bar"
+    let(:new_entry) do
+      entry_klass.new.tap do |e|
+        e.name = "foo"
+        e.vagrantfile_path = "/bar"
+      end
+    end
 
-      result = subject.set(entry)
+    it "adds a new entry" do
+      result = subject.set(new_entry)
       expect(result.id).to_not be_empty
 
+      # It should be locked
+      expect { subject.get(result.id) }.
+        to raise_error(Vagrant::Errors::MachineLocked)
+
       # Get it froma new class and check the results
-      subject = described_class.new(data_file)
-      entry   = subject[result.id]
+      subject.release(result)
+      subject = described_class.new(data_dir)
+      entry   = subject.get(result.id)
       expect(entry).to_not be_nil
       expect(entry.name).to eq("foo")
 
@@ -100,9 +125,12 @@ describe Vagrant::MachineIndex do
       nextresult = subject.set(result)
       expect(nextresult.id).to eq(result.id)
 
+      # Release it so we can test the contents
+      subject.release(nextresult)
+
       # Get it froma new class and check the results
-      subject = described_class.new(data_file)
-      entry   = subject[result.id]
+      subject = described_class.new(data_dir)
+      entry   = subject.get(result.id)
       expect(entry).to_not be_nil
       expect(entry.name).to eq("bar")
     end