fix(ops): disallow auto-borrowing OpState across potential await point (

denoland#16952) Fixes denoland#16934 Example compiler error: ``` error: mutable opstate is not supported in async ops --> core/ops_builtin.rs:122:1 | 122 | #[op] | ^^^^^ | = note: this error originates in the attribute macro `op` (in Nightly builds, run with -Z macro-backtrace for more info) ```
DavidDeSimone · Dec 5, 2022 · 55595ca · 55595ca
1 parent 715f35d
commit 55595ca
Show file tree

Hide file tree

Showing 10 changed files with 234 additions and 5 deletions.
diff --git a/ext/ffi/lib.rs b/ext/ffi/lib.rs
@@ -1829,9 +1829,10 @@ impl Future for CallbackInfo {
 
 #[op]
 fn op_ffi_unsafe_callback_ref(
-  state: &mut deno_core::OpState,
+  state: Rc<RefCell<deno_core::OpState>>,
   rid: ResourceId,
 ) -> Result<impl Future<Output = Result<(), AnyError>>, AnyError> {
+  let state = state.borrow();
   let callback_resource =
     state.resource_table.get::<UnsafeCallbackResource>(rid)?;
 

diff --git a/ext/flash/lib.rs b/ext/flash/lib.rs
@@ -1290,10 +1290,11 @@ where
 
 #[op]
 fn op_flash_wait_for_listening(
-  state: &mut OpState,
+  state: Rc<RefCell<OpState>>,
   server_id: u32,
 ) -> Result<impl Future<Output = Result<u16, AnyError>> + 'static, AnyError> {
   let mut listening_rx = {
+    let mut state = state.borrow_mut();
     let flash_ctx = state.borrow_mut::<FlashContext>();
     let server_ctx = flash_ctx
       .servers
@@ -1312,10 +1313,11 @@ fn op_flash_wait_for_listening(
 
 #[op]
 fn op_flash_drive_server(
-  state: &mut OpState,
+  state: Rc<RefCell<OpState>>,
   server_id: u32,
 ) -> Result<impl Future<Output = Result<(), AnyError>> + 'static, AnyError> {
   let join_handle = {
+    let mut state = state.borrow_mut();
     let flash_ctx = state.borrow_mut::<FlashContext>();
     flash_ctx
       .join_handles

diff --git a/ops/lib.rs b/ops/lib.rs
@@ -193,7 +193,8 @@ fn codegen_v8_async(
     .inputs
     .iter()
     .map_while(|a| {
-      (if is_v8 { scope_arg(a) } else { None }).or_else(|| opstate_arg(a))
+      (if is_v8 { scope_arg(a) } else { None })
+        .or_else(|| rc_refcell_opstate_arg(a))
     })
     .collect::<Vec<_>>();
   let rust_i0 = special_args.len();
@@ -291,6 +292,16 @@ fn opstate_arg(arg: &FnArg) -> Option<TokenStream2> {
   }
 }
 
+fn rc_refcell_opstate_arg(arg: &FnArg) -> Option<TokenStream2> {
+  match arg {
+    arg if is_rc_refcell_opstate(arg) => Some(quote! { ctx.state.clone(), }),
+    arg if is_mut_ref_opstate(arg) => Some(
+      quote! { compile_error!("mutable opstate is not supported in async ops"), },
+    ),
+    _ => None,
+  }
+}
+
 /// Generate the body of a v8 func for a sync op
 fn codegen_v8_sync(
   core: &TokenStream2,

diff --git a/ops/optimizer.rs b/ops/optimizer.rs
@@ -650,7 +650,9 @@ impl Optimizer {
             let segment = single_segment(segments)?;
             match segment {
               // Is `T` a OpState?
-              PathSegment { ident, .. } if ident == "OpState" => {
+              PathSegment { ident, .. }
+                if ident == "OpState" && !self.is_async =>
+              {
                 self.has_ref_opstate = true;
               }
               // Is `T` a str?

diff --git a/ops/optimizer_tests/issue16934.expected b/ops/optimizer_tests/issue16934.expected
@@ -0,0 +1,11 @@
+=== Optimizer Dump ===
+returns_result: false
+has_ref_opstate: false
+has_rc_opstate: false
+has_fast_callback_option: false
+needs_fast_callback_option: false
+fast_result: None
+fast_parameters: []
+transforms: {}
+is_async: false
+fast_compatible: false
diff --git a/ops/optimizer_tests/issue16934.out b/ops/optimizer_tests/issue16934.out
@@ -0,0 +1,92 @@
+#[allow(non_camel_case_types)]
+///Auto-generated by `deno_ops`, i.e: `#[op]`
+///
+///Use `send_stdin::decl()` to get an op-declaration
+///you can include in a `deno_core::Extension`.
+pub struct send_stdin;
+#[doc(hidden)]
+impl send_stdin {
+    pub fn name() -> &'static str {
+        stringify!(send_stdin)
+    }
+    pub fn v8_fn_ptr<'scope>() -> deno_core::v8::FunctionCallback {
+        use deno_core::v8::MapFnTo;
+        Self::v8_func.map_fn_to()
+    }
+    pub fn decl<'scope>() -> deno_core::OpDecl {
+        deno_core::OpDecl {
+            name: Self::name(),
+            v8_fn_ptr: Self::v8_fn_ptr(),
+            enabled: true,
+            fast_fn: None,
+            is_async: true,
+            is_unstable: false,
+            is_v8: false,
+            argc: 1usize,
+        }
+    }
+    #[inline]
+    #[allow(clippy::too_many_arguments)]
+    async fn call(state: &mut OpState, cmd: String) -> Result<(), anyhow::Error> {
+        let instance = state.borrow::<MinecraftInstance>().clone();
+        instance.send_command(&cmd, CausedBy::Unknown).await?;
+        Ok(())
+    }
+    pub fn v8_func<'scope>(
+        scope: &mut deno_core::v8::HandleScope<'scope>,
+        args: deno_core::v8::FunctionCallbackArguments,
+        mut rv: deno_core::v8::ReturnValue,
+    ) {
+        use deno_core::futures::FutureExt;
+        let ctx = unsafe {
+            &*(deno_core::v8::Local::<deno_core::v8::External>::cast(args.data()).value()
+                as *const deno_core::_ops::OpCtx)
+        };
+        let op_id = ctx.id;
+        let promise_id = args.get(0);
+        let promise_id = deno_core::v8::Local::<
+            deno_core::v8::Integer,
+        >::try_from(promise_id)
+            .map(|l| l.value() as deno_core::PromiseId)
+            .map_err(deno_core::anyhow::Error::from);
+        let promise_id: deno_core::PromiseId = match promise_id {
+            Ok(promise_id) => promise_id,
+            Err(err) => {
+                deno_core::_ops::throw_type_error(
+                    scope,
+                    format!("invalid promise id: {}", err),
+                );
+                return;
+            }
+        };
+        let arg_0 = match deno_core::v8::Local::<
+            deno_core::v8::String,
+        >::try_from(args.get(1usize as i32)) {
+            Ok(v8_string) => deno_core::serde_v8::to_utf8(v8_string, scope),
+            Err(_) => {
+                return deno_core::_ops::throw_type_error(
+                    scope,
+                    format!("Expected string at position {}", 1usize),
+                );
+            }
+        };
+        let get_class = {
+            let state = ::std::cell::RefCell::borrow(&ctx.state);
+            state.tracker.track_async(op_id);
+            state.get_error_class_fn
+        };
+        deno_core::_ops::queue_async_op(
+            ctx,
+            scope,
+            false,
+            async move {
+                let result = Self::call(
+                        compile_error!("mutable opstate is not supported in async ops"),
+                        arg_0,
+                    )
+                    .await;
+                (promise_id, op_id, deno_core::_ops::to_op_result(get_class, result))
+            },
+        );
+    }
+}
diff --git a/ops/optimizer_tests/issue16934.rs b/ops/optimizer_tests/issue16934.rs
@@ -0,0 +1,11 @@
+async fn send_stdin(
+  state: &mut OpState,
+  cmd: String,
+) -> Result<(), anyhow::Error> {
+  // https://github.com/denoland/deno/issues/16934
+  //
+  // OpState borrowed across await point is not allowed, as it will likely panic at runtime.
+  let instance = state.borrow::<MinecraftInstance>().clone();
+  instance.send_command(&cmd, CausedBy::Unknown).await?;
+  Ok(())
+}
diff --git a/ops/optimizer_tests/issue16934_fast.expected b/ops/optimizer_tests/issue16934_fast.expected
@@ -0,0 +1 @@
+FastUnsupportedParamType
diff --git a/ops/optimizer_tests/issue16934_fast.out b/ops/optimizer_tests/issue16934_fast.out
@@ -0,0 +1,90 @@
+#[allow(non_camel_case_types)]
+///Auto-generated by `deno_ops`, i.e: `#[op]`
+///
+///Use `send_stdin::decl()` to get an op-declaration
+///you can include in a `deno_core::Extension`.
+pub struct send_stdin;
+#[doc(hidden)]
+impl send_stdin {
+    pub fn name() -> &'static str {
+        stringify!(send_stdin)
+    }
+    pub fn v8_fn_ptr<'scope>() -> deno_core::v8::FunctionCallback {
+        use deno_core::v8::MapFnTo;
+        Self::v8_func.map_fn_to()
+    }
+    pub fn decl<'scope>() -> deno_core::OpDecl {
+        deno_core::OpDecl {
+            name: Self::name(),
+            v8_fn_ptr: Self::v8_fn_ptr(),
+            enabled: true,
+            fast_fn: None,
+            is_async: true,
+            is_unstable: false,
+            is_v8: false,
+            argc: 1usize,
+        }
+    }
+    #[inline]
+    #[allow(clippy::too_many_arguments)]
+    async fn call(state: &mut OpState, v: i32) -> Result<(), anyhow::Error> {
+        Ok(())
+    }
+    pub fn v8_func<'scope>(
+        scope: &mut deno_core::v8::HandleScope<'scope>,
+        args: deno_core::v8::FunctionCallbackArguments,
+        mut rv: deno_core::v8::ReturnValue,
+    ) {
+        use deno_core::futures::FutureExt;
+        let ctx = unsafe {
+            &*(deno_core::v8::Local::<deno_core::v8::External>::cast(args.data()).value()
+                as *const deno_core::_ops::OpCtx)
+        };
+        let op_id = ctx.id;
+        let promise_id = args.get(0);
+        let promise_id = deno_core::v8::Local::<
+            deno_core::v8::Integer,
+        >::try_from(promise_id)
+            .map(|l| l.value() as deno_core::PromiseId)
+            .map_err(deno_core::anyhow::Error::from);
+        let promise_id: deno_core::PromiseId = match promise_id {
+            Ok(promise_id) => promise_id,
+            Err(err) => {
+                deno_core::_ops::throw_type_error(
+                    scope,
+                    format!("invalid promise id: {}", err),
+                );
+                return;
+            }
+        };
+        let arg_0 = args.get(1usize as i32);
+        let arg_0 = match deno_core::serde_v8::from_v8(scope, arg_0) {
+            Ok(v) => v,
+            Err(err) => {
+                let msg = format!(
+                    "Error parsing args at position {}: {}", 1usize,
+                    deno_core::anyhow::Error::from(err)
+                );
+                return deno_core::_ops::throw_type_error(scope, msg);
+            }
+        };
+        let get_class = {
+            let state = ::std::cell::RefCell::borrow(&ctx.state);
+            state.tracker.track_async(op_id);
+            state.get_error_class_fn
+        };
+        deno_core::_ops::queue_async_op(
+            ctx,
+            scope,
+            false,
+            async move {
+                let result = Self::call(
+                        compile_error!("mutable opstate is not supported in async ops"),
+                        arg_0,
+                    )
+                    .await;
+                (promise_id, op_id, deno_core::_ops::to_op_result(get_class, result))
+            },
+        );
+    }
+}
diff --git a/ops/optimizer_tests/issue16934_fast.rs b/ops/optimizer_tests/issue16934_fast.rs
@@ -0,0 +1,8 @@
+async fn send_stdin(state: &mut OpState, v: i32) -> Result<(), anyhow::Error> {
+  // @test-attr:fast
+  //
+  // https://github.com/denoland/deno/issues/16934
+  //
+  // OpState borrowed across await point is not allowed, as it will likely panic at runtime.
+  Ok(())
+}