Modify op dispatcher to include &mut Isolate argument (#4821)

- Removes unnecessary RwLock and Rc around the op registry table
- Preparation to move resource_table to deno_core::Isolate.
- Towards #3453, #4222

1 files changed, 57 insertions, 62 deletions

diff --git a/core/ops.rs b/core/ops.rs
index ab183f4de..ed9b27e46 100644
--- a/core/ops.rs
+++ b/core/ops.rs
@@ -1,10 +1,10 @@
 // Copyright 2018-2020 the Deno authors. All rights reserved. MIT license.
+use crate::Isolate;
 use crate::ZeroCopyBuf;
 use futures::Future;
 use std::collections::HashMap;
 use std::pin::Pin;
 use std::rc::Rc;
-use std::sync::RwLock;
 
 pub type OpId = u32;
 
@@ -21,72 +21,48 @@ pub enum Op {
 }
 
 /// Main type describing op
-pub type OpDispatcher = dyn Fn(&[u8], Option<ZeroCopyBuf>) -> Op + 'static;
+pub type OpDispatcher =
+  dyn Fn(&mut Isolate, &[u8], Option<ZeroCopyBuf>) -> Op + 'static;
 
 #[derive(Default)]
 pub struct OpRegistry {
-  dispatchers: RwLock<Vec<Rc<OpDispatcher>>>,
-  name_to_id: RwLock<HashMap<String, OpId>>,
+  dispatchers: Vec<Rc<OpDispatcher>>,
+  name_to_id: HashMap<String, OpId>,
 }
 
 impl OpRegistry {
   pub fn new() -> Self {
-    let registry = Self::default();
-    let op_id = registry.register("ops", |_, _| {
-      // ops is a special op which is handled in call.
-      unreachable!()
+    let mut registry = Self::default();
+    let op_id = registry.register("ops", |isolate, _, _| {
+      let buf = isolate.op_registry.json_map();
+      Op::Sync(buf)
     });
     assert_eq!(op_id, 0);
     registry
   }
 
-  pub fn register<F>(&self, name: &str, op: F) -> OpId
+  pub fn register<F>(&mut self, name: &str, op: F) -> OpId
   where
-    F: Fn(&[u8], Option<ZeroCopyBuf>) -> Op + 'static,
+    F: Fn(&mut Isolate, &[u8], Option<ZeroCopyBuf>) -> Op + 'static,
   {
-    let mut lock = self.dispatchers.write().unwrap();
-    let op_id = lock.len() as u32;
+    let op_id = self.dispatchers.len() as u32;
 
-    let mut name_lock = self.name_to_id.write().unwrap();
-    let existing = name_lock.insert(name.to_string(), op_id);
+    let existing = self.name_to_id.insert(name.to_string(), op_id);
     assert!(
       existing.is_none(),
       format!("Op already registered: {}", name)
     );
-    lock.push(Rc::new(op));
-    drop(name_lock);
-    drop(lock);
+    self.dispatchers.push(Rc::new(op));
     op_id
   }
 
   fn json_map(&self) -> Buf {
-    let lock = self.name_to_id.read().unwrap();
-    let op_map_json = serde_json::to_string(&*lock).unwrap();
+    let op_map_json = serde_json::to_string(&self.name_to_id).unwrap();
     op_map_json.as_bytes().to_owned().into_boxed_slice()
   }
 
-  /// This function returns None only if op with given id doesn't exist in registry.
-  pub fn call(
-    &self,
-    op_id: OpId,
-    control: &[u8],
-    zero_copy_buf: Option<ZeroCopyBuf>,
-  ) -> Option<Op> {
-    // Op with id 0 has special meaning - it's a special op that is always
-    // provided to retrieve op id map. The map consists of name to `OpId`
-    // mappings.
-    if op_id == 0 {
-      return Some(Op::Sync(self.json_map()));
-    }
-    let lock = self.dispatchers.read().unwrap();
-    if let Some(op) = lock.get(op_id as usize) {
-      let op_ = Rc::clone(&op);
-      // This should allow for changes to the dispatcher list during a call.
-      drop(lock);
-      Some(op_(control, zero_copy_buf))
-    } else {
-      None
-    }
+  pub fn get(&self, op_id: OpId) -> Option<Rc<OpDispatcher>> {
+    self.dispatchers.get(op_id as usize).map(Rc::clone)
   }
 }
 
@@ -94,12 +70,12 @@ impl OpRegistry {
 fn test_op_registry() {
   use std::sync::atomic;
   use std::sync::Arc;
-  let op_registry = OpRegistry::new();
+  let mut op_registry = OpRegistry::new();
 
   let c = Arc::new(atomic::AtomicUsize::new(0));
   let c_ = c.clone();
 
-  let test_id = op_registry.register("test", move |_, _| {
+  let test_id = op_registry.register("test", move |_, _, _| {
     c_.fetch_add(1, atomic::Ordering::SeqCst);
     Op::Sync(Box::new([]))
   });
@@ -108,10 +84,12 @@ fn test_op_registry() {
   let mut expected = HashMap::new();
   expected.insert("ops".to_string(), 0);
   expected.insert("test".to_string(), 1);
-  let name_to_id = op_registry.name_to_id.read().unwrap();
-  assert_eq!(*name_to_id, expected);
+  assert_eq!(op_registry.name_to_id, expected);
 
-  let res = op_registry.call(test_id, &[], None).unwrap();
+  let mut isolate = Isolate::new(crate::StartupData::None, false);
+
+  let dispatch = op_registry.get(test_id).unwrap();
+  let res = dispatch(&mut isolate, &[], None);
   if let Op::Sync(buf) = res {
     assert_eq!(buf.len(), 0);
   } else {
@@ -119,40 +97,57 @@ fn test_op_registry() {
   }
   assert_eq!(c.load(atomic::Ordering::SeqCst), 1);
 
-  let res = op_registry.call(100, &[], None);
-  assert!(res.is_none());
+  assert!(op_registry.get(100).is_none());
 }
 
 #[test]
 fn register_op_during_call() {
   use std::sync::atomic;
   use std::sync::Arc;
-  let op_registry = Arc::new(OpRegistry::new());
+  use std::sync::Mutex;
+  let op_registry = Arc::new(Mutex::new(OpRegistry::new()));
 
   let c = Arc::new(atomic::AtomicUsize::new(0));
   let c_ = c.clone();
 
   let op_registry_ = op_registry.clone();
-  let test_id = op_registry.register("dynamic_register_op", move |_, _| {
-    let c__ = c_.clone();
-    op_registry_.register("test", move |_, _| {
-      c__.fetch_add(1, atomic::Ordering::SeqCst);
+
+  let test_id = {
+    let mut g = op_registry.lock().unwrap();
+    g.register("dynamic_register_op", move |_, _, _| {
+      let c__ = c_.clone();
+      let mut g = op_registry_.lock().unwrap();
+      g.register("test", move |_, _, _| {
+        c__.fetch_add(1, atomic::Ordering::SeqCst);
+        Op::Sync(Box::new([]))
+      });
       Op::Sync(Box::new([]))
-    });
-    Op::Sync(Box::new([]))
-  });
+    })
+  };
   assert!(test_id != 0);
 
-  op_registry.call(test_id, &[], None);
+  let mut isolate = Isolate::new(crate::StartupData::None, false);
+
+  let dispatcher1 = {
+    let g = op_registry.lock().unwrap();
+    g.get(test_id).unwrap()
+  };
+  dispatcher1(&mut isolate, &[], None);
 
   let mut expected = HashMap::new();
   expected.insert("ops".to_string(), 0);
   expected.insert("dynamic_register_op".to_string(), 1);
   expected.insert("test".to_string(), 2);
-  let name_to_id = op_registry.name_to_id.read().unwrap();
-  assert_eq!(*name_to_id, expected);
+  {
+    let g = op_registry.lock().unwrap();
+    assert_eq!(g.name_to_id, expected);
+  }
 
-  let res = op_registry.call(2, &[], None).unwrap();
+  let dispatcher2 = {
+    let g = op_registry.lock().unwrap();
+    g.get(2).unwrap()
+  };
+  let res = dispatcher2(&mut isolate, &[], None);
   if let Op::Sync(buf) = res {
     assert_eq!(buf.len(), 0);
   } else {
@@ -160,6 +155,6 @@ fn register_op_during_call() {
   }
   assert_eq!(c.load(atomic::Ordering::SeqCst), 1);
 
-  let res = op_registry.call(100, &[], None);
-  assert!(res.is_none());
+  let g = op_registry.lock().unwrap();
+  assert!(g.get(100).is_none());
 }