Merge pull request #4792 from mwrock/state_refresh_fix

fixes infinite loop in machine state call if provider calls machine.action

lib/vagrant/machine.rb

@@ -180,18 +180,7 @@ module Vagrant
         end
 
         # Call the action
-        result = action_raw(name, callable, extra_env)
+        action_raw(name, callable, extra_env)
-
-        # Call the state method so that we update our index state. Don't
-        # worry about exceptions here, since we just care about updating
-        # the cache.
-        begin
-          # Called for side effects
-          self.state
-        rescue Errors::VagrantError
-        end
-
-        return result
       end
     rescue Errors::EnvironmentLockedError
       raise Errors::MachineActionLockedError,

lib/vagrant/plugin/v2/command.rb

@@ -224,6 +224,15 @@ module Vagrant
 
             @logger.info("With machine: #{machine.name} (#{machine.provider.inspect})")
             yield machine
+
+            # Call the state method so that we update our index state. Don't
+            # worry about exceptions here, since we just care about updating
+            # the cache.
+            begin
+              # Called for side effects
+              machine.state
+            rescue Errors::VagrantError
+            end
           end
         end
 

test/unit/vagrant/machine_test.rb

@@ -216,10 +216,6 @@ describe Vagrant::Machine do
   end
 
   describe "#action" do
-    before do
-      provider.stub(state: Vagrant::MachineState.new(:foo, "", ""))
-    end
-
     it "should be able to run an action that exists" do
       action_name = :up
       called      = false
@@ -270,17 +266,6 @@ describe Vagrant::Machine do
       expect { instance.action(action_name) }.
         to raise_error(Vagrant::Errors::UnimplementedProviderAction)
     end
-
-    it "calls #state to update the machine index" do
-      action_name = :up
-      called      = false
-      callable    = lambda { |_env| called = true }
-
-      expect(provider).to receive(:action).with(action_name).and_return(callable)
-      expect(instance).to receive(:state)
-      instance.action(:up)
-      expect(called).to be
-    end
   end
 
   describe "#action_raw" do

test/unit/vagrant/plugin/v2/command_test.rb

@@ -73,14 +73,16 @@ describe Vagrant::Plugin::V2::Command do
         to raise_error(Vagrant::Errors::NoEnvironmentError)
     end
 
-    it "should yield every VM in order is no name is given" do
+    it "should yield every VM in order if no name is given" do
       foo_vm = double("foo")
       foo_vm.stub(name: "foo", provider: :foobarbaz)
       foo_vm.stub(ui: Vagrant::UI::Silent.new)
+      foo_vm.stub(state: nil)
 
       bar_vm = double("bar")
       bar_vm.stub(name: "bar", provider: :foobarbaz)
       bar_vm.stub(ui: Vagrant::UI::Silent.new)
+      bar_vm.stub(state: nil)
 
       environment.stub(machine_names: [:foo, :bar])
       allow(environment).to receive(:machine).with(:foo, environment.default_provider).and_return(foo_vm)
@@ -106,6 +108,7 @@ describe Vagrant::Plugin::V2::Command do
       foo_vm = double("foo")
       foo_vm.stub(name: "foo", provider: :foobarbaz)
       foo_vm.stub(ui: Vagrant::UI::Silent.new)
+      foo_vm.stub(state: nil)
 
       allow(environment).to receive(:machine).with(:foo, environment.default_provider).and_return(foo_vm)
 
@@ -114,12 +117,26 @@ describe Vagrant::Plugin::V2::Command do
       expect(vms).to eq([foo_vm])
     end
 
+    it "calls state after yielding the vm to update the machine index" do
+      foo_vm = double("foo")
+      foo_vm.stub(name: "foo", provider: :foobarbaz)
+      foo_vm.stub(ui: Vagrant::UI::Silent.new)
+      foo_vm.stub(state: nil)
+
+      allow(environment).to receive(:machine).with(:foo, environment.default_provider).and_return(foo_vm)
+
+      vms = []
+      expect(foo_vm).to receive(:state)
+      instance.with_target_vms("foo") { |vm| vms << vm }
+    end
+
     it "yields the given VM with proper provider if given" do
       foo_vm = double("foo")
       provider = :foobarbaz
 
       foo_vm.stub(name: "foo", provider: provider)
       foo_vm.stub(ui: Vagrant::UI::Silent.new)
+      foo_vm.stub(state: nil)
       allow(environment).to receive(:machine).with(:foo, provider).and_return(foo_vm)
 
       vms = []
@@ -144,6 +161,7 @@ describe Vagrant::Plugin::V2::Command do
       allow(environment).to receive(:machine).with(name, provider).and_return(vmware_vm)
       vmware_vm.stub(name: name, provider: provider)
       vmware_vm.stub(ui: Vagrant::UI::Silent.new)
+      vmware_vm.stub(state: nil)
 
       vms = []
       instance.with_target_vms(name.to_s) { |vm| vms << vm }
@@ -158,6 +176,7 @@ describe Vagrant::Plugin::V2::Command do
       environment.stub(active_machines: [[name, provider]])
       allow(environment).to receive(:machine).with(name, provider).and_return(vmware_vm)
       vmware_vm.stub(name: name, provider: provider, ui: Vagrant::UI::Silent.new)
+      vmware_vm.stub(state: nil)
 
       vms = []
       instance.with_target_vms(name.to_s, provider: provider) { |vm| vms << vm }
@@ -171,6 +190,7 @@ describe Vagrant::Plugin::V2::Command do
       allow(environment).to receive(:machine).with(name, environment.default_provider).and_return(machine)
       machine.stub(name: name, provider: environment.default_provider)
       machine.stub(ui: Vagrant::UI::Silent.new)
+      machine.stub(state: nil)
 
       results = []
       instance.with_target_vms(name.to_s) { |m| results << m }
@@ -188,6 +208,7 @@ describe Vagrant::Plugin::V2::Command do
       environment.stub(primary_machine_name: name)
       vmware_vm.stub(name: name, provider: provider)
       vmware_vm.stub(ui: Vagrant::UI::Silent.new)
+      vmware_vm.stub(state: nil)
 
       vms = []
       instance.with_target_vms(nil, single_target: true) { |vm| vms << vm }
@@ -204,6 +225,7 @@ describe Vagrant::Plugin::V2::Command do
       environment.stub(primary_machine_name: name)
       machine.stub(name: name, provider: environment.default_provider)
       machine.stub(ui: Vagrant::UI::Silent.new)
+      machine.stub(state: nil)
 
       vms = []
       instance.with_target_vms(nil, single_target: true) { |vm| vms << machine }