fix(core): avoid op_state.borrow_mut() for OpsTracker (#12525)

By allowing interior mutability in OpsTracker (owning a RefCell<Vec> instead of just a Vec)

Fixes #12453

core/bindings.rs

@@ -351,7 +351,7 @@ fn opcall_sync<'s>(
   let op = OpTable::route_op(op_id, state.op_state.clone(), payload);
   match op {
     Op::Sync(result) => {
-      state.op_state.borrow_mut().tracker.track_sync(op_id);
+      state.op_state.borrow().tracker.track_sync(op_id);
       rv.set(result.to_v8(scope).unwrap());
     }
     Op::NotFound => {
@@ -421,12 +421,12 @@ fn opcall_async<'s>(
       OpResult::Err(_) => rv.set(result.to_v8(scope).unwrap()),
     },
     Op::Async(fut) => {
-      state.op_state.borrow_mut().tracker.track_async(op_id);
+      state.op_state.borrow().tracker.track_async(op_id);
       state.pending_ops.push(fut);
       state.have_unpolled_ops = true;
     }
     Op::AsyncUnref(fut) => {
-      state.op_state.borrow_mut().tracker.track_unref(op_id);
+      state.op_state.borrow().tracker.track_unref(op_id);
       state.pending_unref_ops.push(fut);
       state.have_unpolled_ops = true;
     }

core/ops.rs

@@ -172,7 +172,7 @@ impl OpState {
       op_table: OpTable::default(),
       get_error_class_fn: &|_| "Error",
       tracker: OpsTracker {
-        ops: Vec::with_capacity(256),
+        ops: RefCell::new(Vec::with_capacity(256)),
       },
       gotham_state: Default::default(),
     }

core/ops_metrics.rs

@@ -1,6 +1,8 @@
 // Copyright 2018-2021 the Deno authors. All rights reserved. MIT license.
 use crate::serde::Serialize;
 use crate::OpId;
+use std::cell::RefCell;
+use std::cell::RefMut;
 
 // TODO(@AaronO): split into AggregateMetrics & PerOpMetrics
 #[derive(Clone, Default, Debug, Serialize)]
@@ -22,18 +24,18 @@ pub struct OpMetrics {
 // TODO(@AaronO): track errors
 #[derive(Default, Debug)]
 pub struct OpsTracker {
-  pub ops: Vec<OpMetrics>,
+  pub ops: RefCell<Vec<OpMetrics>>,
 }
 
 impl OpsTracker {
   pub fn per_op(&self) -> Vec<OpMetrics> {
-    self.ops.clone()
+    self.ops.borrow().clone()
   }
 
   pub fn aggregate(&self) -> OpMetrics {
     let mut sum = OpMetrics::default();
 
-    for metrics in self.ops.iter() {
+    for metrics in self.ops.borrow().iter() {
       sum.ops_dispatched += metrics.ops_dispatched;
       sum.ops_dispatched_sync += metrics.ops_dispatched_sync;
       sum.ops_dispatched_async += metrics.ops_dispatched_async;
@@ -50,46 +52,47 @@ impl OpsTracker {
     sum
   }
 
-  fn ensure_capacity(&mut self, op_id: OpId) {
+  fn ensure_capacity(&self, op_id: OpId) {
-    if op_id >= self.ops.len() {
+    let ops = &mut self.ops.borrow_mut();
-      let delta_len = 1 + op_id - self.ops.len();
+    if op_id >= ops.len() {
-      self.ops.extend(vec![OpMetrics::default(); delta_len])
+      let delta_len = 1 + op_id - ops.len();
+      ops.extend(vec![OpMetrics::default(); delta_len])
     }
   }
 
-  fn metrics_mut(&mut self, id: OpId) -> &mut OpMetrics {
+  fn metrics_mut(&self, id: OpId) -> RefMut<OpMetrics> {
     self.ensure_capacity(id);
-    self.ops.get_mut(id).unwrap()
+    RefMut::map(self.ops.borrow_mut(), |ops| ops.get_mut(id).unwrap())
   }
 
-  pub fn track_sync(&mut self, id: OpId) {
+  pub fn track_sync(&self, id: OpId) {
-    let metrics = self.metrics_mut(id);
+    let metrics = &mut self.metrics_mut(id);
     metrics.ops_dispatched += 1;
     metrics.ops_completed += 1;
     metrics.ops_dispatched_sync += 1;
     metrics.ops_completed_sync += 1;
   }
 
-  pub fn track_async(&mut self, id: OpId) {
+  pub fn track_async(&self, id: OpId) {
-    let metrics = self.metrics_mut(id);
+    let metrics = &mut self.metrics_mut(id);
     metrics.ops_dispatched += 1;
     metrics.ops_dispatched_async += 1;
   }
 
-  pub fn track_async_completed(&mut self, id: OpId) {
+  pub fn track_async_completed(&self, id: OpId) {
-    let metrics = self.metrics_mut(id);
+    let metrics = &mut self.metrics_mut(id);
     metrics.ops_completed += 1;
     metrics.ops_completed_async += 1;
   }
 
-  pub fn track_unref(&mut self, id: OpId) {
+  pub fn track_unref(&self, id: OpId) {
-    let metrics = self.metrics_mut(id);
+    let metrics = &mut self.metrics_mut(id);
     metrics.ops_dispatched += 1;
     metrics.ops_dispatched_async_unref += 1;
   }
 
-  pub fn track_unref_completed(&mut self, id: OpId) {
+  pub fn track_unref_completed(&self, id: OpId) {
-    let metrics = self.metrics_mut(id);
+    let metrics = &mut self.metrics_mut(id);
     metrics.ops_completed += 1;
     metrics.ops_completed_async_unref += 1;
   }

core/runtime.rs

@@ -1476,8 +1476,7 @@ impl JsRuntime {
         Poll::Ready(None) => break,
         Poll::Pending => break,
         Poll::Ready(Some((promise_id, op_id, resp))) => {
-          let tracker = &mut state.op_state.borrow_mut().tracker;
+          state.op_state.borrow().tracker.track_async_completed(op_id);
-          tracker.track_async_completed(op_id);
           async_responses.push((promise_id, resp));
         }
       };
@@ -1489,8 +1488,7 @@ impl JsRuntime {
         Poll::Ready(None) => break,
         Poll::Pending => break,
         Poll::Ready(Some((promise_id, op_id, resp))) => {
-          let tracker = &mut state.op_state.borrow_mut().tracker;
+          state.op_state.borrow().tracker.track_unref_completed(op_id);
-          tracker.track_unref_completed(op_id);
           async_responses.push((promise_id, resp));
         }
       };
@@ -1606,6 +1604,7 @@ pub mod tests {
   use super::*;
   use crate::error::custom_error;
   use crate::modules::ModuleSourceFuture;
+  use crate::op_async;
   use crate::op_sync;
   use crate::ZeroCopyBuf;
   use futures::future::lazy;
@@ -2319,4 +2318,45 @@ assertEquals(1, notify_return_value);
     let all_true = v8::Local::<v8::Value>::new(&mut scope, &all_true);
     assert!(all_true.is_true());
   }
+
+  #[tokio::test]
+  async fn test_async_opstate_borrow() {
+    struct InnerState(u64);
+
+    async fn op_async_borrow(
+      op_state: Rc<RefCell<OpState>>,
+      _: (),
+      _: (),
+    ) -> Result<(), AnyError> {
+      let op_state = op_state.borrow();
+      let inner_state = op_state.borrow::<InnerState>();
+      // Future must be Poll::Pending on first call
+      tokio::time::sleep(std::time::Duration::from_millis(1)).await;
+      if inner_state.0 != 42 {
+        unreachable!();
+      }
+      Ok(())
+    }
+
+    let extension = Extension::builder()
+      .ops(vec![("op_async_borrow", op_async(op_async_borrow))])
+      .state(|state| {
+        state.put(InnerState(42));
+        Ok(())
+      })
+      .build();
+
+    let mut runtime = JsRuntime::new(RuntimeOptions {
+      extensions: vec![extension],
+      ..Default::default()
+    });
+
+    runtime
+      .execute_script(
+        "op_async_borrow.js",
+        "Deno.core.opAsync('op_async_borrow')",
+      )
+      .unwrap();
+    runtime.run_event_loop(false).await.unwrap();
+  }
 }