From 4bd75d4235626f872610bacc1a9cbef6fcc74b08 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Sat, 23 Nov 2024 23:45:17 +0100 Subject: [PATCH] Fix incorrect download locking. --- Library/Homebrew/download_strategy.rb | 97 +++++++++++++------------ Library/Homebrew/lock_file.rb | 29 ++++++-- Library/Homebrew/test/lock_file_spec.rb | 29 +++++++- 3 files changed, 96 insertions(+), 59 deletions(-) diff --git a/Library/Homebrew/download_strategy.rb b/Library/Homebrew/download_strategy.rb index 09858f5f69..eb5774106a 100644 --- a/Library/Homebrew/download_strategy.rb +++ b/Library/Homebrew/download_strategy.rb @@ -392,60 +392,63 @@ class CurlDownloadStrategy < AbstractFileDownloadStrategy end_time = Time.now + timeout if timeout download_lock = DownloadLock.new(temporary_path) - download_lock.lock - - urls = [url, *mirrors] - begin - url = urls.shift + download_lock.lock - if (domain = Homebrew::EnvConfig.artifact_domain) - url = url.sub(%r{^https?://#{GitHubPackages::URL_DOMAIN}/}o, "#{domain.chomp("/")}/") - urls = [] if Homebrew::EnvConfig.artifact_domain_no_fallback? - end + urls = [url, *mirrors] - ohai "Downloading #{url}" + begin + url = urls.shift - use_cached_location = cached_location.exist? - use_cached_location = false if version.respond_to?(:latest?) && version.latest? - - resolved_url, _, last_modified, _, is_redirection = begin - resolve_url_basename_time_file_size(url, timeout: Utils::Timer.remaining!(end_time)) - rescue ErrorDuringExecution - raise unless use_cached_location - end - - # Authorization is no longer valid after redirects - meta[:headers]&.delete_if { |header| header.start_with?("Authorization") } if is_redirection - - # The cached location is no longer fresh if Last-Modified is after the file's timestamp - use_cached_location = false if cached_location.exist? && last_modified && last_modified > cached_location.mtime - - if use_cached_location - puts "Already downloaded: #{cached_location}" - else - begin - _fetch(url:, resolved_url:, timeout: Utils::Timer.remaining!(end_time)) - rescue ErrorDuringExecution - raise CurlDownloadStrategyError, url + if (domain = Homebrew::EnvConfig.artifact_domain) + url = url.sub(%r{^https?://#{GitHubPackages::URL_DOMAIN}/}o, "#{domain.chomp("/")}/") + urls = [] if Homebrew::EnvConfig.artifact_domain_no_fallback? end - cached_location.dirname.mkpath - temporary_path.rename(cached_location) + + ohai "Downloading #{url}" + + use_cached_location = cached_location.exist? + use_cached_location = false if version.respond_to?(:latest?) && version.latest? + + resolved_url, _, last_modified, _, is_redirection = begin + resolve_url_basename_time_file_size(url, timeout: Utils::Timer.remaining!(end_time)) + rescue ErrorDuringExecution + raise unless use_cached_location + end + + # Authorization is no longer valid after redirects + meta[:headers]&.delete_if { |header| header.start_with?("Authorization") } if is_redirection + + # The cached location is no longer fresh if Last-Modified is after the file's timestamp + if cached_location.exist? && last_modified && last_modified > cached_location.mtime + use_cached_location = false + end + + if use_cached_location + puts "Already downloaded: #{cached_location}" + else + begin + _fetch(url:, resolved_url:, timeout: Utils::Timer.remaining!(end_time)) + rescue ErrorDuringExecution + raise CurlDownloadStrategyError, url + end + cached_location.dirname.mkpath + temporary_path.rename(cached_location) + end + + symlink_location.dirname.mkpath + FileUtils.ln_s cached_location.relative_path_from(symlink_location.dirname), symlink_location, force: true + rescue CurlDownloadStrategyError + raise if urls.empty? + + puts "Trying a mirror..." + retry + rescue Timeout::Error => e + raise Timeout::Error, "Timed out downloading #{self.url}: #{e}" end - - symlink_location.dirname.mkpath - FileUtils.ln_s cached_location.relative_path_from(symlink_location.dirname), symlink_location, force: true - rescue CurlDownloadStrategyError - raise if urls.empty? - - puts "Trying a mirror..." - retry - rescue Timeout::Error => e - raise Timeout::Error, "Timed out downloading #{self.url}: #{e}" + ensure + download_lock.unlock(unlink: true) end - ensure - download_lock&.unlock - download_lock&.path&.unlink end def clear_cache diff --git a/Library/Homebrew/lock_file.rb b/Library/Homebrew/lock_file.rb index 3f37bddaaf..cad31b80ae 100644 --- a/Library/Homebrew/lock_file.rb +++ b/Library/Homebrew/lock_file.rb @@ -15,19 +15,30 @@ class LockFile @lockfile = nil end + sig { void } def lock - @path.parent.mkpath - create_lockfile - return if @lockfile.flock(File::LOCK_EX | File::LOCK_NB) + ignore_interrupts do + create_lockfile - raise OperationInProgressError, @locked_path + next if @lockfile.flock(File::LOCK_EX | File::LOCK_NB) + + raise OperationInProgressError, @locked_path + end end - def unlock - return if @lockfile.nil? || @lockfile.closed? + sig { params(unlink: T::Boolean).void } + def unlock(unlink: false) + ignore_interrupts do + next if @lockfile.nil? || @lockfile.closed? - @lockfile.flock(File::LOCK_UN) - @lockfile.close + @lockfile.flock(File::LOCK_UN) + @lockfile.close + + if unlink + @path.unlink if @path.exist? + @lockfile = nil + end + end end def with_lock @@ -42,6 +53,8 @@ class LockFile def create_lockfile return if @lockfile.present? && !@lockfile.closed? + @path.dirname.mkpath + begin @lockfile = @path.open(File::RDWR | File::CREAT) rescue Errno::EMFILE diff --git a/Library/Homebrew/test/lock_file_spec.rb b/Library/Homebrew/test/lock_file_spec.rb index cb82d3eda2..7f141e4463 100644 --- a/Library/Homebrew/test/lock_file_spec.rb +++ b/Library/Homebrew/test/lock_file_spec.rb @@ -5,18 +5,26 @@ require "lock_file" RSpec.describe LockFile do subject(:lock_file) { described_class.new(:lock, Pathname("foo")) } + let(:lock_file_copy) { described_class.new(:lock, Pathname("foo")) } + describe "#lock" do - it "does not raise an error when already locked" do + it "ensures the lock file is created" do + expect(lock_file.path).not_to exist + lock_file.lock + expect(lock_file.path).to exist + end + + it "does not raise an error when the same instance is locked multiple times" do lock_file.lock expect { lock_file.lock }.not_to raise_error end - it "raises an error if a lock already exists" do + it "raises an error if another instance is already locked" do lock_file.lock expect do - described_class.new(:lock, Pathname("foo")).lock + lock_file_copy.lock end.to raise_error(OperationInProgressError) end end @@ -30,7 +38,20 @@ RSpec.describe LockFile do lock_file.lock lock_file.unlock - expect { described_class.new(:lock, Pathname("foo")).lock }.not_to raise_error + expect { lock_file_copy.lock }.not_to raise_error + end + + it "allows deleting a lock file only by the instance that locked it" do + lock_file.lock + expect(lock_file.path).to exist + + expect(lock_file_copy.path).to exist + lock_file_copy.unlock(unlink: true) + expect(lock_file_copy.path).to exist + expect(lock_file.path).to exist + + lock_file.unlock(unlink: true) + expect(lock_file.path).not_to exist end end end