From 0214320087d5b37bf9d08d2b3b0b44eae512bf93 Mon Sep 17 00:00:00 2001 From: Andrew Voynov <37143421+Andrew15-5@users.noreply.github.com> Date: Tue, 11 Mar 2025 23:20:41 +0300 Subject: [PATCH] Fix parallel package installation (#5979) Co-authored-by: Laurenz --- Cargo.lock | 1 + Cargo.toml | 1 + crates/typst-kit/Cargo.toml | 3 +- crates/typst-kit/src/package.rs | 84 ++++++++++++++++++++++++++++++--- 4 files changed, 82 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ac08b57ee..06dd4ab80 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2896,6 +2896,7 @@ dependencies = [ "dirs", "ecow", "env_proxy", + "fastrand", "flate2", "fontdb", "native-tls", diff --git a/Cargo.toml b/Cargo.toml index 40abaaca7..4e0d3a26c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -55,6 +55,7 @@ ctrlc = "3.4.1" dirs = "6" ecow = { version = "0.2", features = ["serde"] } env_proxy = "0.4" +fastrand = "2.3" flate2 = "1" fontdb = { version = "0.23", default-features = false } fs_extra = "1.3" diff --git a/crates/typst-kit/Cargo.toml b/crates/typst-kit/Cargo.toml index 52aa407c3..e59127d71 100644 --- a/crates/typst-kit/Cargo.toml +++ b/crates/typst-kit/Cargo.toml @@ -19,6 +19,7 @@ typst-utils = { workspace = true } dirs = { workspace = true, optional = true } ecow = { workspace = true } env_proxy = { workspace = true, optional = true } +fastrand = { workspace = true, optional = true } flate2 = { workspace = true, optional = true } fontdb = { workspace = true, optional = true } native-tls = { workspace = true, optional = true } @@ -43,7 +44,7 @@ fonts = ["dep:fontdb", "fontdb/memmap", "fontdb/fontconfig"] downloads = ["dep:env_proxy", "dep:native-tls", "dep:ureq", "dep:openssl"] # Add package downloading utilities, implies `downloads` -packages = ["downloads", "dep:dirs", "dep:flate2", "dep:tar"] +packages = ["downloads", "dep:dirs", "dep:flate2", "dep:tar", "dep:fastrand"] # Embeds some fonts into the binary: # - For text: Libertinus Serif, New Computer Modern diff --git a/crates/typst-kit/src/package.rs b/crates/typst-kit/src/package.rs index 172d8740a..1a1abd607 100644 --- a/crates/typst-kit/src/package.rs +++ b/crates/typst-kit/src/package.rs @@ -1,6 +1,7 @@ //! Download and unpack packages and package indices. use std::fs; +use std::io; use std::path::{Path, PathBuf}; use ecow::eco_format; @@ -100,7 +101,7 @@ impl PackageStorage { // Download from network if it doesn't exist yet. if spec.namespace == DEFAULT_NAMESPACE { - self.download_package(spec, &dir, progress)?; + self.download_package(spec, cache_dir, progress)?; if dir.exists() { return Ok(dir); } @@ -167,7 +168,7 @@ impl PackageStorage { pub fn download_package( &self, spec: &PackageSpec, - package_dir: &Path, + cache_dir: &Path, progress: &mut dyn Progress, ) -> PackageResult<()> { assert_eq!(spec.namespace, DEFAULT_NAMESPACE); @@ -191,11 +192,52 @@ impl PackageStorage { } }; + // The directory in which the package's version lives. + let base_dir = cache_dir.join(format!("{}/{}", spec.namespace, spec.name)); + + // The place at which the specific package version will live in the end. + let package_dir = base_dir.join(format!("{}", spec.version)); + + // To prevent multiple Typst instances from interferring, we download + // into a temporary directory first and then move this directory to + // its final destination. + // + // In the `rename` function's documentation it is stated: + // > This will not work if the new name is on a different mount point. + // + // By locating the temporary directory directly next to where the + // package directory will live, we are (trying our best) making sure + // that `tempdir` and `package_dir` are on the same mount point. + let tempdir = Tempdir::create(base_dir.join(format!( + ".tmp-{}-{}", + spec.version, + fastrand::u32(..), + ))) + .map_err(|err| error("failed to create temporary package directory", err))?; + + // Decompress the archive into the temporary directory. let decompressed = flate2::read::GzDecoder::new(data.as_slice()); - tar::Archive::new(decompressed).unpack(package_dir).map_err(|err| { - fs::remove_dir_all(package_dir).ok(); - PackageError::MalformedArchive(Some(eco_format!("{err}"))) - }) + tar::Archive::new(decompressed) + .unpack(&tempdir) + .map_err(|err| PackageError::MalformedArchive(Some(eco_format!("{err}"))))?; + + // When trying to move (i.e., `rename`) the directory from one place to + // another and the target/destination directory is empty, then the + // operation will succeed (if it's atomic, or hardware doesn't fail, or + // power doesn't go off, etc.). If however the target directory is not + // empty, i.e., another instance already successfully moved the package, + // then we can safely ignore the `DirectoryNotEmpty` error. + // + // This means that we do not check the integrity of an existing moved + // package, just like we don't check the integrity if the package + // directory already existed in the first place. If situations with + // broken packages still occur even with the rename safeguard, we might + // consider more complex solutions like file locking or checksums. + match fs::rename(&tempdir, &package_dir) { + Ok(()) => Ok(()), + Err(err) if err.kind() == io::ErrorKind::DirectoryNotEmpty => Ok(()), + Err(err) => Err(error("failed to move downloaded package directory", err)), + } } } @@ -207,6 +249,36 @@ struct MinimalPackageInfo { version: PackageVersion, } +/// A temporary directory that is a automatically cleaned up. +struct Tempdir(PathBuf); + +impl Tempdir { + /// Creates a directory at the path and auto-cleans it. + fn create(path: PathBuf) -> io::Result { + std::fs::create_dir_all(&path)?; + Ok(Self(path)) + } +} + +impl Drop for Tempdir { + fn drop(&mut self) { + _ = fs::remove_dir_all(&self.0); + } +} + +impl AsRef for Tempdir { + fn as_ref(&self) -> &Path { + &self.0 + } +} + +/// Enriches an I/O error with a message and turns it into a +/// `PackageError::Other`. +#[cold] +fn error(message: &str, err: io::Error) -> PackageError { + PackageError::Other(Some(eco_format!("{message}: {err}"))) +} + #[cfg(test)] mod tests { use super::*;