Add tests for things that have created the lockfile but not yet acquired the lock

2 files changed, 109 insertions, 42 deletions

diff --git a/lib/chef/run_lock.rb b/lib/chef/run_lock.rb
index cefe637db6..9e0952bdcb 100644
--- a/lib/chef/run_lock.rb
+++ b/lib/chef/run_lock.rb
@@ -87,27 +87,8 @@ class Chef
     # Either acquire() or test() methods should be called in order to
     # get the ownership of run_lock.
     def test
-      # ensure the runlock_file path exists
-      create_path(File.dirname(runlock_file))
-      @runlock = File.open(runlock_file,'a+')
-
-      if Chef::Platform.windows?
-        acquire_win32_mutex
-      else
-        # If we support FD_CLOEXEC, then use it.
-        # NB: ruby-2.0.0-p195 sets FD_CLOEXEC by default, but not
-        # ruby-1.8.7/1.9.3
-        if Fcntl.const_defined?('F_SETFD') && Fcntl.const_defined?('FD_CLOEXEC')
-          runlock.fcntl(Fcntl::F_SETFD, runlock.fcntl(Fcntl::F_GETFD, 0) | Fcntl::FD_CLOEXEC)
-        end
-        # Flock will return 0 if it can acquire the lock otherwise it
-        # will return false
-        if runlock.flock(File::LOCK_NB|File::LOCK_EX) == 0
-          true
-        else
-          false
-        end
-      end
+      create_lock
+      acquire_lock
     end
 
     #
@@ -147,6 +128,34 @@ class Chef
       end
     end
 
+    # @api private solely for race condition tests
+    def create_lock
+      # ensure the runlock_file path exists
+      create_path(File.dirname(runlock_file))
+      @runlock = File.open(runlock_file,'a+')
+    end
+
+    # @api private solely for race condition tests
+    def acquire_lock
+      if Chef::Platform.windows?
+        acquire_win32_mutex
+      else
+        # If we support FD_CLOEXEC, then use it.
+        # NB: ruby-2.0.0-p195 sets FD_CLOEXEC by default, but not
+        # ruby-1.8.7/1.9.3
+        if Fcntl.const_defined?('F_SETFD') && Fcntl.const_defined?('FD_CLOEXEC')
+          runlock.fcntl(Fcntl::F_SETFD, runlock.fcntl(Fcntl::F_GETFD, 0) | Fcntl::FD_CLOEXEC)
+        end
+        # Flock will return 0 if it can acquire the lock otherwise it
+        # will return false
+        if runlock.flock(File::LOCK_NB|File::LOCK_EX) == 0
+          true
+        else
+          false
+        end
+      end
+    end
+
     private
 
     def reset
diff --git a/spec/functional/run_lock_spec.rb b/spec/functional/run_lock_spec.rb
index 4aee91279c..b66bc86f65 100644
--- a/spec/functional/run_lock_spec.rb
+++ b/spec/functional/run_lock_spec.rb
@@ -85,6 +85,55 @@ describe Chef::RunLock do
     end
 
     context "when the lockfile does not already exist" do
+      context "when a client creates the lockfile but has not yet acquired the lock" do
+        before { p1.run_to("created lock") }
+        shared_context "second client gets the lock" do
+          it "the lockfile is created" do
+            log_event("lockfile exists? #{File.exist?(lockfile)}")
+            expect(File.exist?(lockfile)).to be_truthy
+          end
+
+          it "the lockfile is not locked" do
+            run_lock = Chef::RunLock.new(lockfile)
+            begin
+              expect(run_lock.test).to be_truthy
+            ensure
+              run_lock.release
+            end
+          end
+
+          it "the lockfile is empty" do
+            expect(IO.read(lockfile)).to eq('')
+          end
+
+          context "and a second client gets the lock" do
+            before { p2.run_to("acquired lock") }
+            it "the first client does not get the lock until the second finishes" do
+              p1.run_to("acquired lock") do
+                p2.run_to_completion
+              end
+            end
+            it "and the first client tries to get the lock and the second is killed, the first client gets the lock immediately" do
+              p1.run_to("acquired lock") do
+                sleep BREATHING_ROOM
+                expect(p1.last_event).to match(/after (started|created lock)/)
+                p2.stop
+              end
+              p1.run_to_completion
+            end
+          end
+        end
+
+        context "and the second client has done nothing" do
+          include_context "second client gets the lock"
+        end
+
+        context "and the second client has created the lockfile but not yet acquired the lock" do
+          before { p2.run_to("created lock") }
+          include_context "second client gets the lock"
+        end
+      end
+
       context "when a client acquires the lock but has not yet saved the pid" do
         before { p1.run_to("acquired lock") }
 
@@ -116,7 +165,7 @@ describe Chef::RunLock do
           p2.run_to("acquired lock") do
             # While p2 is trying to acquire, wait a bit and then let p1 complete
             sleep(BREATHING_ROOM)
-            expect(p2.last_event).to eq("started").or match(/from started to/)
+            expect(p2.last_event).to match(/after (started|created lock)/)
             p1.run_to_completion
           end
 
@@ -126,7 +175,7 @@ describe Chef::RunLock do
         it "and a second client tries to get the lock and the first is killed, the second client gets the lock immediately" do
           p2.run_to("acquired lock") do
             sleep BREATHING_ROOM
-            expect(p2.last_event).to eq("started").or match(/from started to/)
+            expect(p2.last_event).to match(/after (started|created lock)/)
             p1.stop
           end
           p2.run_to_completion
@@ -163,7 +212,7 @@ describe Chef::RunLock do
           p2.run_to("acquired lock") do
             # While p2 is trying to acquire, wait a bit and then let p1 complete
             sleep(BREATHING_ROOM)
-            expect(p2.last_event).to eq("started").or match(/from started to/)
+            expect(p2.last_event).to match(/after (started|created lock)/)
             p1.run_to_completion
           end
 
@@ -173,7 +222,7 @@ describe Chef::RunLock do
         it "when a second client tries to get the lock and the first is killed, the second client gets the lock immediately" do
           p2.run_to("acquired lock") do
             sleep BREATHING_ROOM
-            expect(p2.last_event).to eq("started").or match(/from started to/)
+            expect(p2.last_event).to match(/after (started|created lock)/)
             p1.stop
           end
           p2.run_to_completion
@@ -297,7 +346,7 @@ describe Chef::RunLock do
 
       # Wait until it gets there
       Timeout::timeout(CLIENT_PROCESS_TIMEOUT) do
-        until @last_event == to_event
+        until @last_event == "after #{to_event}"
           got_event, time = read_from_process.gets.split("@")
           example.log_event("#{name}.last_event got #{got_event}")
           example.log_event("[#{name}] #{got_event}", time.strip)
@@ -346,6 +395,20 @@ describe Chef::RunLock do
       end
     end
 
+    def fire_event(event)
+      # Let the caller know what event we've reached
+      write_to_tests.puts("after #{event}@#{Time.now.strftime("%H:%M:%S.%L")}")
+
+      # Block until the client tells us where to stop
+      if !@run_to_event || event == @run_to_event
+        write_to_tests.puts("waiting for instructions after #{event}@#{Time.now.strftime("%H:%M:%S.%L")}")
+        @run_to_event = read_from_tests.gets.strip
+        write_to_tests.puts("told to run to #{@run_to_event} after #{event}@#{Time.now.strftime("%H:%M:%S.%L")}")
+      elsif @run_to_event
+        write_to_tests.puts("continuing until #{@run_to_event} after #{event}@#{Time.now.strftime("%H:%M:%S.%L")}")
+      end
+    end
+
     private
 
     attr_reader :read_from_process
@@ -353,12 +416,21 @@ describe Chef::RunLock do
     attr_reader :read_from_tests
     attr_reader :write_to_process
 
+    class TestRunLock < Chef::RunLock
+      attr_accessor :client_process
+      def create_lock
+        super
+        client_process.fire_event("created lock")
+      end
+    end
+
     def start
       example.log_event("#{name}.start")
       @pid = fork do
         begin
           Timeout::timeout(CLIENT_PROCESS_TIMEOUT) do
-            run_lock = Chef::RunLock.new(example.lockfile)
+            run_lock = TestRunLock.new(example.lockfile)
+            run_lock.client_process = self
             fire_event("started")
             run_lock.acquire
             fire_event("acquired lock")
@@ -367,27 +439,13 @@ describe Chef::RunLock do
             exit!(0)
           end
         rescue
-          fire_event(e.message.lines.join(" // "))
+          fire_event($!.message.lines.join(" // "))
           raise
         end
       end
       example.log_event("#{name}.start forked (pid #{pid})")
     end
 
-    def fire_event(event)
-      # Let the caller know what event we've reached
-      write_to_tests.puts("#{event}@#{Time.now.strftime("%H:%M:%S.%L")}")
-
-      # Block until the client tells us where to stop
-      if !@run_to_event || event == @run_to_event
-        write_to_tests.puts("waiting for instructions #{event} to ?@#{Time.now.strftime("%H:%M:%S.%L")}")
-        @run_to_event = read_from_tests.gets.strip
-        write_to_tests.puts("told to run from #{event} to #{@run_to_event}@#{Time.now.strftime("%H:%M:%S.%L")}")
-      elsif @run_to_event
-        write_to_tests.puts("continuing from #{event} to #{@run_to_event}@#{Time.now.strftime("%H:%M:%S.%L")}")
-      end
-    end
-
     def readline_nonblock(fd)
       buffer = ""
       buffer << fd.read_nonblock(1) while buffer[-1] != "\n"