fix: align plugin api with Extension (#10427)

Cargo.lock

@@ -1,5 +1,7 @@
 # This file is automatically @generated by Cargo.
 # It is not intended for manual editing.
+version = 3
+
 [[package]]
 name = "Inflector"
 version = "0.11.4"

core/extensions.rs

@@ -24,7 +24,7 @@ impl Extension {
 
   /// returns JS source code to be loaded into the isolate (either at snapshotting,
   /// or at startup).  as a vector of a tuple of the file name, and the source code.
-  pub(crate) fn init_js(&self) -> Vec<SourcePair> {
+  pub fn init_js(&self) -> Vec<SourcePair> {
     match &self.js_files {
       Some(files) => files.clone(),
       None => vec![],
@@ -32,7 +32,7 @@ impl Extension {
   }
 
   /// Called at JsRuntime startup to initialize ops in the isolate.
-  pub(crate) fn init_ops(&mut self) -> Option<Vec<OpPair>> {
+  pub fn init_ops(&mut self) -> Option<Vec<OpPair>> {
     // TODO(@AaronO): maybe make op registration idempotent
     if self.initialized {
       panic!("init_ops called twice: not idempotent or correct");
@@ -43,7 +43,7 @@ impl Extension {
   }
 
   /// Allows setting up the initial op-state of an isolate at startup.
-  pub(crate) fn init_state(&self, state: &mut OpState) -> Result<(), AnyError> {
+  pub fn init_state(&self, state: &mut OpState) -> Result<(), AnyError> {
     match &self.opstate_fn {
       Some(ofn) => ofn(state),
       None => Ok(()),
@@ -51,7 +51,7 @@ impl Extension {
   }
 
   /// init_middleware lets us middleware op registrations, it's called before init_ops
-  pub(crate) fn init_middleware(&mut self) -> Option<Box<OpMiddlewareFn>> {
+  pub fn init_middleware(&mut self) -> Option<Box<OpMiddlewareFn>> {
     self.middleware_fn.take()
   }
 }

core/lib.rs

@@ -12,7 +12,6 @@ mod normalize_path;
 mod ops;
 mod ops_builtin;
 mod ops_json;
-pub mod plugin_api;
 mod resources;
 mod runtime;
 

core/plugin_api.rs

@@ -1,22 +0,0 @@
-// Copyright 2018-2021 the Deno authors. All rights reserved. MIT license.
-
-// This file defines the public interface for dynamically loaded plugins.
-
-// The plugin needs to do all interaction with the CLI crate through trait
-// objects and function pointers. This ensures that no concrete internal methods
-// (such as register_op and the closures created by it) can end up in the plugin
-// shared library itself, which would cause segfaults when the plugin is
-// unloaded and all functions in the plugin library are unmapped from memory.
-
-pub use crate::Op;
-pub use crate::OpId;
-pub use crate::OpResult;
-pub use crate::ZeroCopyBuf;
-
-pub type InitFn = fn(&mut dyn Interface);
-
-pub type DispatchOpFn = fn(&mut dyn Interface, Option<ZeroCopyBuf>) -> Op;
-
-pub trait Interface {
-  fn register_op(&mut self, name: &str, dispatcher: DispatchOpFn) -> OpId;
-}

runtime/js/40_plugins.js

@@ -5,7 +5,9 @@
   const core = window.Deno.core;
 
   function openPlugin(filename) {
-    return core.opSync("op_open_plugin", filename);
+    const rid = core.opSync("op_open_plugin", filename);
+    core.syncOpsCache();
+    return rid;
   }
 
   window.__bootstrap.plugins = {

runtime/ops/plugin.rs

@@ -1,15 +1,8 @@
 // Copyright 2018-2021 the Deno authors. All rights reserved. MIT license.
-use crate::metrics::metrics_op;
 use crate::permissions::Permissions;
 use deno_core::error::AnyError;
-use deno_core::futures::prelude::*;
 use deno_core::op_sync;
-use deno_core::plugin_api;
 use deno_core::Extension;
-use deno_core::Op;
-use deno_core::OpAsyncFuture;
-use deno_core::OpFn;
-use deno_core::OpId;
 use deno_core::OpState;
 use deno_core::Resource;
 use deno_core::ResourceId;
@@ -17,11 +10,18 @@ use deno_core::ZeroCopyBuf;
 use dlopen::symbor::Library;
 use log::debug;
 use std::borrow::Cow;
+use std::mem;
 use std::path::PathBuf;
-use std::pin::Pin;
 use std::rc::Rc;
-use std::task::Context;
-use std::task::Poll;
+
+/// A default `init` function for plugins which mimics the way the internal
+/// extensions are initalized. Plugins currently do not support all extension
+/// features and are most likely not going to in the future. Currently only
+/// `init_state` and `init_ops` are supported while `init_middleware` and `init_js`
+/// are not. Currently the `PluginResource` does not support being closed due to
+/// certain risks in unloading the dynamic library without unloading dependent
+/// functions and resources.
+pub type InitFn = fn() -> Extension;
 
 pub fn init() -> Extension {
   Extension::builder()
@@ -44,111 +44,44 @@ pub fn op_open_plugin(
   let plugin_lib = Library::open(filename).map(Rc::new)?;
   let plugin_resource = PluginResource::new(&plugin_lib);
 
-  let rid;
-  let deno_plugin_init;
-  {
-    rid = state.resource_table.add(plugin_resource);
-    deno_plugin_init = *unsafe {
-      state
-        .resource_table
-        .get::<PluginResource>(rid)
-        .unwrap()
-        .lib
-        .symbol::<plugin_api::InitFn>("deno_plugin_init")
-        .unwrap()
-    };
+  // Forgets the plugin_lib value to prevent segfaults when the process exits
+  mem::forget(plugin_lib);
+
+  let init = *unsafe { plugin_resource.0.symbol::<InitFn>("init") }?;
+  let rid = state.resource_table.add(plugin_resource);
+  let mut extension = init();
+
+  if !extension.init_js().is_empty() {
+    panic!("Plugins do not support loading js");
   }
 
-  let mut interface = PluginInterface::new(state, &plugin_lib);
-  deno_plugin_init(&mut interface);
+  if extension.init_middleware().is_some() {
+    panic!("Plugins do not support middleware");
+  }
+
+  extension.init_state(state)?;
+  let ops = extension.init_ops().unwrap_or_default();
+  for (name, opfn) in ops {
+    state.op_table.register_op(name, opfn);
+  }
 
   Ok(rid)
 }
 
-struct PluginResource {
-  lib: Rc<Library>,
-}
+struct PluginResource(Rc<Library>);
 
 impl Resource for PluginResource {
   fn name(&self) -> Cow<str> {
     "plugin".into()
   }
+
+  fn close(self: Rc<Self>) {
+    unimplemented!();
+  }
 }
 
 impl PluginResource {
   fn new(lib: &Rc<Library>) -> Self {
-    Self { lib: lib.clone() }
-  }
-}
-
-struct PluginInterface<'a> {
-  state: &'a mut OpState,
-  plugin_lib: &'a Rc<Library>,
-}
-
-impl<'a> PluginInterface<'a> {
-  fn new(state: &'a mut OpState, plugin_lib: &'a Rc<Library>) -> Self {
-    Self { state, plugin_lib }
-  }
-}
-
-impl<'a> plugin_api::Interface for PluginInterface<'a> {
-  /// Does the same as `core::Isolate::register_op()`, but additionally makes
-  /// the registered op dispatcher, as well as the op futures created by it,
-  /// keep reference to the plugin `Library` object, so that the plugin doesn't
-  /// get unloaded before all its op registrations and the futures created by
-  /// them are dropped.
-  fn register_op(
-    &mut self,
-    name: &str,
-    dispatch_op_fn: plugin_api::DispatchOpFn,
-  ) -> OpId {
-    let plugin_lib = self.plugin_lib.clone();
-    let plugin_op_fn: Box<OpFn> = Box::new(move |state_rc, payload| {
-      let mut state = state_rc.borrow_mut();
-      let mut interface = PluginInterface::new(&mut state, &plugin_lib);
-      let (_, buf): ((), Option<ZeroCopyBuf>) = payload.deserialize().unwrap();
-      let op = dispatch_op_fn(&mut interface, buf);
-      match op {
-        sync_op @ Op::Sync(..) => sync_op,
-        Op::Async(fut) => Op::Async(PluginOpAsyncFuture::new(&plugin_lib, fut)),
-        Op::AsyncUnref(fut) => {
-          Op::AsyncUnref(PluginOpAsyncFuture::new(&plugin_lib, fut))
-        }
-        _ => unreachable!(),
-      }
-    });
-    self.state.op_table.register_op(
-      name,
-      metrics_op(Box::leak(Box::new(name.to_string())), plugin_op_fn),
-    )
-  }
-}
-
-struct PluginOpAsyncFuture {
-  fut: Option<OpAsyncFuture>,
-  _plugin_lib: Rc<Library>,
-}
-
-impl PluginOpAsyncFuture {
-  fn new(plugin_lib: &Rc<Library>, fut: OpAsyncFuture) -> Pin<Box<Self>> {
-    let wrapped_fut = Self {
-      fut: Some(fut),
-      _plugin_lib: plugin_lib.clone(),
-    };
-    Box::pin(wrapped_fut)
-  }
-}
-
-impl Future for PluginOpAsyncFuture {
-  type Output = <OpAsyncFuture as Future>::Output;
-  fn poll(mut self: Pin<&mut Self>, ctx: &mut Context) -> Poll<Self::Output> {
-    self.fut.as_mut().unwrap().poll_unpin(ctx)
-  }
-}
-
-impl Drop for PluginOpAsyncFuture {
-  fn drop(&mut self) {
-    self.fut.take();
+    Self(lib.clone())
   }
 }

test_plugin/src/lib.rs

@@ -1,55 +1,105 @@
 // Copyright 2018-2021 the Deno authors. All rights reserved. MIT license.
 
-use deno_core::plugin_api::Interface;
-use deno_core::plugin_api::Op;
-use deno_core::plugin_api::OpResult;
-use deno_core::plugin_api::ZeroCopyBuf;
-use futures::future::FutureExt;
+use std::borrow::Cow;
+use std::cell::RefCell;
+use std::rc::Rc;
+
+use deno_core::error::bad_resource_id;
+use deno_core::error::AnyError;
+use deno_core::op_async;
+use deno_core::op_sync;
+use deno_core::Extension;
+use deno_core::OpState;
+use deno_core::Resource;
+use deno_core::ResourceId;
+use deno_core::ZeroCopyBuf;
 
 #[no_mangle]
-pub fn deno_plugin_init(interface: &mut dyn Interface) {
-  interface.register_op("testSync", op_test_sync);
-  interface.register_op("testAsync", op_test_async);
+pub fn init() -> Extension {
+  Extension::builder()
+    .ops(vec![
+      ("op_test_sync", op_sync(op_test_sync)),
+      ("op_test_async", op_async(op_test_async)),
+      (
+        "op_test_resource_table_add",
+        op_sync(op_test_resource_table_add),
+      ),
+      (
+        "op_test_resource_table_get",
+        op_sync(op_test_resource_table_get),
+      ),
+    ])
+    .build()
 }
 
 fn op_test_sync(
-  _interface: &mut dyn Interface,
+  _state: &mut OpState,
+  _args: (),
   zero_copy: Option<ZeroCopyBuf>,
-) -> Op {
-  if zero_copy.is_some() {
-    println!("Hello from plugin.");
-  }
+) -> Result<String, AnyError> {
+  println!("Hello from sync plugin op.");
+
   if let Some(buf) = zero_copy {
-    let buf_str = std::str::from_utf8(&buf[..]).unwrap();
+    let buf_str = std::str::from_utf8(&buf[..])?;
     println!("zero_copy: {}", buf_str);
   }
-  let result = b"test";
-  let result_box: Box<[u8]> = Box::new(*result);
-  Op::Sync(OpResult::Ok(result_box.into()))
+
+  Ok("test".to_string())
 }
 
-fn op_test_async(
-  _interface: &mut dyn Interface,
+async fn op_test_async(
+  _state: Rc<RefCell<OpState>>,
+  _args: (),
   zero_copy: Option<ZeroCopyBuf>,
-) -> Op {
-  if zero_copy.is_some() {
-    println!("Hello from plugin.");
-  }
-  let fut = async move {
-    if let Some(buf) = zero_copy {
-      let buf_str = std::str::from_utf8(&buf[..]).unwrap();
-      println!("zero_copy: {}", buf_str);
-    }
-    let (tx, rx) = futures::channel::oneshot::channel::<Result<(), ()>>();
-    std::thread::spawn(move || {
-      std::thread::sleep(std::time::Duration::from_secs(1));
-      tx.send(Ok(())).unwrap();
-    });
-    assert!(rx.await.is_ok());
-    let result = b"test";
-    let result_box: Box<[u8]> = Box::new(*result);
-    (0, OpResult::Ok(result_box.into()))
-  };
+) -> Result<String, AnyError> {
+  println!("Hello from async plugin op.");
 
-  Op::Async(fut.boxed())
+  if let Some(buf) = zero_copy {
+    let buf_str = std::str::from_utf8(&buf[..])?;
+    println!("zero_copy: {}", buf_str);
+  }
+
+  let (tx, rx) = futures::channel::oneshot::channel::<Result<(), ()>>();
+  std::thread::spawn(move || {
+    std::thread::sleep(std::time::Duration::from_secs(1));
+    tx.send(Ok(())).unwrap();
+  });
+  assert!(rx.await.is_ok());
+
+  Ok("test".to_string())
+}
+
+struct TestResource(String);
+impl Resource for TestResource {
+  fn name(&self) -> Cow<str> {
+    "TestResource".into()
+  }
+}
+
+#[allow(clippy::unnecessary_wraps)]
+fn op_test_resource_table_add(
+  state: &mut OpState,
+  text: String,
+  _zero_copy: Option<ZeroCopyBuf>,
+) -> Result<u32, AnyError> {
+  println!("Hello from resource_table.add plugin op.");
+
+  Ok(state.resource_table.add(TestResource(text)))
+}
+
+fn op_test_resource_table_get(
+  state: &mut OpState,
+  rid: ResourceId,
+  _zero_copy: Option<ZeroCopyBuf>,
+) -> Result<String, AnyError> {
+  println!("Hello from resource_table.get plugin op.");
+
+  Ok(
+    state
+      .resource_table
+      .get::<TestResource>(rid)
+      .ok_or_else(bad_resource_id)?
+      .0
+      .clone(),
+  )
 }

test_plugin/tests/integration_tests.rs

@@ -10,11 +10,6 @@ const BUILD_VARIANT: &str = "debug";
 const BUILD_VARIANT: &str = "release";
 
 #[test]
-// TODO: re-enable after adapting plugins to new op-layer
-// see:
-// - https://github.com/denoland/deno/pull/9843
-// - https://github.com/denoland/deno/pull/9850
-#[ignore]
 fn basic() {
   let mut build_plugin_base = Command::new("cargo");
   let mut build_plugin =
@@ -38,8 +33,9 @@ fn basic() {
     println!("stdout {}", stdout);
     println!("stderr {}", stderr);
   }
+  println!("{:?}", output.status);
   assert!(output.status.success());
-  let expected = "Hello from plugin.\nzero_copy[0]: test\nzero_copy[1]: 123\nzero_copy[2]: cba\nPlugin Sync Response: test\nHello from plugin.\nzero_copy[0]: test\nzero_copy[1]: 123\nzero_copy[2]: cba\nPlugin Async Response: test\n";
+  let expected = "Plugin rid: 3\nHello from sync plugin op.\nzero_copy: test\nop_test_sync returned: test\nHello from async plugin op.\nzero_copy: 123\nop_test_async returned: test\nHello from resource_table.add plugin op.\nTestResource rid: 4\nHello from resource_table.get plugin op.\nTestResource get value: hello plugin!\nHello from sync plugin op.\nOps completed count is correct!\nOps dispatched count is correct!\n";
   assert_eq!(stdout, expected);
   assert_eq!(stderr, "");
 }

test_plugin/tests/test.js

@@ -1,4 +1,5 @@
 // Copyright 2018-2021 the Deno authors. All rights reserved. MIT license.
+// deno-lint-ignore-file
 
 const filenameBase = "test_plugin";
 
@@ -17,69 +18,101 @@ const filename = `../target/${
   Deno.args[0]
 }/${filenamePrefix}${filenameBase}${filenameSuffix}`;
 
-// This will be checked against open resources after Plugin.close()
-// in runTestClose() below.
 const resourcesPre = Deno.resources();
 
-const rid = Deno.openPlugin(filename);
+const pluginRid = Deno.openPlugin(filename);
+console.log(`Plugin rid: ${pluginRid}`);
 
-const { testSync, testAsync } = Deno.core.ops();
-if (!(testSync > 0)) {
-  throw "bad op id for testSync";
-}
-if (!(testAsync > 0)) {
-  throw "bad op id for testAsync";
-}
+const {
+  op_test_sync,
+  op_test_async,
+  op_test_resource_table_add,
+  op_test_resource_table_get,
+} = Deno.core.ops();
 
-const textDecoder = new TextDecoder();
+if (
+  op_test_sync === null ||
+  op_test_async === null ||
+  op_test_resource_table_add === null ||
+  op_test_resource_table_get === null
+) {
+  throw new Error("Not all expected ops were registered");
+}
 
 function runTestSync() {
-  const response = Deno.core.dispatch(
-    testSync,
+  const result = Deno.core.opSync(
+    "op_test_sync",
+    null,
     new Uint8Array([116, 101, 115, 116]),
-    new Uint8Array([49, 50, 51]),
-    new Uint8Array([99, 98, 97]),
   );
 
-  console.log(`Plugin Sync Response: ${textDecoder.decode(response)}`);
+  console.log(`op_test_sync returned: ${result}`);
+
+  if (result !== "test") {
+    throw new Error("op_test_sync returned an unexpected value!");
+  }
 }
 
-Deno.core.setAsyncHandler(testAsync, (response) => {
-  console.log(`Plugin Async Response: ${textDecoder.decode(response)}`);
-});
-
-function runTestAsync() {
-  const response = Deno.core.dispatch(
-    testAsync,
-    new Uint8Array([116, 101, 115, 116]),
+async function runTestAsync() {
+  const promise = Deno.core.opAsync(
+    "op_test_async",
+    null,
     new Uint8Array([49, 50, 51]),
-    new Uint8Array([99, 98, 97]),
   );
 
-  if (response != null || response != undefined) {
-    throw new Error("Expected null response!");
+  if (!(promise instanceof Promise)) {
+    throw new Error("Expected promise!");
   }
+
+  const result = await promise;
+  console.log(`op_test_async returned: ${result}`);
+
+  if (result !== "test") {
+    throw new Error("op_test_async promise resolved to an unexpected value!");
+  }
+}
+
+function runTestResourceTable() {
+  const expect = "hello plugin!";
+
+  const testRid = Deno.core.opSync("op_test_resource_table_add", expect);
+  console.log(`TestResource rid: ${testRid}`);
+
+  if (testRid === null || Deno.resources()[testRid] !== "TestResource") {
+    throw new Error("TestResource was not found!");
+  }
+
+  const testValue = Deno.core.opSync("op_test_resource_table_get", testRid);
+  console.log(`TestResource get value: ${testValue}`);
+
+  if (testValue !== expect) {
+    throw new Error("Did not get correct resource value!");
+  }
+
+  Deno.close(testRid);
 }
 
 function runTestOpCount() {
   const start = Deno.metrics();
 
-  Deno.core.dispatch(testSync);
+  Deno.core.opSync("op_test_sync");
 
   const end = Deno.metrics();
 
-  if (end.opsCompleted - start.opsCompleted !== 2) {
-    // one op for the plugin and one for Deno.metrics
+  if (end.opsCompleted - start.opsCompleted !== 1) {
     throw new Error("The opsCompleted metric is not correct!");
   }
-  if (end.opsDispatched - start.opsDispatched !== 2) {
-    // one op for the plugin and one for Deno.metrics
+  console.log("Ops completed count is correct!");
+
+  if (end.opsDispatched - start.opsDispatched !== 1) {
     throw new Error("The opsDispatched metric is not correct!");
   }
+  console.log("Ops dispatched count is correct!");
 }
 
 function runTestPluginClose() {
-  Deno.close(rid);
+  // Closing does not yet work
+  Deno.close(pluginRid);
 
   const resourcesPost = Deno.resources();
 
@@ -92,10 +125,12 @@ Before: ${preStr}
 After: ${postStr}`,
     );
   }
+  console.log("Correct number of resources");
 }
 
 runTestSync();
-runTestAsync();
+await runTestAsync();
+runTestResourceTable();
 
 runTestOpCount();
-runTestPluginClose();
+// runTestPluginClose();