fix: prevent multiple downloads of modules (#2477)

1 files changed, 166 insertions, 68 deletions

diff --git a/cli/deno_dir.rs b/cli/deno_dir.rs
index ba0217bbe..990d3c661 100644
--- a/cli/deno_dir.rs
+++ b/cli/deno_dir.rs
@@ -18,15 +18,33 @@ use http;
 use ring;
 use serde_json;
 use std;
+use std::collections::HashSet;
 use std::fmt::Write;
 use std::fs;
 use std::path::Path;
 use std::path::PathBuf;
 use std::result::Result;
 use std::str;
+use std::sync::Arc;
+use std::sync::Mutex;
 use url;
 use url::Url;
 
+#[derive(Clone, Default)]
+pub struct DownloadCache(Arc<Mutex<HashSet<String>>>);
+
+impl DownloadCache {
+  pub fn mark(&self, module_id: &str) {
+    let mut c = self.0.lock().unwrap();
+    c.insert(module_id.to_string());
+  }
+
+  pub fn has(&self, module_id: &str) -> bool {
+    let c = self.0.lock().unwrap();
+    c.contains(module_id)
+  }
+}
+
 #[derive(Clone)]
 pub struct DenoDir {
   // Example: /Users/rld/.deno/
@@ -47,6 +65,11 @@ pub struct DenoDir {
   pub config: Vec<u8>,
 
   pub progress: Progress,
+
+  /// Set of all URLs that have been fetched in this run. This is a hacky way to work
+  /// around the fact that --reload will force multiple downloads of the same
+  /// module.
+  download_cache: DownloadCache,
 }
 
 impl DenoDir {
@@ -90,6 +113,7 @@ impl DenoDir {
       deps_https,
       config,
       progress,
+      download_cache: DownloadCache::default(),
     };
 
     // TODO Lazily create these directories.
@@ -392,7 +416,11 @@ fn get_source_code_async(
   // 1. Remote downloads are not allowed, we're only allowed to use cache.
   // 2. This is a remote module and we're allowed to use cached downloads.
   // 3. This is a local module.
-  if !is_module_remote || use_cache || no_fetch {
+  if !is_module_remote
+    || use_cache
+    || no_fetch
+    || deno_dir.download_cache.has(&module_name)
+  {
     debug!(
       "fetch local or reload {} is_module_remote {}",
       module_name, is_module_remote
@@ -436,11 +464,16 @@ fn get_source_code_async(
 
   debug!("is remote but didn't find module");
 
+  let download_cache = deno_dir.download_cache.clone();
+
   // not cached/local, try remote.
   Either::B(
     fetch_remote_source_async(deno_dir, &module_name, &filename).and_then(
       move |maybe_remote_source| match maybe_remote_source {
-        Some(output) => Ok(output),
+        Some(output) => {
+          download_cache.mark(&module_name);
+          Ok(output)
+        }
         None => Err(DenoError::from(std::io::Error::new(
           std::io::ErrorKind::NotFound,
           format!("cannot find remote file '{}'", &filename),
@@ -589,6 +622,45 @@ fn filter_shebang(bytes: Vec<u8>) -> Vec<u8> {
   }
 }
 
+/// Save source code and related headers for given module
+fn save_module_code_and_headers(
+  filename: &str,
+  module_name: &str,
+  source: &str,
+  maybe_content_type: Option<String>,
+  maybe_initial_filename: Option<String>,
+) -> DenoResult<()> {
+  let p = PathBuf::from(filename);
+  match p.parent() {
+    Some(ref parent) => fs::create_dir_all(parent),
+    None => Ok(()),
+  }?;
+  // Write file and create .headers.json for the file.
+  deno_fs::write_file(&p, &source, 0o666)?;
+  {
+    save_source_code_headers(filename, maybe_content_type.clone(), None);
+  }
+  // Check if this file is downloaded due to some old redirect request.
+  if maybe_initial_filename.is_some() {
+    // If yes, record down the headers for redirect.
+    // Also create its containing folder.
+    let pp = PathBuf::from(filename);
+    match pp.parent() {
+      Some(ref parent) => fs::create_dir_all(parent),
+      None => Ok(()),
+    }?;
+    {
+      save_source_code_headers(
+        &maybe_initial_filename.clone().unwrap(),
+        maybe_content_type.clone(),
+        Some(module_name.to_string()),
+      );
+    }
+  }
+
+  Ok(())
+}
+
 /// Asynchronously fetch remote source file specified by the URL `module_name`
 /// and write it to disk at `filename`.
 fn fetch_remote_source_async(
@@ -630,74 +702,52 @@ fn fetch_remote_source_async(
         match fetch_once_result {
           FetchOnceResult::Redirect(url) => {
             // If redirects, update module_name and filename for next looped call.
-            let resolve_result = dir
-              .resolve_module(&(url.to_string()), ".")
-              .map_err(DenoError::from);
-            match resolve_result {
-              Ok((new_module_name, new_filename)) => {
-                if maybe_initial_module_name.is_none() {
-                  maybe_initial_module_name = Some(module_name.clone());
-                  maybe_initial_filename = Some(filename.clone());
-                }
-                // Not yet completed. Follow the redirect and loop.
-                Ok(Loop::Continue((
-                  dir,
-                  maybe_initial_module_name,
-                  maybe_initial_filename,
-                  new_module_name,
-                  new_filename,
-                )))
-              }
-              Err(e) => Err(e),
+            let (new_module_name, new_filename) = dir
+              .resolve_module(&url.to_string(), ".")?;
+
+            if maybe_initial_module_name.is_none() {
+              maybe_initial_module_name = Some(module_name.clone());
+              maybe_initial_filename = Some(filename.clone());
             }
+
+            // Not yet completed. Follow the redirect and loop.
+            Ok(Loop::Continue((
+              dir,
+              maybe_initial_module_name,
+              maybe_initial_filename,
+              new_module_name,
+              new_filename,
+            )))
           }
           FetchOnceResult::Code(source, maybe_content_type) => {
             // We land on the code.
+            save_module_code_and_headers(
+              &filename.clone(),
+              &module_name.clone(),
+              &source,
+              maybe_content_type.clone(),
+              maybe_initial_filename,
+            )?;
+
             let p = PathBuf::from(filename.clone());
-            match p.parent() {
-              Some(ref parent) => fs::create_dir_all(parent),
-              None => Ok(()),
-            }?;
-            // Write file and create .headers.json for the file.
-            deno_fs::write_file(&p, &source, 0o666)?;
-            {
-              save_source_code_headers(
-                &filename,
-                maybe_content_type.clone(),
-                None,
-              );
-            }
-            // Check if this file is downloaded due to some old redirect request.
-            if maybe_initial_filename.is_some() {
-              // If yes, record down the headers for redirect.
-              // Also create its containing folder.
-              let pp = PathBuf::from(filename.clone());
-              match pp.parent() {
-                Some(ref parent) => fs::create_dir_all(parent),
-                None => Ok(()),
-              }?;
-              {
-                save_source_code_headers(
-                  &maybe_initial_filename.clone().unwrap(),
-                  maybe_content_type.clone(),
-                  Some(module_name.clone()),
-                );
-              }
-            }
-            Ok(Loop::Break(Some(ModuleMetaData {
+            let media_type = map_content_type(
+              &p,
+              maybe_content_type.as_ref().map(String::as_str),
+            );
+
+            let module_meta_data = ModuleMetaData {
               module_name: module_name.to_string(),
               module_redirect_source_name: maybe_initial_module_name,
               filename: filename.to_string(),
-              media_type: map_content_type(
-                &p,
-                maybe_content_type.as_ref().map(String::as_str),
-              ),
+              media_type,
               source_code: source.as_bytes().to_owned(),
               maybe_output_code_filename: None,
               maybe_output_code: None,
               maybe_source_map_filename: None,
               maybe_source_map: None,
-            })))
+            };
+
+            Ok(Loop::Break(Some(module_meta_data)))
           }
         }
       })
@@ -933,17 +983,17 @@ mod tests {
   use super::*;
   use tempfile::TempDir;
 
+  fn setup_deno_dir(dir_path: &Path) -> DenoDir {
+    let config = Some(b"{}".to_vec());
+    DenoDir::new(Some(dir_path.to_path_buf()), &config, Progress::new())
+      .expect("setup fail")
+  }
+
   fn test_setup() -> (TempDir, DenoDir) {
     let temp_dir = TempDir::new().expect("tempdir fail");
-    let config = Some(b"{}".to_vec());
-    let deno_dir = DenoDir::new(
-      Some(temp_dir.path().to_path_buf()),
-      &config,
-      Progress::new(),
-    ).expect("setup fail");
+    let deno_dir = setup_deno_dir(temp_dir.path());
     (temp_dir, deno_dir)
   }
-
   // The `add_root` macro prepends "C:" to a string if on windows; on posix
   // systems it returns the input string untouched. This is necessary because
   // `Url::from_file_path()` fails if the input path isn't an absolute path.
@@ -1099,7 +1149,7 @@ mod tests {
 
   #[test]
   fn test_get_source_code_1() {
-    let (_temp_dir, deno_dir) = test_setup();
+    let (temp_dir, deno_dir) = test_setup();
     // http_util::fetch_sync_string requires tokio
     tokio_util::init(|| {
       let module_name = "http://localhost:4545/tests/subdir/mod2.ts";
@@ -1165,7 +1215,9 @@ mod tests {
           .contains("application/json")
       );
 
-      // Don't use_cache
+      // let's create fresh instance of DenoDir (simulating another freshh Deno process)
+      // and don't use cache
+      let deno_dir = setup_deno_dir(temp_dir.path());
       let result4 =
         get_source_code(&deno_dir, module_name, &filename, false, false);
       assert!(result4.is_ok());
@@ -1181,7 +1233,7 @@ mod tests {
 
   #[test]
   fn test_get_source_code_2() {
-    let (_temp_dir, deno_dir) = test_setup();
+    let (temp_dir, deno_dir) = test_setup();
     // http_util::fetch_sync_string requires tokio
     tokio_util::init(|| {
       let module_name = "http://localhost:4545/tests/subdir/mismatch_ext.ts";
@@ -1223,7 +1275,9 @@ mod tests {
       assert_eq!(&(r2.media_type), &msg::MediaType::TypeScript);
       assert!(fs::read_to_string(&headers_file_name).is_err());
 
-      // Don't use_cache
+      // let's create fresh instance of DenoDir (simulating another freshh Deno process)
+      // and don't use cache
+      let deno_dir = setup_deno_dir(temp_dir.path());
       let result3 =
         get_source_code(&deno_dir, module_name, &filename, false, false);
       assert!(result3.is_ok());
@@ -1241,6 +1295,50 @@ mod tests {
   }
 
   #[test]
+  fn test_get_source_code_multiple_downloads_of_same_file() {
+    let (_temp_dir, deno_dir) = test_setup();
+    // http_util::fetch_sync_string requires tokio
+    tokio_util::init(|| {
+      let module_name = "http://localhost:4545/tests/subdir/mismatch_ext.ts";
+      let filename = deno_fs::normalize_path(
+        deno_dir
+          .deps_http
+          .join("localhost_PORT4545/tests/subdir/mismatch_ext.ts")
+          .as_ref(),
+      );
+      let headers_file_name = source_code_headers_filename(&filename);
+
+      // first download
+      let result =
+        get_source_code(&deno_dir, module_name, &filename, false, false);
+      assert!(result.is_ok());
+
+      let result = fs::File::open(&headers_file_name);
+      assert!(result.is_ok());
+      let headers_file = result.unwrap();
+      // save modified timestamp for headers file
+      let headers_file_metadata = headers_file.metadata().unwrap();
+      let headers_file_modified = headers_file_metadata.modified().unwrap();
+
+      // download file again, it should use already fetched file even though `use_cache` is set to
+      // false, this can be verified using source header file creation timestamp (should be
+      // the same as after first download)
+      let result =
+        get_source_code(&deno_dir, module_name, &filename, false, false);
+      assert!(result.is_ok());
+
+      let result = fs::File::open(&headers_file_name);
+      assert!(result.is_ok());
+      let headers_file_2 = result.unwrap();
+      // save modified timestamp for headers file
+      let headers_file_metadata_2 = headers_file_2.metadata().unwrap();
+      let headers_file_modified_2 = headers_file_metadata_2.modified().unwrap();
+
+      assert_eq!(headers_file_modified, headers_file_modified_2);
+    });
+  }
+
+  #[test]
   fn test_get_source_code_3() {
     let (_temp_dir, deno_dir) = test_setup();
     // Test basic follow and headers recording