From 2a5138a5166f0945d5fda68c89fa8e23c66fb681 Mon Sep 17 00:00:00 2001
From: Ryan Dahl <ry@tinyclouds.org>
Date: Wed, 12 Jun 2019 10:53:24 -0700
Subject: [PATCH] Remove Config struct from core (#2502)

It's unnecessary indirection and is preventing the ability to easily
pass isolate references into the dispatch and dyn_import closures.

Note: this changes how StartupData::Script is executed. It's no longer done
during Isolate::new() but rather lazily on first poll or execution.
---
 cli/worker.rs               |  25 ++++---
 core/examples/http_bench.rs |   5 +-
 core/isolate.rs             | 126 ++++++++++++++++++------------------
 3 files changed, 78 insertions(+), 78 deletions(-)

diff --git a/cli/worker.rs b/cli/worker.rs
index f856ca0c0c..94d33e3919 100644
--- a/cli/worker.rs
+++ b/cli/worker.rs
@@ -5,7 +5,6 @@ use crate::js_errors;
 use crate::state::ThreadSafeState;
 use crate::tokio_util;
 use deno;
-use deno::Config;
 use deno::JSError;
 use deno::StartupData;
 use futures::Async;
@@ -18,7 +17,7 @@ use url::Url;
 /// high-level module loading
 #[derive(Clone)]
 pub struct Worker {
-  inner: Arc<Mutex<deno::Isolate>>,
+  isolate: Arc<Mutex<deno::Isolate>>,
   pub state: ThreadSafeState,
 }
 
@@ -29,14 +28,14 @@ impl Worker {
     state: ThreadSafeState,
   ) -> Worker {
     let state_ = state.clone();
-    let mut config = Config::default();
-    config.dispatch(move |control_buf, zero_copy_buf| {
-      state_.dispatch(control_buf, zero_copy_buf)
-    });
-    Self {
-      inner: Arc::new(Mutex::new(deno::Isolate::new(startup_data, config))),
-      state,
+    let isolate = Arc::new(Mutex::new(deno::Isolate::new(startup_data, false)));
+    {
+      let mut i = isolate.lock().unwrap();
+      i.set_dispatch(move |control_buf, zero_copy_buf| {
+        state_.dispatch(control_buf, zero_copy_buf)
+      });
     }
+    Self { isolate, state }
   }
 
   /// Same as execute2() but the filename defaults to "<anonymous>".
@@ -51,7 +50,7 @@ impl Worker {
     js_filename: &str,
     js_source: &str,
   ) -> Result<(), JSError> {
-    let mut isolate = self.inner.lock().unwrap();
+    let mut isolate = self.isolate.lock().unwrap();
     isolate.execute(js_filename, js_source)
   }
 
@@ -64,7 +63,7 @@ impl Worker {
     let worker = self.clone();
     let worker_ = worker.clone();
     let loader = self.state.clone();
-    let isolate = self.inner.clone();
+    let isolate = self.isolate.clone();
     let modules = self.state.modules.clone();
     let recursive_load =
       deno::RecursiveLoad::new(js_url.as_str(), loader, isolate, modules);
@@ -74,7 +73,7 @@ impl Worker {
         if is_prefetch {
           Ok(())
         } else {
-          let mut isolate = worker.inner.lock().unwrap();
+          let mut isolate = worker.isolate.lock().unwrap();
           let result = isolate.mod_evaluate(id);
           if let Err(err) = result {
             Err(deno::JSErrorOr::JSError(err))
@@ -166,7 +165,7 @@ impl Future for Worker {
   type Error = JSError;
 
   fn poll(&mut self) -> Result<Async<()>, Self::Error> {
-    let mut isolate = self.inner.lock().unwrap();
+    let mut isolate = self.isolate.lock().unwrap();
     isolate.poll().map_err(|err| self.apply_source_map(err))
   }
 }
diff --git a/core/examples/http_bench.rs b/core/examples/http_bench.rs
index 757e9a3b71..e8c5ec1b73 100644
--- a/core/examples/http_bench.rs
+++ b/core/examples/http_bench.rs
@@ -179,9 +179,8 @@ fn main() {
       filename: "http_bench.js",
     });
 
-    let mut config = deno::Config::default();
-    config.dispatch(dispatch);
-    let isolate = deno::Isolate::new(startup_data, config);
+    let mut isolate = deno::Isolate::new(startup_data, false);
+    isolate.set_dispatch(dispatch);
 
     isolate.then(|r| {
       js_check(r);
diff --git a/core/isolate.rs b/core/isolate.rs
index 18ac381838..14e1b88aad 100644
--- a/core/isolate.rs
+++ b/core/isolate.rs
@@ -42,6 +42,22 @@ pub struct Script<'a> {
   pub filename: &'a str,
 }
 
+// TODO(ry) It's ugly that we have both Script and OwnedScript. Ideally we
+// wouldn't expose such twiddly complexity.
+struct OwnedScript {
+  pub source: String,
+  pub filename: String,
+}
+
+impl<'a> From<Script<'a>> for OwnedScript {
+  fn from(s: Script<'a>) -> OwnedScript {
+    OwnedScript {
+      source: s.source.to_string(),
+      filename: s.filename.to_string(),
+    }
+  }
+}
+
 /// Represents data used to initialize isolate at startup
 /// either a binary snapshot or a javascript source file
 /// in the form of the StartupScript struct.
@@ -77,32 +93,6 @@ impl Future for DynImport {
   }
 }
 
-#[derive(Default)]
-pub struct Config {
-  dispatch: Option<Arc<DispatchFn>>,
-  dyn_import: Option<Arc<DynImportFn>>,
-  pub will_snapshot: bool,
-}
-
-impl Config {
-  /// Defines the how Deno.core.dispatch() acts.
-  /// Called whenever Deno.core.dispatch() is called in JavaScript. zero_copy_buf
-  /// corresponds to the second argument of Deno.core.dispatch().
-  pub fn dispatch<F>(&mut self, f: F)
-  where
-    F: Fn(&[u8], Option<PinnedBuf>) -> Op + Send + Sync + 'static,
-  {
-    self.dispatch = Some(Arc::new(f));
-  }
-
-  pub fn dyn_import<F>(&mut self, f: F)
-  where
-    F: Fn(&str, &str) -> DynImportFuture + Send + Sync + 'static,
-  {
-    self.dyn_import = Some(Arc::new(f));
-  }
-}
-
 /// A single execution context of JavaScript. Corresponds roughly to the "Web
 /// Worker" concept in the DOM. An Isolate is a Future that can be used with
 /// Tokio.  The Isolate future complete when there is an error or when all
@@ -114,12 +104,14 @@ impl Config {
 pub struct Isolate {
   libdeno_isolate: *const libdeno::isolate,
   shared_libdeno_isolate: Arc<Mutex<Option<*const libdeno::isolate>>>,
-  config: Config,
+  dispatch: Option<Arc<DispatchFn>>,
+  dyn_import: Option<Arc<DynImportFn>>,
   needs_init: bool,
   shared: SharedQueue,
   pending_ops: FuturesUnordered<OpAsyncFuture>,
   pending_dyn_imports: FuturesUnordered<DynImport>,
   have_unpolled_ops: bool,
+  startup_script: Option<OwnedScript>,
 }
 
 unsafe impl Send for Isolate {}
@@ -138,9 +130,7 @@ static DENO_INIT: Once = ONCE_INIT;
 impl Isolate {
   /// startup_data defines the snapshot or script used at startup to initalize
   /// the isolate.
-  // TODO(ry) move startup_data into Config. Ideally without introducing a
-  // generic lifetime into the Isolate struct...
-  pub fn new(startup_data: StartupData, config: Config) -> Self {
+  pub fn new(startup_data: StartupData, will_snapshot: bool) -> Self {
     DENO_INIT.call_once(|| {
       unsafe { libdeno::deno_init() };
     });
@@ -149,19 +139,20 @@ impl Isolate {
 
     let needs_init = true;
 
-    let mut startup_script: Option<Script> = None;
     let mut libdeno_config = libdeno::deno_config {
-      will_snapshot: if config.will_snapshot { 1 } else { 0 },
+      will_snapshot: will_snapshot.into(),
       load_snapshot: Snapshot2::empty(),
       shared: shared.as_deno_buf(),
       recv_cb: Self::pre_dispatch,
       dyn_import_cb: Self::dyn_import,
     };
 
+    let mut startup_script: Option<OwnedScript> = None;
+
     // Seperate into Option values for each startup type
     match startup_data {
       StartupData::Script(d) => {
-        startup_script = Some(d);
+        startup_script = Some(d.into());
       }
       StartupData::Snapshot(d) => {
         libdeno_config.load_snapshot = d.into();
@@ -174,23 +165,35 @@ impl Isolate {
 
     let libdeno_isolate = unsafe { libdeno::deno_new(libdeno_config) };
 
-    let mut core_isolate = Self {
+    Self {
       libdeno_isolate,
       shared_libdeno_isolate: Arc::new(Mutex::new(Some(libdeno_isolate))),
-      config,
+      dispatch: None,
+      dyn_import: None,
       shared,
       needs_init,
       pending_ops: FuturesUnordered::new(),
       have_unpolled_ops: false,
       pending_dyn_imports: FuturesUnordered::new(),
-    };
+      startup_script,
+    }
+  }
 
-    // If we want to use execute this has to happen here sadly.
-    if let Some(s) = startup_script {
-      core_isolate.execute(s.filename, s.source).unwrap()
-    };
+  /// Defines the how Deno.core.dispatch() acts.
+  /// Called whenever Deno.core.dispatch() is called in JavaScript. zero_copy_buf
+  /// corresponds to the second argument of Deno.core.dispatch().
+  pub fn set_dispatch<F>(&mut self, f: F)
+  where
+    F: Fn(&[u8], Option<PinnedBuf>) -> Op + Send + Sync + 'static,
+  {
+    self.dispatch = Some(Arc::new(f));
+  }
 
-    core_isolate
+  pub fn set_dyn_import<F>(&mut self, f: F)
+  where
+    F: Fn(&str, &str) -> DynImportFuture + Send + Sync + 'static,
+  {
+    self.dyn_import = Some(Arc::new(f));
   }
 
   /// Get a thread safe handle on the isolate.
@@ -201,12 +204,16 @@ impl Isolate {
   }
 
   /// Executes a bit of built-in JavaScript to provide Deno.sharedQueue.
-  pub fn shared_init(&mut self) {
+  fn shared_init(&mut self) {
     if self.needs_init {
       self.needs_init = false;
       js_check(
         self.execute("shared_queue.js", include_str!("shared_queue.js")),
       );
+      // Maybe execute the startup script.
+      if let Some(s) = self.startup_script.take() {
+        self.execute(&s.filename, &s.source).unwrap()
+      }
     }
   }
 
@@ -222,7 +229,7 @@ impl Isolate {
     let referrer = unsafe { CStr::from_ptr(referrer).to_str().unwrap() };
     debug!("dyn_import specifier {} referrer {} ", specifier, referrer);
 
-    if let Some(ref f) = isolate.config.dyn_import {
+    if let Some(ref f) = isolate.dyn_import {
       let inner = f(specifier, referrer);
       let fut = DynImport { inner, id };
       task::current().notify();
@@ -242,17 +249,17 @@ impl Isolate {
 
     let op = if control_argv0.len() > 0 {
       // The user called Deno.core.send(control)
-      if let Some(ref f) = isolate.config.dispatch {
+      if let Some(ref f) = isolate.dispatch {
         f(control_argv0.as_ref(), PinnedBuf::new(zero_copy_buf))
       } else {
-        panic!("isolate.config.dispatch not set")
+        panic!("isolate.dispatch not set")
       }
     } else if let Some(c) = control_shared {
       // The user called Deno.sharedQueue.push(control)
-      if let Some(ref f) = isolate.config.dispatch {
+      if let Some(ref f) = isolate.dispatch {
         f(&c, PinnedBuf::new(zero_copy_buf))
       } else {
-        panic!("isolate.config.dispatch not set")
+        panic!("isolate.dispatch not set")
       }
     } else {
       // The sharedQueue is empty. The shouldn't happen usually, but it's also
@@ -516,6 +523,8 @@ impl Future for Isolate {
     // Lock the current thread for V8.
     let _locker = LockerScope::new(self.libdeno_isolate);
 
+    self.shared_init();
+
     let mut overflow_response: Option<Buf> = None;
 
     loop {
@@ -654,8 +663,8 @@ pub mod tests {
     let dispatch_count = Arc::new(AtomicUsize::new(0));
     let dispatch_count_ = dispatch_count.clone();
 
-    let mut config = Config::default();
-    config.dispatch(move |control, _| -> Op {
+    let mut isolate = Isolate::new(StartupData::None, false);
+    isolate.set_dispatch(move |control, _| -> Op {
       dispatch_count_.fetch_add(1, Ordering::Relaxed);
       match mode {
         Mode::AsyncImmediate => {
@@ -694,8 +703,6 @@ pub mod tests {
         }
       }
     });
-
-    let mut isolate = Isolate::new(StartupData::None, config);
     js_check(isolate.execute(
       "setup.js",
       r#"
@@ -861,14 +868,13 @@ pub mod tests {
     run_in_task(|| {
       let count = Arc::new(AtomicUsize::new(0));
       let count_ = count.clone();
-      let mut config = Config::default();
-      config.dyn_import(move |specifier, referrer| {
+      let mut isolate = Isolate::new(StartupData::None, false);
+      isolate.set_dyn_import(move |specifier, referrer| {
         count_.fetch_add(1, Ordering::Relaxed);
         assert_eq!(specifier, "foo.js");
         assert_eq!(referrer, "dyn_import2.js");
         Box::new(futures::future::err(()))
       });
-      let mut isolate = Isolate::new(StartupData::None, config);
       js_check(isolate.execute(
         "dyn_import2.js",
         r#"
@@ -896,8 +902,8 @@ pub mod tests {
       let mod_b = Arc::new(Mutex::new(0));
       let mod_b2 = mod_b.clone();
 
-      let mut config = Config::default();
-      config.dyn_import(move |_specifier, referrer| {
+      let mut isolate = Isolate::new(StartupData::None, false);
+      isolate.set_dyn_import(move |_specifier, referrer| {
         count_.fetch_add(1, Ordering::Relaxed);
         // assert_eq!(specifier, "foo.js");
         assert_eq!(referrer, "dyn_import3.js");
@@ -905,8 +911,6 @@ pub mod tests {
         Box::new(futures::future::ok(*mod_id))
       });
 
-      let mut isolate = Isolate::new(StartupData::None, config);
-
       // Instantiate mod_b
       {
         let mut mod_id = mod_b.lock().unwrap();
@@ -1159,9 +1163,7 @@ pub mod tests {
   #[test]
   fn will_snapshot() {
     let snapshot = {
-      let mut config = Config::default();
-      config.will_snapshot = true;
-      let mut isolate = Isolate::new(StartupData::None, config);
+      let mut isolate = Isolate::new(StartupData::None, true);
       js_check(isolate.execute("a.js", "a = 1 + 2"));
       let s = isolate.snapshot().unwrap();
       drop(isolate);
@@ -1169,7 +1171,7 @@ pub mod tests {
     };
 
     let startup_data = StartupData::LibdenoSnapshot(snapshot);
-    let mut isolate2 = Isolate::new(startup_data, Config::default());
+    let mut isolate2 = Isolate::new(startup_data, false);
     js_check(isolate2.execute("check.js", "if (a != 3) throw Error('x')"));
   }
 }