From 090ca84cfeeebefc2ee77144e1bbde49b730901a Mon Sep 17 00:00:00 2001 From: Mike McQuaid Date: Tue, 30 Jul 2024 13:50:15 +0100 Subject: [PATCH] Improve locking UX My experience recently playing around with our locking behaviour is that, while mostly seamless and not seen by users, it's leaks implementation details a bit too heavily. As a result, the following improvements are in this commit: - Ensure that, whenever possible, we tell the user the actual command that is holding a given lock instead of the lock name (an internal implementation detail) - Make the locking error output a little more consistent and user friendly - Add a `DownloadLock` class to simplify locking downloads - Add a `HOMEBREW_LOCK_CONTEXT` variable to allow adding additional context for logging error messages - Lock paths and leave deciding how this translates to lock names up to the locking code itself - Lock the Cellar/Caskroom paths explicitly rather than implicitly --- Library/Homebrew/brew.rb | 2 ++ Library/Homebrew/cleanup.rb | 2 +- Library/Homebrew/cmd/vendor-install.sh | 2 +- Library/Homebrew/download_strategy.rb | 2 +- Library/Homebrew/env_config.rb | 3 +++ Library/Homebrew/exceptions.rb | 10 ++++++--- Library/Homebrew/global.rb | 10 +++++++++ Library/Homebrew/lock_file.rb | 30 ++++++++++++++++++-------- Library/Homebrew/utils/lock.sh | 29 ++++++++++++++++--------- 9 files changed, 65 insertions(+), 25 deletions(-) diff --git a/Library/Homebrew/brew.rb b/Library/Homebrew/brew.rb index 16059f829cf12e..114f034dd3c6a9 100644 --- a/Library/Homebrew/brew.rb +++ b/Library/Homebrew/brew.rb @@ -87,6 +87,7 @@ if internal_cmd || Commands.external_ruby_v2_cmd_path(cmd) cmd = T.must(cmd) cmd_class = Homebrew::AbstractCommand.command(cmd) + Homebrew.running_command = cmd if cmd_class command_instance = cmd_class.new @@ -97,6 +98,7 @@ Homebrew.public_send Commands.method_name(cmd) end elsif (path = Commands.external_ruby_cmd_path(cmd)) + Homebrew.running_command = cmd require?(path) exit Homebrew.failed? ? 1 : 0 elsif Commands.external_cmd_path(cmd) diff --git a/Library/Homebrew/cleanup.rb b/Library/Homebrew/cleanup.rb index e28bff8d7ab74e..da64f43ad6946c 100644 --- a/Library/Homebrew/cleanup.rb +++ b/Library/Homebrew/cleanup.rb @@ -411,7 +411,7 @@ def cleanup_unreferenced_downloads (downloads - referenced_downloads).each do |download| if self.class.incomplete?(download) begin - LockFile.new(download.basename).with_lock do + DownloadLock.new(download).with_lock do download.unlink end rescue OperationInProgressError diff --git a/Library/Homebrew/cmd/vendor-install.sh b/Library/Homebrew/cmd/vendor-install.sh index 5a03a016292d9e..9fa5728d9dcf06 100644 --- a/Library/Homebrew/cmd/vendor-install.sh +++ b/Library/Homebrew/cmd/vendor-install.sh @@ -359,7 +359,7 @@ homebrew-vendor-install() { CACHED_LOCATION="${HOMEBREW_CACHE}/${VENDOR_FILENAME}" - lock "vendor-install-${VENDOR_NAME}" + lock "vendor-install ${VENDOR_NAME}" fetch install diff --git a/Library/Homebrew/download_strategy.rb b/Library/Homebrew/download_strategy.rb index 2175015d43d2d8..2b3e17dc2d4a98 100644 --- a/Library/Homebrew/download_strategy.rb +++ b/Library/Homebrew/download_strategy.rb @@ -395,7 +395,7 @@ def initialize(url, name, version, **meta) def fetch(timeout: nil) end_time = Time.now + timeout if timeout - download_lock = LockFile.new(temporary_path.basename) + download_lock = DownloadLock.new(temporary_path.basename) download_lock.lock urls = [url, *mirrors] diff --git a/Library/Homebrew/env_config.rb b/Library/Homebrew/env_config.rb index 7e22af45f9eada..d01571f0078065 100644 --- a/Library/Homebrew/env_config.rb +++ b/Library/Homebrew/env_config.rb @@ -295,6 +295,9 @@ module EnvConfig "or `$HOME/.homebrew/livecheck_watchlist.txt` otherwise.", default: "#{ENV.fetch("HOMEBREW_USER_CONFIG_HOME")}/livecheck_watchlist.txt", }, + HOMEBREW_LOCK_CONTEXT: { + description: "If set, Homebrew will add this output as additional context for locking errors.", + }, HOMEBREW_LOGS: { description: "Use this directory to store log files.", default_text: "macOS: `$HOME/Library/Logs/Homebrew`, " \ diff --git a/Library/Homebrew/exceptions.rb b/Library/Homebrew/exceptions.rb index 8671f3084e604f..de9b1ebbd3e78e 100644 --- a/Library/Homebrew/exceptions.rb +++ b/Library/Homebrew/exceptions.rb @@ -359,10 +359,14 @@ def initialize(name) # Raised when another Homebrew operation is already in progress. class OperationInProgressError < RuntimeError - def initialize(name) + sig { params(locked_path: Pathname).void } + def initialize(locked_path) + full_command = Homebrew.running_command_with_args.presence || "brew" + lock_context = if (env_lock_context = Homebrew::EnvConfig.lock_context.presence) + "\n#{env_lock_context}" + end message = <<~EOS - Operation already in progress for #{name} - Another active Homebrew process is already using #{name}. + A `#{full_command}` process has already locked #{locked_path}.#{lock_context} Please wait for it to finish or terminate it to continue. EOS diff --git a/Library/Homebrew/global.rb b/Library/Homebrew/global.rb index 9191f7d463f471..9ac55d5fedbfb2 100644 --- a/Library/Homebrew/global.rb +++ b/Library/Homebrew/global.rb @@ -110,6 +110,16 @@ def running_as_root_but_not_owned_by_root? def auto_update_command? ENV.fetch("HOMEBREW_AUTO_UPDATE_COMMAND", false).present? end + + sig { params(cmd: T.nilable(String)).void } + def running_command=(cmd) + @running_command_with_args = "#{cmd} #{ARGV.join(" ")}" + end + + sig { returns String } + def running_command_with_args + "brew #{@running_command_with_args}".strip + end end end diff --git a/Library/Homebrew/lock_file.rb b/Library/Homebrew/lock_file.rb index 73adbb0c51837f..93b8d0498ba7cc 100644 --- a/Library/Homebrew/lock_file.rb +++ b/Library/Homebrew/lock_file.rb @@ -3,13 +3,15 @@ require "fcntl" -# A lock file. +# A lock file to prevent multiple Homebrew processes from modifying the same path. class LockFile attr_reader :path - def initialize(name) - @name = name.to_s - @path = HOMEBREW_LOCKS/"#{@name}.lock" + sig { params(type: Symbol, locked_path: Pathname).void } + def initialize(type, locked_path) + @locked_path = locked_path + lock_name = locked_path.basename.to_s + @path = HOMEBREW_LOCKS/"#{lock_name}.#{type}.lock" @lockfile = nil end @@ -18,7 +20,7 @@ def lock create_lockfile return if @lockfile.flock(File::LOCK_EX | File::LOCK_NB) - raise OperationInProgressError, @name + raise OperationInProgressError, @locked_path end def unlock @@ -51,14 +53,24 @@ def create_lockfile # A lock file for a formula. class FormulaLock < LockFile - def initialize(name) - super("#{name}.formula") + sig { params(rack_name: String).void } + def initialize(rack_name) + super(:formula, HOMEBREW_CELLAR/rack_name) end end # A lock file for a cask. class CaskLock < LockFile - def initialize(name) - super("#{name}.cask") + sig { params(cask_token: String).void } + def initialize(cask_token) + super(:cask, HOMEBREW_CASKROOM/cask_token) + end +end + +# A lock file for a download. +class DownloadLock < LockFile + sig { params(download_path: Pathname).void } + def initialize(download_path) + super(:download, download_path) end end diff --git a/Library/Homebrew/utils/lock.sh b/Library/Homebrew/utils/lock.sh index 17e43e00e6c54e..49ccaa1120b2fc 100644 --- a/Library/Homebrew/utils/lock.sh +++ b/Library/Homebrew/utils/lock.sh @@ -1,18 +1,20 @@ -# create a lock using `flock(2)`. A name is required as first argument. -# the lock will be automatically unlocked when the shell process quits. -# Noted due to the fixed FD, a shell process can only create one lock. +# Create a lock using `flock(2)`. A command name with arguments is required as +# first argument. The lock will be automatically unlocked when the shell process +# quits. Note due to the fixed FD, a shell process can only create one lock. # HOMEBREW_LIBRARY is by brew.sh # HOMEBREW_PREFIX is set by extend/ENV/super.rb # shellcheck disable=SC2154 lock() { - local name="$1" + local command_name_and_args="$1" + # use bash to replace spaces with dashes + local lock_filename="${command_name_and_args// /-}" local lock_dir="${HOMEBREW_PREFIX}/var/homebrew/locks" - local lock_file="${lock_dir}/${name}" + local lock_file="${lock_dir}/${lock_filename}" [[ -d "${lock_dir}" ]] || mkdir -p "${lock_dir}" if [[ ! -w "${lock_dir}" ]] then odie <&- # open the lock file to FD, so the shell process can hold the lock. exec 200>"${lock_file}" - if ! _create_lock 200 "${name}" + + if ! _create_lock 200 "${command_name_and_args}" then + local lock_context + if [[ -n "${HOMEBREW_LOCK_CONTEXT}" ]] + then + lock_context="\n${HOMEBREW_LOCK_CONTEXT}" + fi + odie <