From 49f60d37fe6ab76eb4d1e7de63302aebe6f103db Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Wed, 4 Jan 2023 12:01:02 +1100 Subject: [PATCH] Fix #588 race cond updating `.crates.toml` (#645) Fixed #588 * Add new dep fs-lock v0.1.0 to crates/bin * Refactor: Impl new type `Manifests` in crates/bin for managing manifests. * Fix #588 race cond updating `.crates.toml` Signed-off-by: Jiahao XU --- Cargo.lock | 1 + crates/bin/Cargo.toml | 1 + crates/bin/src/entry.rs | 105 ++++++++++-------------------------- crates/bin/src/lib.rs | 1 + crates/bin/src/manifests.rs | 85 +++++++++++++++++++++++++++++ 5 files changed, 117 insertions(+), 76 deletions(-) create mode 100644 crates/bin/src/manifests.rs diff --git a/Cargo.lock b/Cargo.lock index 47e36edb..33d0a1a0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -353,6 +353,7 @@ dependencies = [ "crates_io_api", "dirs", "embed-resource", + "fs-lock", "log", "miette", "mimalloc", diff --git a/crates/bin/Cargo.toml b/crates/bin/Cargo.toml index 00ab33ef..5861c712 100644 --- a/crates/bin/Cargo.toml +++ b/crates/bin/Cargo.toml @@ -27,6 +27,7 @@ binstalk-manifests = { path = "../binstalk-manifests", version = "0.1.1" } clap = { version = "4.0.32", features = ["derive"] } crates_io_api = { version = "0.8.1", default-features = false } dirs = "4.0.0" +fs-lock = { version = "0.1.0", path = "../fs-lock" } log = { version = "0.4.17", features = ["std"] } miette = "5.5.0" mimalloc = { version = "0.1.32", default-features = false, optional = true } diff --git a/crates/bin/src/entry.rs b/crates/bin/src/entry.rs index 5431e4c9..f1490f6c 100644 --- a/crates/bin/src/entry.rs +++ b/crates/bin/src/entry.rs @@ -1,4 +1,4 @@ -use std::{collections::BTreeMap, fs, io, path::PathBuf, sync::Arc, time::Duration}; +use std::{fs, path::PathBuf, sync::Arc, time::Duration}; use binstalk::{ errors::BinstallError, @@ -11,12 +11,7 @@ use binstalk::{ Resolver, }, }; -use binstalk_manifests::{ - binstall_crates_v1::Records as BinstallCratesV1Records, - cargo_crates_v1::{CratesToml, CratesTomlParseError}, - cargo_toml_binstall::PkgOverride, - CompactString, Version, -}; +use binstalk_manifests::cargo_toml_binstall::PkgOverride; use crates_io_api::AsyncClient as CratesIoApiClient; use log::LevelFilter; use miette::{miette, Result, WrapErr}; @@ -26,6 +21,7 @@ use tracing::{debug, error, info, warn}; use crate::{ args::{Args, Strategy}, install_path, + manifests::Manifests, ui::confirm, }; @@ -47,12 +43,12 @@ pub async fn install_crates(args: Args, jobserver_client: LazyJobserverClient) - .collect(); // Compute paths - let (install_path, cargo_roots, metadata, temp_dir) = + let (install_path, mut manifests, temp_dir) = compute_paths_and_load_manifests(args.roots, args.install_path)?; // Remove installed crates let mut crate_names = - filter_out_installed_crates(args.crate_names, args.force, metadata.as_ref()).peekable(); + filter_out_installed_crates(args.crate_names, args.force, manifests.as_mut())?.peekable(); if crate_names.peek().is_none() { debug!("Nothing to do"); @@ -160,21 +156,8 @@ pub async fn install_crates(args: Args, jobserver_client: LazyJobserverClient) - .map(|fetch| fetch.install(&binstall_opts)) .collect::, BinstallError>>()?; - if let Some((mut cargo_binstall_metadata, _)) = metadata { - // The cargo manifest path is already created when loading - // metadata. - - debug!("Writing .crates.toml"); - CratesToml::append_to_path( - cargo_roots.join(".crates.toml"), - metadata_vec.iter(), - )?; - - debug!("Writing binstall/crates-v1.json"); - for metadata in metadata_vec { - cargo_binstall_metadata.replace(metadata); - } - cargo_binstall_metadata.overwrite()?; + if let Some(manifests) = manifests { + manifests.update(metadata_vec)?; } if no_cleanup { @@ -205,13 +188,11 @@ pub async fn install_crates(args: Args, jobserver_client: LazyJobserverClient) - Ok(()) } -type Metadata = (BinstallCratesV1Records, BTreeMap); - -/// Return (install_path, cargo_roots, metadata, temp_dir) +/// Return (install_path, manifests, temp_dir) fn compute_paths_and_load_manifests( roots: Option, install_path: Option, -) -> Result<(PathBuf, PathBuf, Option, tempfile::TempDir)> { +) -> Result<(PathBuf, Option, tempfile::TempDir)> { block_in_place(|| { // Compute cargo_roots let cargo_roots = install_path::get_cargo_roots_path(roots).ok_or_else(|| { @@ -229,42 +210,9 @@ fn compute_paths_and_load_manifests( fs::create_dir_all(&install_path).map_err(BinstallError::Io)?; debug!("Using install path: {}", install_path.display()); - // Load metadata - let metadata = if !custom_install_path { - // Read cargo_binstall_metadata - let metadata_dir = cargo_roots.join("binstall"); - fs::create_dir_all(&metadata_dir).map_err(BinstallError::Io)?; - let manifest_path = metadata_dir.join("crates-v1.json"); - - debug!( - "Reading {} from {}", - "cargo_binstall_metadata", - manifest_path.display() - ); - - let cargo_binstall_metadata = BinstallCratesV1Records::load_from_path(&manifest_path)?; - - // Read cargo_install_v1_metadata - let manifest_path = cargo_roots.join(".crates.toml"); - - debug!( - "Reading {} from {}", - "cargo_install_v1_metadata", - manifest_path.display() - ); - - let cargo_install_v1_metadata = match CratesToml::load_from_path(&manifest_path) { - Ok(metadata) => metadata.collect_into_crates_versions()?, - Err(CratesTomlParseError::Io(io_err)) - if io_err.kind() == io::ErrorKind::NotFound => - { - // .crates.toml does not exist, create an empty BTreeMap - Default::default() - } - Err(err) => Err(err)?, - }; - - Some((cargo_binstall_metadata, cargo_install_v1_metadata)) + // Load manifests + let manifests = if !custom_install_path { + Some(Manifests::open_exclusive(&cargo_roots)?) } else { None }; @@ -280,7 +228,7 @@ fn compute_paths_and_load_manifests( .map_err(BinstallError::from) .wrap_err("Creating a temporary directory failed.")?; - Ok((install_path, cargo_roots, metadata, temp_dir)) + Ok((install_path, manifests, temp_dir)) }) } @@ -288,18 +236,23 @@ fn compute_paths_and_load_manifests( fn filter_out_installed_crates( crate_names: Vec, force: bool, - metadata: Option<&Metadata>, -) -> impl Iterator)> + '_ { - CrateName::dedup(crate_names) + manifests: Option<&mut Manifests>, +) -> Result)> + '_> { + let mut installed_crates = manifests + .map(Manifests::load_installed_crates) + .transpose()?; + + Ok(CrateName::dedup(crate_names) .filter_map(move |crate_name| { let name = &crate_name.name; - let curr_version = metadata - // `cargo-uninstall` can be called to uninstall crates, - // but it only updates .crates.toml. + let curr_version = installed_crates + .as_mut() + // Since crate_name is deduped, every entry of installed_crates + // can be visited at most once. // - // So here we will honour .crates.toml only. - .and_then(|metadata| metadata.1.get(name)); + // So here we take ownership of the version stored to avoid cloning. + .and_then(|crates| crates.remove(name)); match ( force, @@ -307,7 +260,7 @@ fn filter_out_installed_crates( &crate_name.version_req, ) { (false, Some(curr_version), Some(version_req)) - if version_req.is_latest_compatible(curr_version) => + if version_req.is_latest_compatible(&curr_version) => { debug!("Bailing out early because we can assume wanted is already installed from metafile"); info!("{name} v{curr_version} is already installed, use --force to override"); @@ -316,10 +269,10 @@ fn filter_out_installed_crates( // The version req is "*" thus a remote upgraded version could exist (false, Some(curr_version), None) => { - Some((crate_name, Some(curr_version.clone()))) + Some((crate_name, Some(curr_version))) } _ => Some((crate_name, None)), } - }) + })) } diff --git a/crates/bin/src/lib.rs b/crates/bin/src/lib.rs index d3221595..c1c70c59 100644 --- a/crates/bin/src/lib.rs +++ b/crates/bin/src/lib.rs @@ -3,4 +3,5 @@ pub mod bin_util; pub mod entry; pub mod install_path; pub mod logging; +pub mod manifests; pub mod ui; diff --git a/crates/bin/src/manifests.rs b/crates/bin/src/manifests.rs new file mode 100644 index 00000000..609fa250 --- /dev/null +++ b/crates/bin/src/manifests.rs @@ -0,0 +1,85 @@ +use std::{collections::BTreeMap, fs, io::Seek, path::Path}; + +use binstalk::errors::BinstallError; +use binstalk_manifests::{ + binstall_crates_v1::Records as BinstallCratesV1Records, cargo_crates_v1::CratesToml, + crate_info::CrateInfo, CompactString, Version, +}; +use fs_lock::FileLock; +use miette::{Error, Result}; +use tracing::debug; + +pub struct Manifests { + binstall: BinstallCratesV1Records, + cargo_crates_v1: FileLock, +} + +impl Manifests { + pub fn open_exclusive(cargo_roots: &Path) -> Result { + // Read cargo_binstall_metadata + let metadata_path = cargo_roots.join("binstall/crates-v1.json"); + fs::create_dir_all(metadata_path.parent().unwrap()).map_err(BinstallError::Io)?; + + debug!( + "Reading binstall metadata from {} and obtaining exclusive lock", + metadata_path.display() + ); + + let binstall = BinstallCratesV1Records::load_from_path(&metadata_path)?; + + // Read cargo_install_v1_metadata + let manifest_path = cargo_roots.join(".crates.toml"); + + debug!( + "Obtaining exclusive lock of cargo install v1 metadata in path {}", + manifest_path.display() + ); + + let cargo_crates_v1 = fs::File::options() + .read(true) + .write(true) + .create(true) + .open(manifest_path) + .and_then(FileLock::new_exclusive) + .map_err(BinstallError::Io)?; + + Ok(Self { + binstall, + cargo_crates_v1, + }) + } + + fn rewind_cargo_crates_v1(&mut self) -> Result<()> { + self.cargo_crates_v1 + .rewind() + .map_err(BinstallError::Io) + .map_err(Error::from) + } + + /// `cargo-uninstall` can be called to uninstall crates, + /// but it only updates .crates.toml. + /// + /// So here we will honour .crates.toml only. + pub fn load_installed_crates(&mut self) -> Result> { + self.rewind_cargo_crates_v1()?; + + CratesToml::load_from_reader(&mut self.cargo_crates_v1) + .and_then(CratesToml::collect_into_crates_versions) + .map_err(Error::from) + } + + pub fn update(mut self, metadata_vec: Vec) -> Result<()> { + self.rewind_cargo_crates_v1()?; + + debug!("Writing .crates.toml"); + CratesToml::append_to_file(&mut self.cargo_crates_v1, &metadata_vec)?; + + debug!("Writing binstall/crates-v1.json"); + for metadata in metadata_vec { + self.binstall.replace(metadata); + } + self.binstall.overwrite()?; + + Ok(()) + } +}