diff --git a/lib/bundler/rubygems_ext.rb b/lib/bundler/rubygems_ext.rb index d3c4ce6d9dc..f777e06c01e 100644 --- a/lib/bundler/rubygems_ext.rb +++ b/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 diff --git a/lib/rubygems.rb b/lib/rubygems.rb index 62b857d0868..e1031665aa2 100644 --- a/lib/rubygems.rb +++ b/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 ## diff --git a/spec/bundler/install/process_lock_spec.rb b/spec/bundler/install/process_lock_spec.rb index b096291d1a9..3bea0340bd4 100644 --- a/spec/bundler/install/process_lock_spec.rb +++ b/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) } diff --git a/test/rubygems/test_gem.rb b/test/rubygems/test_gem.rb index 88b461a2a0a..052a15aa638 100644 --- a/test/rubygems/test_gem.rb +++ b/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"