perf: use mutex to lock package tarball extraction

Author: anonrig

crates/registry/src/lib.rs

@@ -98,8 +98,10 @@ impl RegistryManager {
         let package_node_modules_path =
             self.store_path.join(dependency_store_folder_name).join("node_modules");
 
-        // TODO: We shouldn't call this function for multiple same packages.
-        // There needs to be some sort of thread safety.
+        // Make sure to lock the package's mutex so we don't install the same package's tarball
+        // in different threads.
+        let mutex_guard = package.mutex.lock().await;
+
         self.tarball_manager
             .download_dependency(
                 name,
@@ -109,6 +111,8 @@ impl RegistryManager {
             )
             .await?;
 
+        drop(mutex_guard);
+
         if let Some(dependencies) = package_version.dependencies.as_ref() {
             join_all(
                 dependencies

crates/registry/src/package.rs

@@ -1,6 +1,7 @@
-use std::collections::HashMap;
+use std::{collections::HashMap, sync::Arc};
 
 use serde::{Deserialize, Serialize};
+use tokio::sync::Mutex;
 
 use crate::error::RegistryError;
 
@@ -35,6 +36,9 @@ pub struct Package {
     #[serde(alias = "dist-tags")]
     dist_tags: HashMap<String, String>,
     pub versions: HashMap<String, PackageVersion>,
+
+    #[serde(skip_serializing, skip_deserializing)]
+    pub mutex: Arc<Mutex<u8>>,
 }
 
 impl Package {

crates/tarball/src/lib.rs

@@ -62,13 +62,7 @@ impl TarballManager {
         archive.unpack(&unpack_path)?;
         fs::remove_file(tarball_path)?;
         fs::create_dir_all(extract_path)?;
-        // Check if extract_path is empty or not.
-        // This is due by a race condition causing 2 different threads to download and
-        // extract to the same folder. Likely be fixed with mutex locks.
-        // TODO: Remove this when we have some sort of thread safety
-        if extract_path.read_dir()?.next().is_none() {
-            fs::rename(unpack_path.join("package"), extract_path)?;
-        }
+        fs::rename(unpack_path.join("package"), extract_path)?;
         fs::remove_dir_all(&unpack_path)?;
         Ok(())
     }
@@ -81,15 +75,17 @@ impl TarballManager {
         save_path: &Path,
         symlink_to: &Path,
     ) -> Result<(), TarballError> {
+        let symlink_exists = symlink_to.is_symlink();
+
         // If name contains `/` such as @fastify/error, we need to make sure that @fastify folder
         // exists before we symlink to that directory.
-        if name.contains('/') {
+        if name.contains('/') && !symlink_exists {
             fs::create_dir_all(symlink_to.parent().unwrap())?;
         }
 
         // Do not try to install dependency if this version already exists in package.json
-        if save_path.exists() {
-            if !symlink_to.is_symlink() {
+        if save_path.exists() || symlink_exists {
+            if !symlink_exists {
                 symlink_dir(&save_path.to_path_buf(), &symlink_to.to_path_buf())?;
             }
             return Ok(());
@@ -102,9 +98,7 @@ impl TarballManager {
 
         // TODO: Currently symlink paths are absolute paths.
         // If you move the root folder to a different path, all symlinks will be broken.
-        if !symlink_to.is_symlink() {
-            symlink_dir(&save_path.to_path_buf(), &symlink_to.to_path_buf())?;
-        }
+        symlink_dir(&save_path.to_path_buf(), &symlink_to.to_path_buf())?;
 
         Ok(())
     }