Repository: denoland/deno_core
Pull request #56 (2023-07-19 17:47:05)

fix(modules): check duplicate loads for aliases
Supersedes #14. Checking the module map in load futures no longer works without borrow issues since the modules-in-realms work.

Normally `RecursiveModuleLoad::visited` prevents duplicate loads from happening, but it doesn't work for redirected/aliased modules for when both the alias and actual specifiers are used.

This adds `RecursiveModuleLoad::visited_by_alias` which targets these cases and is checked just before the loader is invoked.

CLAassistant: [![CLA assistant check](https://cla-assistant.io/pull/badge/not_signed)](https://cla-assistant.io/denoland/deno_core?pullRequest=56) <br/>Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our [Contributor License Agreement](https://cla-assistant.io/denoland/deno_core?pullRequest=56) before we can accept your contribution.<br/>**1** out of **2** committers have signed the CLA.<br/><br/>:white_check_mark: bartlomieju<br/>:x: nayeemrmn<br/><sub>You have signed the CLA already but the status is still pending? Let us [recheck](https://cla-assistant.io/check/denoland/deno_core?pullRequest=56) it.</sub>

Patch:
diff --git a/core/modules/mod.rs b/core/modules/mod.rs
index 83f10bc..dd7323d 100644
--- a/core/modules/mod.rs
+++ b/core/modules/mod.rs
@@ -242,7 +242,7 @@ pub(crate) type PrepareLoadFuture =
 pub type ModuleSourceFuture = dyn Future<Output = Result<ModuleSource, Error>>;
 
 type ModuleLoadFuture =
-  dyn Future<Output = Result<(ModuleRequest, ModuleSource), Error>>;
+  dyn Future<Output = Result<Option<(ModuleRequest, ModuleSource)>, Error>>;
 
 #[derive(Debug, PartialEq, Eq)]
 pub enum ResolutionKind {
@@ -290,6 +290,7 @@ pub(crate) struct RecursiveModuleLoad {
   module_map_rc: Rc<RefCell<ModuleMap>>,
   pending: FuturesUnordered<Pin<Box<ModuleLoadFuture>>>,
   visited: HashSet<ModuleRequest>,
+  visited_as_alias: Rc<RefCell<HashSet<String>>>,
   // The loader is copied from `module_map_rc`, but its reference is cloned
   // ahead of time to avoid already-borrowed errors.
   loader: Rc<dyn ModuleLoader>,
@@ -350,6 +351,7 @@ impl RecursiveModuleLoad {
       loader,
       pending: FuturesUnordered::new(),
       visited: HashSet::new(),
+      visited_as_alias: Default::default(),
     };
     // FIXME(bartlomieju): this seems fishy
     // Ignore the error here, let it be hit in `Stream::poll_next()`.
@@ -513,7 +515,12 @@ impl RecursiveModuleLoad {
         .unwrap()
         .clone();
       for module_request in imports {
-        if !self.visited.contains(&module_request) {
+        if !self.visited.contains(&module_request)
+          && !self
+            .visited_as_alias
+            .borrow()
+            .contains(&module_request.specifier)
+        {
           if let Some(module_id) = self.module_map_rc.borrow().get_id(
             module_request.specifier.as_str(),
             module_request.asserted_module_type,
@@ -523,14 +530,29 @@ impl RecursiveModuleLoad {
             let request = module_request.clone();
             let specifier =
               ModuleSpecifier::parse(&module_request.specifier).unwrap();
+            let visited_as_alias = self.visited_as_alias.clone();
             let referrer = referrer.clone();
             let loader = self.loader.clone();
             let is_dynamic_import = self.is_dynamic_import();
             let fut = async move {
+              // `visited_as_alias` unlike `visited` is checked as late as
+              // possible because it can only be populated after completed
+              // loads, meaning a duplicate load future may have already been
+              // dispatched before we know it's a duplicate.
+              if visited_as_alias.borrow().contains(specifier.as_str()) {
+                return Ok(None);
+              }
               let load_result = loader
                 .load(&specifier, Some(&referrer), is_dynamic_import)
                 .await;
-              load_result.map(|s| (request, s))
+              if let Ok(source) = &load_result {
+                if let Some(found_specifier) = &source.module_url_found {
+                  visited_as_alias
+                    .borrow_mut()
+                    .insert(found_specifier.as_str().to_string());
+                }
+              }
+              load_result.map(|s| Some((request, s)))
             };
             self.pending.push(fut.boxed_local());
           }
@@ -590,7 +612,7 @@ impl Stream for RecursiveModuleLoad {
             Default::default(),
             &module_specifier,
           );
-          futures::future::ok((module_request, module_source)).boxed()
+          futures::future::ok(Some((module_request, module_source))).boxed()
         } else {
           let maybe_referrer = match inner.init {
             LoadInit::DynamicImport(_, ref referrer, _) => {
@@ -616,7 +638,7 @@ impl Stream for RecursiveModuleLoad {
                 is_dynamic_import,
               )
               .await;
-            result.map(|s| (module_request, s))
+            result.map(|s| Some((module_request, s)))
           }
           .boxed_local()
         };
@@ -627,7 +649,15 @@ impl Stream for RecursiveModuleLoad {
       LoadState::LoadingRoot | LoadState::LoadingImports => {
         match inner.pending.try_poll_next_unpin(cx)? {
           Poll::Ready(None) => unreachable!(),
-          Poll::Ready(Some(info)) => Poll::Ready(Some(Ok(info))),
+          Poll::Ready(Some(None)) => {
+            if inner.pending.is_empty() {
+              inner.state = LoadState::Done;
+              Poll::Ready(None)
+            } else {
+              Poll::Pending
+            }
+          }
+          Poll::Ready(Some(Some(info))) => Poll::Ready(Some(Ok(info))),
           Poll::Pending => Poll::Pending,
         }
       }


Test patch:
diff --git a/core/modules/tests.rs b/core/modules/tests.rs
index 0eb7ce5..d9aad41 100644
--- a/core/modules/tests.rs
+++ b/core/modules/tests.rs
@@ -1250,3 +1250,109 @@ fn import_meta_snapshot() {
     )
     .unwrap();
 }
+
+#[tokio::test]
+async fn no_duplicate_loads() {
+  // Both of these imports will cause redirects - ie. their "found" specifier
+  // will be different than requested specifier.
+  const MAIN_SRC: &str = r#"
+  import "https://example.com/foo.js";
+  import "https://example.com/bar.js";
+  "#;
+
+  // foo.js is importing bar.js that is already redirected
+  const FOO_SRC: &str = r#"
+  import "https://example.com/v1/bar.js";
+  "#;
+
+  struct ConsumingLoader {
+    pub files: Rc<RefCell<HashMap<String, Option<String>>>>,
+  }
+
+  impl Default for ConsumingLoader {
+    fn default() -> Self {
+      let files = HashMap::from_iter([
+        (
+          "https://example.com/v1/foo.js".to_string(),
+          Some(FOO_SRC.to_string()),
+        ),
+        (
+          "https://example.com/v1/bar.js".to_string(),
+          Some("".to_string()),
+        ),
+        ("file:///main.js".to_string(), Some(MAIN_SRC.to_string())),
+      ]);
+      Self {
+        files: Rc::new(RefCell::new(files)),
+      }
+    }
+  }
+
+  impl ModuleLoader for ConsumingLoader {
+    fn resolve(
+      &self,
+      specifier: &str,
+      referrer: &str,
+      _kind: ResolutionKind,
+    ) -> Result<ModuleSpecifier, Error> {
+      let referrer = if referrer == "." {
+        "file:///"
+      } else {
+        referrer
+      };
+
+      Ok(resolve_import(specifier, referrer)?)
+    }
+
+    fn load(
+      &self,
+      module_specifier: &ModuleSpecifier,
+      _maybe_referrer: Option<&ModuleSpecifier>,
+      _is_dyn_import: bool,
+    ) -> Pin<Box<ModuleSourceFuture>> {
+      let found_specifier =
+        if module_specifier.as_str() == "https://example.com/foo.js" {
+          Some("https://example.com/v1/foo.js".to_string())
+        } else if module_specifier.as_str() == "https://example.com/bar.js" {
+          Some("https://example.com/v1/bar.js".to_string())
+        } else {
+          None
+        };
+
+      let mut files = self.files.borrow_mut();
+      eprintln!(
+        "getting specifier {} {:?}",
+        module_specifier.as_str(),
+        found_specifier
+      );
+      let source_code = if let Some(found) = &found_specifier {
+        files.get_mut(found).unwrap().take().unwrap()
+      } else {
+        files
+          .get_mut(module_specifier.as_str())
+          .unwrap()
+          .take()
+          .unwrap()
+      };
+      let module_source = ModuleSource {
+        code: ModuleCode::from(source_code),
+        module_type: ModuleType::JavaScript,
+        module_url_specified: module_specifier.clone().into(),
+        module_url_found: found_specifier.map(|s| s.into()),
+      };
+      async move { Ok(module_source) }.boxed_local()
+    }
+  }
+
+  let loader = Rc::new(ConsumingLoader::default());
+  let mut runtime = JsRuntime::new(RuntimeOptions {
+    module_loader: Some(loader),
+    ..Default::default()
+  });
+
+  let spec = resolve_url("file:///main.js").unwrap();
+  let a_id = runtime.load_main_module(&spec, None).await.unwrap();
+  #[allow(clippy::let_underscore_future)]
+  let _ = runtime.mod_evaluate(a_id);
+  runtime.run_event_loop(false).await.unwrap();
+}