From f7d1f82796ac49c43d5a0075f86cfd8f83d83889 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= <biwanczuk@gmail.com>
Date: Sun, 26 Apr 2020 19:00:10 +0200
Subject: [PATCH] core: add id field to RecursiveModuleLoad (#4905)

This commit unifies handling of ids for main module/dynamic
import loads in EsIsolate.
---
 core/bindings.rs   | 12 +++++-------
 core/es_isolate.rs | 16 +++++++---------
 core/modules.rs    | 26 ++++++++++++--------------
 3 files changed, 24 insertions(+), 30 deletions(-)

diff --git a/core/bindings.rs b/core/bindings.rs
index 25272af847..a081cccda4 100644
--- a/core/bindings.rs
+++ b/core/bindings.rs
@@ -277,13 +277,11 @@ pub extern "C" fn host_import_module_dynamically_callback(
   let mut resolver_handle = v8::Global::new();
   resolver_handle.set(scope, resolver);
 
-  let import_id = core_isolate.next_dyn_import_id;
-  core_isolate.next_dyn_import_id += 1;
-  core_isolate
-    .dyn_import_map
-    .insert(import_id, resolver_handle);
-
-  core_isolate.dyn_import_cb(&specifier_str, &referrer_name_str, import_id);
+  core_isolate.dyn_import_cb(
+    resolver_handle,
+    &specifier_str,
+    &referrer_name_str,
+  );
 
   &mut *scope.escape(promise)
 }
diff --git a/core/es_isolate.rs b/core/es_isolate.rs
index 8b6f6fb301..4b5fd58ba4 100644
--- a/core/es_isolate.rs
+++ b/core/es_isolate.rs
@@ -37,7 +37,7 @@ use crate::modules::Modules;
 use crate::modules::RecursiveModuleLoad;
 
 pub type ModuleId = i32;
-pub type DynImportId = i32;
+pub type ModuleLoadId = i32;
 
 /// More specialized version of `CoreIsolate` that provides loading
 /// and execution of ES Modules.
@@ -49,9 +49,8 @@ pub struct EsIsolate {
   core_isolate: Box<CoreIsolate>,
   loader: Rc<dyn ModuleLoader>,
   pub modules: Modules,
-  pub(crate) next_dyn_import_id: DynImportId,
   pub(crate) dyn_import_map:
-    HashMap<DynImportId, v8::Global<v8::PromiseResolver>>,
+    HashMap<ModuleLoadId, v8::Global<v8::PromiseResolver>>,
 
   pending_dyn_imports: FuturesUnordered<StreamFuture<RecursiveModuleLoad>>,
   waker: AtomicWaker,
@@ -92,7 +91,6 @@ impl EsIsolate {
       modules: Modules::new(),
       loader,
       core_isolate,
-      next_dyn_import_id: 0,
       dyn_import_map: HashMap::new(),
       pending_dyn_imports: FuturesUnordered::new(),
       waker: AtomicWaker::new(),
@@ -278,25 +276,25 @@ impl EsIsolate {
   // Called by V8 during `Isolate::mod_instantiate`.
   pub fn dyn_import_cb(
     &mut self,
+    resolver_handle: v8::Global<v8::PromiseResolver>,
     specifier: &str,
     referrer: &str,
-    id: DynImportId,
   ) {
     debug!("dyn_import specifier {} referrer {} ", specifier, referrer);
 
     let load = RecursiveModuleLoad::dynamic_import(
-      id,
       specifier,
       referrer,
       self.loader.clone(),
     );
+    self.dyn_import_map.insert(load.id, resolver_handle);
     self.waker.wake();
     self.pending_dyn_imports.push(load.into_future());
   }
 
   fn dyn_import_error(
     &mut self,
-    id: DynImportId,
+    id: ModuleLoadId,
     err: ErrBox,
   ) -> Result<(), ErrBox> {
     let core_isolate = &mut self.core_isolate;
@@ -331,7 +329,7 @@ impl EsIsolate {
 
   fn dyn_import_done(
     &mut self,
-    id: DynImportId,
+    id: ModuleLoadId,
     mod_id: ModuleId,
   ) -> Result<(), ErrBox> {
     debug!("dyn_import_done {} {:?}", id, mod_id);
@@ -373,7 +371,7 @@ impl EsIsolate {
         Poll::Ready(Some(load_stream_poll)) => {
           let maybe_result = load_stream_poll.0;
           let mut load = load_stream_poll.1;
-          let dyn_import_id = load.dyn_import_id.unwrap();
+          let dyn_import_id = load.id;
 
           if let Some(load_stream_result) = maybe_result {
             match load_stream_result {
diff --git a/core/modules.rs b/core/modules.rs
index fbdf21b81a..7e548ec824 100644
--- a/core/modules.rs
+++ b/core/modules.rs
@@ -3,8 +3,8 @@
 use rusty_v8 as v8;
 
 use crate::any_error::ErrBox;
-use crate::es_isolate::DynImportId;
 use crate::es_isolate::ModuleId;
+use crate::es_isolate::ModuleLoadId;
 use crate::module_specifier::ModuleSpecifier;
 use futures::future::FutureExt;
 use futures::stream::FuturesUnordered;
@@ -16,9 +16,15 @@ use std::fmt;
 use std::future::Future;
 use std::pin::Pin;
 use std::rc::Rc;
+use std::sync::atomic::AtomicI32;
+use std::sync::atomic::Ordering;
 use std::task::Context;
 use std::task::Poll;
 
+lazy_static! {
+  pub static ref NEXT_LOAD_ID: AtomicI32 = AtomicI32::new(0);
+}
+
 /// EsModule source code that will be loaded into V8.
 ///
 /// Users can implement `Into<ModuleInfo>` for different file types that
@@ -89,10 +95,8 @@ pub enum LoadState {
 /// that is consumed by the isolate.
 pub struct RecursiveModuleLoad {
   kind: Kind,
-  // Kind::Main
+  pub id: ModuleLoadId,
   pub root_module_id: Option<ModuleId>,
-  // Kind::Main
-  pub dyn_import_id: Option<DynImportId>,
   pub state: LoadState,
   pub loader: Rc<dyn ModuleLoader>,
   pub pending: FuturesUnordered<Pin<Box<ModuleSourceFuture>>>,
@@ -108,11 +112,10 @@ impl RecursiveModuleLoad {
   ) -> Self {
     let kind = Kind::Main;
     let state = LoadState::ResolveMain(specifier.to_owned(), code);
-    Self::new(kind, state, loader, None)
+    Self::new(kind, state, loader)
   }
 
   pub fn dynamic_import(
-    id: DynImportId,
     specifier: &str,
     referrer: &str,
     loader: Rc<dyn ModuleLoader>,
@@ -120,22 +123,17 @@ impl RecursiveModuleLoad {
     let kind = Kind::DynamicImport;
     let state =
       LoadState::ResolveImport(specifier.to_owned(), referrer.to_owned());
-    Self::new(kind, state, loader, Some(id))
+    Self::new(kind, state, loader)
   }
 
   pub fn is_dynamic_import(&self) -> bool {
     self.kind != Kind::Main
   }
 
-  fn new(
-    kind: Kind,
-    state: LoadState,
-    loader: Rc<dyn ModuleLoader>,
-    dyn_import_id: Option<DynImportId>,
-  ) -> Self {
+  fn new(kind: Kind, state: LoadState, loader: Rc<dyn ModuleLoader>) -> Self {
     Self {
+      id: NEXT_LOAD_ID.fetch_add(1, Ordering::SeqCst),
       root_module_id: None,
-      dyn_import_id,
       kind,
       state,
       loader,