[ruby/rubygems] Fix file locks letting multiple processes in when lock files are removed

Gem.open_file_with_lock removed its lock file after releasing the lock.
Since flock locks the file itself rather than its path, a process that
was waiting on the old file could acquire it after it had been removed,
while a newly arriving process created a fresh lock file and locked it
immediately, so both ran at the same time.

Remove the lock file while still holding the lock, and after acquiring
it, check that the locked file is still the one at the lock path,
retrying otherwise.

https://github.com/ruby/rubygems/commit/9a93c235e7

lib/bundler/rubygems_ext.rb

@@ -86,11 +86,22 @@ module Gem
         end
       end
 
-      def open_file_with_lock(path, &block)
+      def open_file_with_lock(path)
         file_lock = "#{path}.lock"
-        open_file_with_flock(file_lock, &block)
-      ensure
-        FileUtils.rm_f file_lock
+
+        loop do
+          open_file_with_flock(file_lock) do |io|
+            # The previous holder removed the file while we were waiting on it,
+            # so leave this block and try again on the file now at the lock path
+            next unless File.identical?(io, file_lock)
+
+            begin
+              return yield io
+            ensure
+              FileUtils.rm_f file_lock
+            end
+          end
+        end
       end
     end
   end

lib/rubygems.rb

@@ -868,14 +868,30 @@ An Array (#{env.inspect}) was passed in from #{caller[3]}
   end
 
   ##
-  # Open a file with given flags, and protect access with a file lock
+  # Open a file with given flags, and protect access with a file lock.
+  #
+  # The lock file is removed once done, while still holding the lock. Since
+  # processes waiting for the lock may end up acquiring it on the removed file,
+  # after acquiring it we check that the locked file is still the one at the
+  # lock path, and start over otherwise.
 
-  def self.open_file_with_lock(path, &block)
+  def self.open_file_with_lock(path)
     file_lock = "#{path}.lock"
-    open_file_with_flock(file_lock, &block)
-  ensure
-    require "fileutils"
-    FileUtils.rm_f file_lock
+
+    loop do
+      open_file_with_flock(file_lock) do |io|
+        # The previous holder removed the file while we were waiting on it, so
+        # leave this block and try again on the file now at the lock path
+        next unless File.identical?(io, file_lock)
+
+        begin
+          return yield io
+        ensure
+          require "fileutils"
+          FileUtils.rm_f file_lock
+        end
+      end
+    end
   end
 
   ##

spec/bundler/install/process_lock_spec.rb

@@ -21,6 +21,52 @@ RSpec.describe "process lock spec" do
       expect(the_bundle).to include_gems "myrack 1.0"
     end
 
+    it "keeps excluding late arrivals after the lock is handed over to a waiting process" do
+      events = Queue.new
+      release_waiter = Queue.new
+
+      # The warning printed while blocking on the lock is the only observable
+      # sign that a contender has opened the lock file and is waiting on it
+      allow(Gem).to receive(:warn) do |message|
+        events << [Thread.current.name, :waiting] if message.include?("Waiting for another process")
+      end
+
+      next_event = lambda do
+        events.pop(timeout: 10) || raise("Timed out waiting for a lock event")
+      end
+
+      contender = lambda do |name, &block|
+        Thread.new do
+          Thread.current.name = name
+          Thread.current.abort_on_exception = true
+          Bundler::ProcessLock.lock(default_bundle_path) do
+            events << [name, :locked]
+            block&.call
+          end
+        end
+      end
+
+      waiter = nil
+      Bundler::ProcessLock.lock(default_bundle_path) do
+        waiter = contender.call("waiter") { release_waiter.pop }
+        expect(next_event.call).to eq(["waiter", :waiting])
+      end
+      expect(next_event.call).to eq(["waiter", :locked])
+
+      # Arrives only after the original holder is gone, while the waiter is
+      # still inside its critical section
+      late_arrival = contender.call("late_arrival")
+      expect(next_event.call).to eq(["late_arrival", :waiting])
+
+      release_waiter << true
+      [waiter, late_arrival].each(&:join)
+      expect(next_event.call).to eq(["late_arrival", :locked])
+      expect(default_bundle_path("bundler.lock")).not_to exist
+    ensure
+      # Never leave the waiter blocked while holding the lock
+      release_waiter << true
+    end
+
     context "when creating a lock raises Errno::ENOTSUP" do
       before { allow(File).to receive(:open).and_raise(Errno::ENOTSUP) }
 

test/rubygems/test_gem.rb

@@ -941,6 +941,51 @@ class TestGem < Gem::TestCase
     RbConfig::CONFIG["sitelibdir"] = orig_sitelibdir
   end
 
+  def test_self_open_file_with_lock_excludes_late_arrivals_after_handover
+    path = File.join(@tempdir, "resource")
+    events = Queue.new
+    release_waiter = Queue.new
+
+    on_warn = lambda do |message|
+      events << [Thread.current.name, :waiting] if message.include?("Waiting for another process")
+    end
+
+    next_event = lambda do
+      events.pop(timeout: 10) || flunk("Timed out waiting for a lock event")
+    end
+
+    contender = lambda do |name, &block|
+      Thread.new do
+        Thread.current.name = name
+        Thread.current.abort_on_exception = true
+        Gem.open_file_with_lock(path) do
+          events << [name, :locked]
+          block&.call
+        end
+      end
+    end
+
+    Gem.stub(:warn, on_warn) do
+      waiter = nil
+      Gem.open_file_with_lock(path) do
+        waiter = contender.call("waiter") { release_waiter.pop }
+        assert_equal ["waiter", :waiting], next_event.call
+      end
+      assert_equal ["waiter", :locked], next_event.call
+
+      late_arrival = contender.call("late_arrival")
+      assert_equal ["late_arrival", :waiting], next_event.call
+
+      release_waiter << true
+      [waiter, late_arrival].each(&:join)
+      assert_equal ["late_arrival", :locked], next_event.call
+    ensure
+      release_waiter << true
+    end
+
+    assert_path_not_exist "#{path}.lock"
+  end
+
   def test_self_read_binary
     File.open "test", "w" do |io|
       io.write "\xCF\x80"