diff --git a/devdocs/locking_and_update_flows.md b/devdocs/locking_and_update_flows.md new file mode 100644 index 000000000..5ee98ebf4 --- /dev/null +++ b/devdocs/locking_and_update_flows.md @@ -0,0 +1,217 @@ +# Configuration locking & update flows + +This document describes how juliaup serializes access to its configuration +(`juliaup.json`) and where the configuration lock is held during the various +launcher and update code paths. It exists primarily to make the locking +behaviour auditable, since holding the lock across network operations has +historically caused spurious "configuration is locked by another process" +stalls (see [#1524](https://github.com/JuliaLang/juliaup/issues/1524)). + +## The lock + +All access to the configuration is serialized through a single lock file +(`paths.lockfile`) using advisory file locks (`cluFlock`). There are two lock +modes: + +| Mode | Acquired by | Blocks | +| --- | --- | --- | +| **Shared** (read) | `get_read_lock` / `load_config_db(paths, None)` | only an exclusive holder | +| **Exclusive** (write) | `load_mut_config_db` | any other shared or exclusive holder | + +Many readers can hold the shared lock simultaneously. A single writer holding +the exclusive lock blocks everyone, including readers. The user-visible message + +> Juliaup configuration is locked by another process, waiting for it to unlock. + +is printed whenever a process cannot acquire the lock it wants within a short +grace period (1 second) and falls back to a blocking wait. The acquisition is +retried (polling) during the grace period, so the message is suppressed for the +common case where another process holds the lock for only a few milliseconds +(e.g. while committing a config change). Both `get_read_lock` and +`load_mut_config_db` route through `lock_with_delayed_message` to get this +behaviour. + +### Design rule + +> **Never hold the exclusive lock across a network operation.** + +Downloads can be slow or hang. A writer that holds the exclusive lock across a +download blocks every concurrent `julia` / `juliaup` invocation (each of which +needs at least a shared lock at startup) for the entire duration of the +download. The commands below follow a common pattern to honour this rule: + +```mermaid +flowchart LR + A["shared lock
(read snapshot)"] --> B["release lock"] + B --> C["download / extract
NO lock held"] + C --> D["exclusive lock
(commit: rename + config write)"] + D --> E["release lock"] +``` + +The download phase produces files in a temporary directory; the commit phase +re-checks the configuration (optimistic concurrency) and atomically renames the +result into place while holding the exclusive lock only briefly. + +## `julialauncher` (the `julia` shim) + +Every `julia` invocation runs the launcher, which reads the configuration and +then spawns a background worker that may trigger `juliaup self update` and the +version-db update. On *nix the worker is a double-forked daemon; on Windows it +runs inline before waiting on the Julia child. + +```mermaid +flowchart TD + start(["julia +channel ..."]) --> setup["do_initial_setup"] + setup --> read["load_config_db(None)
shared lock → released"] + read --> resolve["resolve channel → julia_path"] + resolve --> fork{"platform"} + + fork -->|"*nix"| forkp["fork()"] + forkp --> parent["parent: exec() into Julia
(replaces process)"] + forkp --> child["double-forked daemon"] + child --> vdb["run_versiondb_update"] + child --> su["run_selfupdate"] + + fork -->|"Windows"| spawn["spawn Julia child"] + spawn --> vdbw["run_versiondb_update"] + vdbw --> suw["run_selfupdate"] + suw --> waitw["wait for Julia child"] + + vdb -.->|"if interval elapsed"| spawnvdb["spawn: juliaup <versiondb uuid>"] + su -.->|"if interval elapsed"| spawnsu["spawn: juliaup self update"] +``` + +Key points: + +- The launcher itself only takes a **shared** lock, briefly, via + `load_config_db`. It is released before Julia is launched. +- `run_versiondb_update` and `run_selfupdate` do **not** run the work inline; + they merely *spawn* `juliaup` subprocesses (subject to their configured + intervals). Those subprocesses do the locking described below. +- Because the background self-update runs detached, a slow download there must + not hold the exclusive lock — otherwise a *later, unrelated* `julia` + invocation would stall on the shared lock at startup. This is exactly the + scenario reported in #1524. + +## `update_version_db` + +Run by `juliaup` directly and as the first step of `add`, `update`, and +`self update`. It reads a config snapshot, releases the lock, downloads the +version database, then re-acquires the exclusive lock to commit — with an +optimistic check that aborts if the config changed underneath it. + +```mermaid +flowchart TD + s(["update_version_db"]) --> rl["get_read_lock
shared lock"] + rl --> snap["load_config_db(snapshot)"] + snap --> ul["data_unlock
(release)"] + ul --> net["download db version + versiondb json
NO lock held"] + net --> wl["load_mut_config_db
exclusive lock"] + wl --> check{"config changed
since snapshot?"} + check -->|yes| abort["discard download, return"] + check -->|no| commit["rename versiondb + save_config_db"] + commit --> done(["release"]) + abort --> done +``` + +## `juliaup add` + +Splits installation into `download_version_to_temp` (lock-free) and +`commit_version_install` (exclusive lock). The shared-lock pre-check avoids a +redundant download when the channel is already installed. + +```mermaid +flowchart TD + s(["juliaup add <channel>"]) --> vdb["update_version_db
(see above)"] + vdb --> pre["load_config_db(None)
shared lock → released"] + pre --> inst{"already
installed?"} + inst -->|yes| ret["print + return"] + inst -->|no| dl["download_version_to_temp
NO lock held"] + dl --> wl["load_mut_config_db
exclusive lock"] + wl --> recheck{"installed by
another process?"} + recheck -->|yes| ret2["print + return"] + recheck -->|no| commit["commit_version_install
(rename temp → final)"] + commit --> cfg["insert channel + save_config_db"] + cfg --> done(["release"]) +``` + +For nightly/PR channels (`add_non_db`), `install_non_db_version` → +`install_from_url` downloads into a temp dir and atomically renames into place +with **no lock held**; the exclusive lock is taken afterwards only to write the +config entry. + +## `juliaup self update` + +Reads the configured juliaup channel under a shared lock, releases it, performs +all network work (version check + binary download/extract) lock-free, and +re-acquires the exclusive lock only to record the self-update timestamp. + +```mermaid +flowchart TD + s(["juliaup self update"]) --> vdb["update_version_db
(see above)"] + vdb --> rl["get_read_lock + load_config_db
shared lock"] + rl --> chan["read juliaup_channel"] + chan --> ul["data_unlock (release)"] + ul --> ver["download_juliaup_version
NO lock held"] + ver --> ts["load_mut_config_db
exclusive lock
write last_selfupdate → release"] + ts --> changed{"new version?"} + changed -->|no| done(["done"]) + changed -->|yes| dlx["download_extract_sans_parent
NO lock held"] + dlx --> hook["spawn juliaup _post-update
(child reads config)"] + hook --> done +``` + +The post-update hook runs in a child process that needs to read the config, so +the exclusive lock must not be held when it is spawned — which is naturally the +case here because the lock is released right after the timestamp write. + +## `juliaup update` + +Two-phase like the others, but fans out over multiple channels: phase 1 takes a +single config snapshot under a shared lock and downloads everything needed +lock-free; phase 2 takes the exclusive lock once and commits all prepared +updates together. + +```mermaid +flowchart TD + s(["juliaup update [channel]"]) --> vdb["update_version_db
(see above)"] + vdb --> snap["get_read_lock + load_config_db
shared lock → snapshot → release"] + snap --> list["select channels to update
(skip aliases)"] + list --> loop["for each channel:
prepare_channel_update"] + loop --> dl["download needed versions
(or reuse already-installed)
NO lock held"] + dl --> wl["load_mut_config_db
exclusive lock"] + wl --> commit["for each prepared update:
commit_channel_update
(rename + config + symlinks)"] + commit --> gc["garbage_collect_versions"] + gc --> save["save_config_db"] + save --> done(["release"]) +``` + +`commit_channel_update` re-checks that each channel still exists before applying +its update, so a channel removed concurrently is skipped rather than causing an +error. System-channel updates whose target version is already installed skip the +download entirely. + +## The #1524 scenario + +Sequence that produced the spurious stall, and why it no longer does: + +```mermaid +sequenceDiagram + participant J1 as julia (1st) + participant SU as juliaup self update (daemon) + participant J2 as julia (2nd) + + J1->>J1: shared lock (read) → release + J1-->>SU: spawn background self update + J1->>J1: exec into Julia (runs Pkg.instantiate) + Note over SU: before: held EXCLUSIVE lock
across slow download + J2->>J2: startup wants shared lock + Note over J2: before: BLOCKS on SU's exclusive lock
→ prints "locked by another process" + Note over SU: after: lock released before download
→ J2 acquires shared lock immediately +``` + +Before the fix, `self update` (and `add` / `update`) held the exclusive lock +across their downloads. A second `julia` started while a background self-update +was downloading would block at startup on its shared-lock acquisition. After the +fix, every command releases the lock before downloading, so the only contention +left is the millisecond-scale commit phase. diff --git a/src/command_add.rs b/src/command_add.rs index 8dfe616ca..e950084cb 100644 --- a/src/command_add.rs +++ b/src/command_add.rs @@ -1,9 +1,12 @@ -use crate::config_file::{load_mut_config_db, save_config_db, JuliaupConfigChannel}; +use crate::config_file::{ + load_config_db, load_mut_config_db, save_config_db, JuliaupConfigChannel, +}; use crate::global_paths::GlobalPaths; #[cfg(not(windows))] use crate::operations::create_symlink; use crate::operations::{ - channel_to_name, install_non_db_version, install_version, update_version_db, + channel_to_name, commit_version_install, download_version_to_temp, install_non_db_version, + update_version_db, }; use crate::utils::{print_juliaup_style, JuliaupMessageType}; use crate::versions_file::load_versions_db; @@ -35,6 +38,23 @@ pub fn run_command_add(channel: &str, paths: &GlobalPaths) -> Result<()> { })? .version; + // Check whether the channel is already installed before downloading. This + // read only briefly takes a shared lock, which is released immediately. + { + let config_file = load_config_db(paths, None) + .with_context(|| "`add` command failed to load configuration data.")?; + + if config_file.data.installed_channels.contains_key(channel) { + eprintln!("'{}' is already installed.", &channel); + return Ok(()); + } + } + + // Download and extract the version without holding the configuration lock, + // so concurrent juliaup processes (and the launcher) are not blocked. + let downloaded = download_version_to_temp(required_version, &version_db, paths)?; + + // Re-acquire the exclusive lock to commit the installation. let mut config_file = load_mut_config_db(paths) .with_context(|| "`add` command failed to load configuration data.")?; @@ -43,7 +63,7 @@ pub fn run_command_add(channel: &str, paths: &GlobalPaths) -> Result<()> { return Ok(()); } - install_version(required_version, &mut config_file.data, &version_db, paths)?; + commit_version_install(downloaded, required_version, &mut config_file.data, paths)?; config_file.data.installed_channels.insert( channel.to_string(), @@ -88,12 +108,16 @@ pub fn run_command_add(channel: &str, paths: &GlobalPaths) -> Result<()> { } fn add_non_db(channel: &str, paths: &GlobalPaths) -> Result<()> { - let mut config_file = load_mut_config_db(paths) - .with_context(|| "`add` command failed to load configuration data.")?; + // Check whether the channel is already installed before downloading. This + // read only briefly takes a shared lock, which is released immediately. + { + let config_file = load_config_db(paths, None) + .with_context(|| "`add` command failed to load configuration data.")?; - if config_file.data.installed_channels.contains_key(channel) { - eprintln!("'{}' is already installed.", &channel); - return Ok(()); + if config_file.data.installed_channels.contains_key(channel) { + eprintln!("'{}' is already installed.", &channel); + return Ok(()); + } } // Warn about security implications of PR builds @@ -108,9 +132,19 @@ fn add_non_db(channel: &str, paths: &GlobalPaths) -> Result<()> { ); } + // Download and extract the version without holding the configuration lock. let name = channel_to_name(channel)?; let (config_channel, _used_dmg) = install_non_db_version(channel, &name, paths)?; + // Re-acquire the exclusive lock to commit the installation. + let mut config_file = load_mut_config_db(paths) + .with_context(|| "`add` command failed to load configuration data.")?; + + if config_file.data.installed_channels.contains_key(channel) { + eprintln!("'{}' is already installed.", &channel); + return Ok(()); + } + config_file .data .installed_channels diff --git a/src/command_selfupdate.rs b/src/command_selfupdate.rs index 8f776e231..5c8ef9a14 100644 --- a/src/command_selfupdate.rs +++ b/src/command_selfupdate.rs @@ -4,7 +4,7 @@ use anyhow::{Context, Result}; #[cfg(feature = "selfupdate")] pub fn run_command_selfupdate(paths: &GlobalPaths) -> Result<()> { - use crate::config_file::{load_mut_config_db, save_config_db}; + use crate::config_file::{get_read_lock, load_config_db, load_mut_config_db, save_config_db}; use crate::operations::{download_extract_sans_parent, download_juliaup_version}; use crate::utils::get_juliaserver_base_url; use crate::{get_juliaup_target, get_own_version}; @@ -12,7 +12,14 @@ pub fn run_command_selfupdate(paths: &GlobalPaths) -> Result<()> { update_version_db(&None, paths).with_context(|| "Failed to update versions db.")?; - let mut config_file = load_mut_config_db(paths) + // Read the configured juliaup channel under a short-lived shared lock, then + // release it before any network operations. Holding the exclusive lock across + // the downloads below would block concurrent julia/juliaup invocations (which + // only need a shared read lock), surfacing as spurious "configuration is locked + // by another process" stalls. This mirrors how `juliaup add` downloads outside + // the lock and only re-acquires it to commit. + let file_lock = get_read_lock(paths)?; + let config_file = load_config_db(paths, Some(&file_lock)) .with_context(|| "`self update` command failed to load configuration db.")?; let juliaup_channel = match &config_file.self_data.juliaup_channel { @@ -20,6 +27,16 @@ pub fn run_command_selfupdate(paths: &GlobalPaths) -> Result<()> { None => "release".to_string(), }; + { + let (_, res) = file_lock.data_unlock(); + res.with_context(|| { + format!( + "Failed to unlock configuration lock file `{}`.", + paths.lockfile.display() + ) + })?; + } + let juliaupserver_base = get_juliaserver_base_url().with_context(|| "Failed to get Juliaup server base URL.")?; @@ -44,14 +61,21 @@ pub fn run_command_selfupdate(paths: &GlobalPaths) -> Result<()> { let version = download_juliaup_version(version_url.as_ref())?; - config_file.self_data.last_selfupdate = Some(chrono::Utc::now()); + // Re-acquire the exclusive lock only briefly to record the self-update + // timestamp, so the lock is never held across the network operations above. + { + let mut config_file = load_mut_config_db(paths) + .with_context(|| "`self update` command failed to load configuration db.")?; - save_config_db(&mut config_file, paths).with_context(|| { - format!( - "Failed to save configuration file at `{}`.", - paths.juliaupconfig.display() - ) - })?; + config_file.self_data.last_selfupdate = Some(chrono::Utc::now()); + + save_config_db(&mut config_file, paths).with_context(|| { + format!( + "Failed to save configuration file at `{}`.", + paths.juliaupconfig.display() + ) + })?; + } if version == get_own_version().unwrap() { eprintln!( @@ -90,12 +114,6 @@ pub fn run_command_selfupdate(paths: &GlobalPaths) -> Result<()> { download_extract_sans_parent(new_juliaup_url.as_ref(), my_own_folder, 0)?; - // Release the configuration lock before invoking the post-update hook: - // the hook runs in a child process that needs to read the config (e.g. - // to restore channel symlinks), and would otherwise deadlock waiting on - // the lock this process still holds. - drop(config_file); - let new_juliaup = my_own_folder.join(format!("juliaup{}", std::env::consts::EXE_SUFFIX)); if let Err(e) = std::process::Command::new(&new_juliaup) .arg("_post-update") diff --git a/src/command_update.rs b/src/command_update.rs index 241e00c57..5f21baad1 100644 --- a/src/command_update.rs +++ b/src/command_update.rs @@ -1,15 +1,20 @@ use crate::config_file::JuliaupConfig; -use crate::config_file::{load_mut_config_db, save_config_db, JuliaupConfigChannel}; +use crate::config_file::{ + get_read_lock, load_config_db, load_mut_config_db, save_config_db, JuliaupConfigChannel, +}; use crate::global_paths::GlobalPaths; use crate::jsonstructs_versionsdb::JuliaupVersionDB; #[cfg(not(windows))] use crate::operations::create_symlink; -use crate::operations::{garbage_collect_versions, install_from_url, is_pr_channel}; -use crate::operations::{install_version, update_version_db}; +use crate::operations::{ + commit_version_install, download_version_to_temp, garbage_collect_versions, install_from_url, + is_pr_channel, update_version_db, +}; use crate::utils::{print_juliaup_style, JuliaupMessageType}; use crate::versions_file::load_versions_db; use anyhow::{anyhow, bail, Context, Result}; use std::path::PathBuf; +use tempfile::TempDir; fn resolve_channel_alias(config_db: &JuliaupConfig, channel_name: &str) -> Result { match config_db.installed_channels.get(channel_name) { @@ -19,15 +24,45 @@ fn resolve_channel_alias(config_db: &JuliaupConfig, channel_name: &str) -> Resul } } -fn update_channel( - config_db: &mut JuliaupConfig, - channel: &String, +/// A channel update that has been prepared (downloaded) without holding the +/// configuration lock, ready to be committed under the exclusive lock. +enum PreparedUpdate { + /// A database (system) channel update. `downloaded` is `None` when the + /// target version was already installed and only the channel pointer needs + /// to move. + System { + channel: String, + new_version: String, + downloaded: Option, + }, + /// A direct-download (nightly/PR) channel update. `install_from_url` has + /// already placed the new install on disk; only the config entry remains. + DirectDownload { + channel: String, + channel_data: JuliaupConfigChannel, + }, +} + +impl PreparedUpdate { + fn channel(&self) -> &str { + match self { + PreparedUpdate::System { channel, .. } + | PreparedUpdate::DirectDownload { channel, .. } => channel, + } + } +} + +/// Phase 1 (no lock held): decide whether `channel` needs updating based on a +/// configuration snapshot and, if so, perform the network download. Returns +/// `None` when the channel is already up to date or is not updatable. +fn prepare_channel_update( + config_db: &JuliaupConfig, + channel: &str, version_db: &JuliaupVersionDB, ignore_non_updatable_channel: bool, paths: &GlobalPaths, -) -> Result<()> { - let current_version = - &config_db.installed_channels.get(channel).ok_or_else(|| anyhow!("Trying to get the installed version for a channel that does not exist in the config database."))?.clone(); +) -> Result> { + let current_version = config_db.installed_channels.get(channel).ok_or_else(|| anyhow!("Trying to get the installed version for a channel that does not exist in the config database."))?; match current_version { JuliaupConfigChannel::DirectDownloadChannel { @@ -58,14 +93,12 @@ fn update_channel( paths, )?; - #[cfg(not(windows))] - if config_db.settings.create_channel_symlinks { - create_symlink(&channel_data, channel, paths)?; - } - - config_db - .installed_channels - .insert(channel.clone(), channel_data); + Ok(Some(PreparedUpdate::DirectDownload { + channel: channel.to_string(), + channel_data, + })) + } else { + Ok(None) } } JuliaupConfigChannel::SystemChannel { version } => { @@ -79,34 +112,35 @@ fn update_channel( JuliaupMessageType::Progress, ); - install_version(&should_version.version, config_db, version_db, paths) - .with_context(|| { - format!( - "Failed to install '{}' while updating channel '{}'.", - should_version.version, channel - ) - })?; - - config_db.installed_channels.insert( - channel.clone(), - JuliaupConfigChannel::SystemChannel { - version: should_version.version.clone(), - }, - ); + // Only download if the target version is not already installed. + let downloaded = if config_db + .installed_versions + .contains_key(&should_version.version) + { + None + } else { + Some( + download_version_to_temp(&should_version.version, version_db, paths) + .with_context(|| { + format!( + "Failed to download '{}' while updating channel '{}'.", + should_version.version, channel + ) + })?, + ) + }; - #[cfg(not(windows))] - if config_db.settings.create_channel_symlinks { - create_symlink( - &JuliaupConfigChannel::SystemChannel { - version: should_version.version.clone(), - }, - &format!("julia-{}", channel), - paths, - )?; - } + Ok(Some(PreparedUpdate::System { + channel: channel.to_string(), + new_version: should_version.version.clone(), + downloaded, + })) + } else { + Ok(None) } } else if ignore_non_updatable_channel { eprintln!("Skipping update for '{}' channel, it no longer exists in the version database.", channel); + Ok(None) } else { bail!( "Failed to update '{}' because it no longer exists in the version database.", @@ -121,9 +155,75 @@ fn update_channel( channel ); } + Ok(None) } JuliaupConfigChannel::AliasChannel { .. } => { - unreachable!("Alias channels should be resolved before calling update_channel. Please submit a bug report."); + unreachable!("Alias channels should be resolved before calling prepare_channel_update. Please submit a bug report."); + } + } +} + +/// Phase 2 (exclusive lock held): commit a previously prepared update into the +/// configuration. If the channel was removed concurrently, the prepared update +/// is discarded. +fn commit_channel_update( + config_db: &mut JuliaupConfig, + prepared: PreparedUpdate, + paths: &GlobalPaths, +) -> Result<()> { + // If the channel was removed while we were downloading, discard the update. + if !config_db + .installed_channels + .contains_key(prepared.channel()) + { + return Ok(()); + } + + match prepared { + PreparedUpdate::DirectDownload { + channel, + channel_data, + } => { + #[cfg(not(windows))] + if config_db.settings.create_channel_symlinks { + create_symlink(&channel_data, &channel, paths)?; + } + + config_db.installed_channels.insert(channel, channel_data); + } + PreparedUpdate::System { + channel, + new_version, + downloaded, + } => { + if let Some(downloaded) = downloaded { + commit_version_install(downloaded, &new_version, config_db, paths).with_context( + || { + format!( + "Failed to install '{}' while updating channel '{}'.", + new_version, channel + ) + }, + )?; + } + + config_db.installed_channels.insert( + channel.clone(), + JuliaupConfigChannel::SystemChannel { + version: new_version.clone(), + }, + ); + + #[cfg(not(windows))] + if config_db.settings.create_channel_symlinks { + create_symlink( + &JuliaupConfigChannel::SystemChannel { + version: new_version, + }, + &format!("julia-{}", channel), + paths, + )?; + } } } @@ -136,47 +236,84 @@ pub fn run_command_update(channel: &Option, paths: &GlobalPaths) -> Resu let version_db = load_versions_db(paths).with_context(|| "`update` command failed to load versions db.")?; - let mut config_file = load_mut_config_db(paths) - .with_context(|| "`update` command failed to load configuration data.")?; + // Phase 1: take a snapshot of the configuration under a short-lived shared + // lock, release it, then perform all downloads with no lock held so that + // concurrent julia/juliaup invocations are not blocked. + let config_snapshot = { + let file_lock = get_read_lock(paths)?; + let config_file = load_config_db(paths, Some(&file_lock)) + .with_context(|| "`update` command failed to load configuration data.")?; + let snapshot = config_file.data.clone(); + let (_, res) = file_lock.data_unlock(); + res.with_context(|| { + format!( + "Failed to unlock configuration lock file `{}`.", + paths.lockfile.display() + ) + })?; + snapshot + }; - match channel { - None => { - for (k, v) in config_file.data.installed_channels.clone() { - // Skip alias channels - they don't need to be updated directly - // since they point to other channels that will be updated - if let JuliaupConfigChannel::AliasChannel { .. } = v { - continue; - } - if let Err(e) = update_channel(&mut config_file.data, &k, &version_db, true, paths) - { + let update_all = channel.is_none(); + + let channels_to_update: Vec = match channel { + None => config_snapshot + .installed_channels + .iter() + // Skip alias channels - they don't need to be updated directly + // since they point to other channels that will be updated. + .filter(|(_, v)| !matches!(v, JuliaupConfigChannel::AliasChannel { .. })) + .map(|(k, _)| k.clone()) + .collect(), + Some(channel) => { + if !config_snapshot.installed_channels.contains_key(channel) { + bail!( + "'{}' cannot be updated because it is currently not installed.", + channel + ); + } + // Resolve any aliases to get the actual target channel + vec![resolve_channel_alias(&config_snapshot, channel)?] + } + }; + + let mut prepared_updates = Vec::new(); + for name in channels_to_update { + match prepare_channel_update(&config_snapshot, &name, &version_db, update_all, paths) { + Ok(Some(prepared)) => prepared_updates.push(prepared), + Ok(None) => {} + Err(e) => { + if update_all { print_juliaup_style( "Failed", - &format!("to update {k}. {e}"), + &format!("to update {name}. {e}"), JuliaupMessageType::Error, ); + } else { + return Err(e); } } } - Some(channel) => { - if !config_file.data.installed_channels.contains_key(channel) { - bail!( - "'{}' cannot be updated because it is currently not installed.", - channel + } + + // Phase 2: re-acquire the exclusive lock only to commit the prepared updates. + let mut config_file = load_mut_config_db(paths) + .with_context(|| "`update` command failed to load configuration data.")?; + + for prepared in prepared_updates { + let name = prepared.channel().to_string(); + if let Err(e) = commit_channel_update(&mut config_file.data, prepared, paths) { + if update_all { + print_juliaup_style( + "Failed", + &format!("to update {name}. {e}"), + JuliaupMessageType::Error, ); + } else { + return Err(e); } - - // Resolve any aliases to get the actual target channel - let resolved_channel = resolve_channel_alias(&config_file.data, channel)?; - - update_channel( - &mut config_file.data, - &resolved_channel, - &version_db, - false, - paths, - )?; } - }; + } garbage_collect_versions(false, &mut config_file.data, paths)?; diff --git a/src/config_file.rs b/src/config_file.rs index 2d515b71a..b632ffd4c 100644 --- a/src/config_file.rs +++ b/src/config_file.rs @@ -173,6 +173,49 @@ pub struct JuliaupReadonlyConfigFile { pub self_data: JuliaupSelfConfig, } +/// Acquires a file lock, only printing the "locked by another process" message +/// if the lock cannot be obtained within a short grace period. This avoids +/// spurious messages for the common case where another process holds the lock +/// for just a few milliseconds (e.g. while committing a config change). +/// +/// `try_lock` must return the original file (via the lock error) when the lock +/// is currently held, so it can be retried; `wait_lock` performs a blocking +/// acquisition. +fn lock_with_delayed_message( + file: File, + try_lock: TryFn, + wait_lock: WaitFn, +) -> Result> +where + TryFn: Fn(File) -> std::result::Result, File>, + WaitFn: FnOnce(File) -> Result>, +{ + use std::time::{Duration, Instant}; + + const GRACE_PERIOD: Duration = Duration::from_secs(1); + const POLL_INTERVAL: Duration = Duration::from_millis(50); + + let mut file = file; + let start = Instant::now(); + + loop { + match try_lock(file) { + Ok(lock) => return Ok(lock), + Err(unlocked_file) => { + file = unlocked_file; + if start.elapsed() >= GRACE_PERIOD { + break; + } + std::thread::sleep(POLL_INTERVAL); + } + } + } + + eprintln!("Juliaup configuration is locked by another process, waiting for it to unlock."); + + wait_lock(file) +} + pub fn get_read_lock(paths: &GlobalPaths) -> Result> { std::fs::create_dir_all(&paths.juliauphome).with_context(|| { format!( @@ -198,16 +241,14 @@ pub fn get_read_lock(paths: &GlobalPaths) -> Result> { } }; - let file_lock = match SharedFlock::try_lock(lock_file) { - Ok(lock) => lock, - Err(e) => { - eprintln!( - "Juliaup configuration is locked by another process, waiting for it to unlock." - ); - - SharedFlock::wait_lock(e.into()).unwrap() - } - }; + let file_lock = lock_with_delayed_message( + lock_file, + |f| SharedFlock::try_lock(f).map_err(|e| e.into()), + |f| { + SharedFlock::wait_lock(f) + .map_err(|e| anyhow!("Failed to acquire shared configuration lock: {}.", e)) + }, + )?; Ok(file_lock) } @@ -322,16 +363,14 @@ pub fn load_mut_config_db(paths: &GlobalPaths) -> Result { } }; - let file_lock = match ExclusiveFlock::try_lock(lock_file) { - Ok(lock) => lock, - Err(e) => { - eprintln!( - "Juliaup configuration is locked by another process, waiting for it to unlock." - ); - - ExclusiveFlock::wait_lock(e.into()).unwrap() - } - }; + let file_lock = lock_with_delayed_message( + lock_file, + |f| ExclusiveFlock::try_lock(f).map_err(|e| e.into()), + |f| { + ExclusiveFlock::wait_lock(f) + .map_err(|e| anyhow!("Failed to acquire exclusive configuration lock: {}.", e)) + }, + )?; let mut file = std::fs::OpenOptions::new() .read(true) diff --git a/src/operations.rs b/src/operations.rs index 9423a7162..daa391fb8 100644 --- a/src/operations.rs +++ b/src/operations.rs @@ -39,6 +39,7 @@ use std::{ #[cfg(not(target_os = "freebsd"))] use tar::Archive; use tempfile::Builder; +use tempfile::TempDir; use tempfile::TempPath; use url::Url; @@ -729,16 +730,30 @@ fn compute_relative_binary_path( }) } -pub fn install_version( - fullversion: &String, - config_data: &mut JuliaupConfig, +/// Downloads and extracts a database version of Julia into a temporary +/// directory inside `juliauphome`, without acquiring the configuration lock. +/// +/// This is the slow, network-bound phase of installing a version. Keeping it +/// outside of any lock allows other juliaup processes (and the launcher) to keep +/// reading and modifying the configuration while a download is in progress. +/// The returned [`TempDir`] is committed into its final location by +/// [`commit_version_install`] while holding the exclusive lock. +pub fn download_version_to_temp( + fullversion: &str, version_db: &JuliaupVersionDB, paths: &GlobalPaths, -) -> Result<()> { - // Return immediately if the version is already installed. - if config_data.installed_versions.contains_key(fullversion) { - return Ok(()); - } +) -> Result { + std::fs::create_dir_all(&paths.juliauphome).with_context(|| { + format!( + "Failed to create juliaup home folder `{}`.", + paths.juliauphome.display() + ) + })?; + + let temp_dir = Builder::new() + .prefix("julia-temp-") + .tempdir_in(&paths.juliauphome) + .with_context(|| "Failed to create temporary directory for download.")?; // TODO At some point we could put this behind a conditional compile, we know // that we don't ship a bundled version for some platforms. @@ -749,27 +764,11 @@ pub fn install_version( .unwrap() // unwrap OK because we can't get a path that does not have a parent .join("BundledJulia"); - let child_target_foldername = format!("julia-{}", fullversion); - let target_path = paths.juliauphome.join(&child_target_foldername); - let target_parent = target_path.parent().ok_or_else(|| { - anyhow!( - "Target installation path `{}` has no parent directory.", - target_path.display() - ) - })?; - std::fs::create_dir_all(target_parent).with_context(|| { - format!( - "Failed to create parent directory `{}` for installation path `{}`.", - target_parent.display(), - target_path.display() - ) - })?; - if fullversion == full_version_string_of_bundled_version && path_of_bundled_version.exists() { let mut options = fs_extra::dir::CopyOptions::new(); options.overwrite = true; options.content_only = true; - fs_extra::dir::copy(path_of_bundled_version, &target_path, &options)?; + fs_extra::dir::copy(path_of_bundled_version, temp_dir.path(), &options)?; } else { let juliaupserver_base = get_juliaserver_base_url().with_context(|| "Failed to get Juliaup server base URL.")?; @@ -805,7 +804,7 @@ pub fn install_version( #[cfg(target_os = "macos")] let used_dmg = { - let (_, used_dmg) = try_download_dmg_with_fallback(&download_url, &target_path)?; + let (_, used_dmg) = try_download_dmg_with_fallback(&download_url, temp_dir.path())?; used_dmg }; @@ -817,17 +816,68 @@ pub fn install_version( .is_some_and(|(v, threshold)| v > threshold); if needs_notarization_check { - let julia_path = crate::utils::resolve_julia_binary_path(&target_path)?; + let julia_path = crate::utils::resolve_julia_binary_path(temp_dir.path())?; check_stdlib_notarization(&julia_path); } } #[cfg(not(target_os = "macos"))] { - download_extract_sans_parent(download_url.as_ref(), &target_path, 1)?; + download_extract_sans_parent(download_url.as_ref(), temp_dir.path(), 1)?; } } + Ok(temp_dir) +} + +/// Commits a version previously downloaded by [`download_version_to_temp`] into +/// its final location and registers it in the configuration. +/// +/// This must be called while holding the exclusive configuration lock (i.e. with +/// a mutable config db). If another process installed the same version while the +/// download was in progress, the temporary directory is discarded and the +/// existing installation is reused. +pub fn commit_version_install( + downloaded: TempDir, + fullversion: &str, + config_data: &mut JuliaupConfig, + paths: &GlobalPaths, +) -> Result<()> { + let child_target_foldername = format!("julia-{}", fullversion); + let target_path = paths.juliauphome.join(&child_target_foldername); + + // Another process may have installed this exact version while we were + // downloading. In that case discard our download and reuse the existing one. + if config_data.installed_versions.contains_key(fullversion) { + return Ok(()); + } + + let target_parent = target_path.parent().ok_or_else(|| { + anyhow!( + "Target installation path `{}` has no parent directory.", + target_path.display() + ) + })?; + std::fs::create_dir_all(target_parent).with_context(|| { + format!( + "Failed to create parent directory `{}` for installation path `{}`.", + target_parent.display(), + target_path.display() + ) + })?; + + if target_path.exists() { + std::fs::remove_dir_all(&target_path).with_context(|| { + format!( + "Failed to remove stale installation directory `{}`.", + target_path.display() + ) + })?; + } + + // keep() consumes the TempDir and returns the path without cleanup + retry_rename(&downloaded.keep(), &target_path)?; + let mut rel_path = PathBuf::new(); rel_path.push("."); rel_path.push(&child_target_foldername); @@ -835,7 +885,7 @@ pub fn install_version( let binary_path = compute_relative_binary_path(&target_path, &rel_path, &paths.juliauphome); config_data.installed_versions.insert( - fullversion.clone(), + fullversion.to_string(), JuliaupConfigVersion { path: rel_path.to_string_lossy().into_owned(), binary_path,