Remove core/plugin.rs (#4824)

This simplifies the plugin interface in order to deliver op crates with a similar API

cli/js/deno.ts

@@ -93,7 +93,7 @@ export {
   PermissionStatus,
   Permissions,
 } from "./permissions.ts";
-export { openPlugin } from "./plugins.ts";
+export { openPlugin } from "./ops/plugins.ts";
 export { kill } from "./ops/process.ts";
 export { run, RunOptions, Process, ProcessStatus } from "./process.ts";
 export { DirEntry, readdirSync, readdir } from "./ops/fs/read_dir.ts";

cli/js/lib.deno.ns.d.ts

@@ -1796,35 +1796,23 @@ declare namespace Deno {
    * Requires `allow-write` permission. */
   export function truncate(name: string, len?: number): Promise<void>;
 
-  export interface AsyncHandler {
-    (msg: Uint8Array): void;
-  }
-
-  export interface PluginOp {
-    dispatch(
-      control: Uint8Array,
-      zeroCopy?: ArrayBufferView | null
-    ): Uint8Array | null;
-    setAsyncHandler(handler: AsyncHandler): void;
-  }
-
-  export interface Plugin {
-    ops: {
-      [name: string]: PluginOp;
-    };
-  }
-
   /** **UNSTABLE**: new API, yet to be vetted.
    *
    * Open and initalize a plugin.
    *
-   *        const plugin = Deno.openPlugin("./path/to/some/plugin.so");
-   *        const some_op = plugin.ops.some_op;
-   *        const response = some_op.dispatch(new Uint8Array([1,2,3,4]));
+   *        const rid = Deno.openPlugin("./path/to/some/plugin.so");
+   *        const opId = Deno.core.ops()["some_op"];
+   *        const response = Deno.core.dispatch(opId, new Uint8Array([1,2,3,4]));
    *        console.log(`Response from plugin ${response}`);
    *
-   * Requires `allow-plugin` permission. */
-  export function openPlugin(filename: string): Plugin;
+   * Requires `allow-plugin` permission.
+   *
+   * The plugin system is not stable and will change in the future, hence the
+   * lack of docs. For now take a look at the example
+   * https://github.com/denoland/deno/tree/master/test_plugin
+   */
+  export function openPlugin(filename: string): number;
+
   export interface NetAddr {
     transport: "tcp" | "udp";
     hostname: string;

cli/js/ops/plugins.ts

@@ -1,12 +1,6 @@
 import { sendSync } from "./dispatch_json.ts";
 
-interface OpenPluginResponse {
-  rid: number;
-  ops: {
-    [name: string]: number;
-  };
-}
-
-export function openPlugin(filename: string): OpenPluginResponse {
-  return sendSync("op_open_plugin", { filename });
+export function openPlugin(filename: string): number {
+  const rid = sendSync("op_open_plugin", { filename });
+  return rid;
 }

cli/js/plugins.ts

@@ -1,64 +0,0 @@
-import { openPlugin as openPluginOp } from "./ops/plugins.ts";
-import { core } from "./core.ts";
-import { close } from "./ops/resources.ts";
-
-export interface AsyncHandler {
-  (msg: Uint8Array): void;
-}
-
-interface PluginOp {
-  dispatch(
-    control: Uint8Array,
-    zeroCopy?: ArrayBufferView | null
-  ): Uint8Array | null;
-  setAsyncHandler(handler: AsyncHandler): void;
-}
-
-class PluginOpImpl implements PluginOp {
-  readonly #opId: number;
-
-  constructor(opId: number) {
-    this.#opId = opId;
-  }
-
-  dispatch(
-    control: Uint8Array,
-    zeroCopy?: ArrayBufferView | null
-  ): Uint8Array | null {
-    return core.dispatch(this.#opId, control, zeroCopy);
-  }
-
-  setAsyncHandler(handler: AsyncHandler): void {
-    core.setAsyncHandler(this.#opId, handler);
-  }
-}
-
-interface Plugin {
-  ops: {
-    [name: string]: PluginOp;
-  };
-  close(): void;
-}
-
-class PluginImpl implements Plugin {
-  #ops: { [name: string]: PluginOp } = {};
-
-  constructor(readonly rid: number, ops: { [name: string]: number }) {
-    for (const op in ops) {
-      this.#ops[op] = new PluginOpImpl(ops[op]);
-    }
-  }
-
-  get ops(): { [name: string]: PluginOp } {
-    return Object.assign({}, this.#ops);
-  }
-
-  close(): void {
-    close(this.rid);
-  }
-}
-
-export function openPlugin(filename: string): Plugin {
-  const response = openPluginOp(filename);
-  return new PluginImpl(response.rid, response.ops);
-}

cli/ops/plugins.rs

@@ -4,16 +4,13 @@ use crate::op_error::OpError;
 use crate::ops::json_op;
 use crate::state::State;
 use deno_core::Isolate;
-use deno_core::OpDispatcher;
-use deno_core::OpId;
-use deno_core::PluginInitContext;
-use deno_core::PluginInitFn;
 use deno_core::ZeroCopyBuf;
 use dlopen::symbor::Library;
-use std::collections::HashMap;
 use std::ffi::OsStr;
 use std::path::Path;
 
+pub type PluginInitFn = fn(isolate: &mut deno_core::Isolate);
+
 pub fn init(i: &mut Isolate, s: &State) {
   i.register_op(
     "op_open_plugin",
@@ -23,27 +20,11 @@ pub fn init(i: &mut Isolate, s: &State) {
 
 fn open_plugin<P: AsRef<OsStr>>(lib_path: P) -> Result<Library, OpError> {
   debug!("Loading Plugin: {:#?}", lib_path.as_ref());
-
   Library::open(lib_path).map_err(OpError::from)
 }
 
 struct PluginResource {
   lib: Library,
-  ops: HashMap<String, OpId>,
-}
-
-struct InitContext {
-  ops: HashMap<String, Box<OpDispatcher>>,
-}
-
-impl PluginInitContext for InitContext {
-  fn register_op(&mut self, name: &str, op: Box<OpDispatcher>) {
-    let existing = self.ops.insert(name.to_string(), op);
-    assert!(
-      existing.is_none(),
-      format!("Op already registered: {}", name)
-    );
-  }
 }
 
 #[derive(Deserialize)]
@@ -58,16 +39,13 @@ pub fn op_open_plugin(
   args: Value,
   _zero_copy: Option<ZeroCopyBuf>,
 ) -> Result<JsonOp, OpError> {
-  let args: OpenPluginArgs = serde_json::from_value(args)?;
+  let args: OpenPluginArgs = serde_json::from_value(args).unwrap();
   let filename = deno_fs::resolve_from_cwd(Path::new(&args.filename))?;
 
   state.check_plugin(&filename)?;
 
-  let lib = open_plugin(filename)?;
-  let plugin_resource = PluginResource {
-    lib,
-    ops: HashMap::new(),
-  };
+  let lib = open_plugin(filename).unwrap();
+  let plugin_resource = PluginResource { lib };
   let mut state_ = state.borrow_mut();
   let rid = state_
     .resource_table
@@ -77,27 +55,13 @@ pub fn op_open_plugin(
     .get_mut::<PluginResource>(rid)
     .unwrap();
 
-  let init_fn = *unsafe {
+  let deno_plugin_init = *unsafe {
     plugin_resource
       .lib
       .symbol::<PluginInitFn>("deno_plugin_init")
-  }?;
-  let mut init_context = InitContext {
-    ops: HashMap::new(),
-  };
-  init_fn(&mut init_context);
-  for op in init_context.ops {
-    // Register each plugin op in the `OpRegistry` with the name
-    // formated like this `plugin_{plugin_rid}_{name}`.
-    // The inclusion of prefix and rid is designed to avoid any
-    // op name collision beyond the bound of a single loaded
-    // plugin instance.
-    let op_id = isolate
-      .register_op(&format!("plugin_{}_{}", rid, op.0), state.core_op(op.1));
-    plugin_resource.ops.insert(op.0, op_id);
   }
+  .unwrap();
+  deno_plugin_init(isolate);
 
-  Ok(JsonOp::Sync(
-    json!({ "rid": rid, "ops": plugin_resource.ops }),
-  ))
+  Ok(JsonOp::Sync(json!(rid)))
 }

cli/ops/resources.rs

@@ -29,7 +29,7 @@ fn op_close(
   struct CloseArgs {
     rid: i32,
   }
-  let args: CloseArgs = serde_json::from_value(args)?;
+  let args: CloseArgs = serde_json::from_value(args).unwrap();
   let mut state = state.borrow_mut();
   state
     .resource_table

core/lib.rs

@@ -17,7 +17,6 @@ mod js_errors;
 mod module_specifier;
 mod modules;
 mod ops;
-mod plugins;
 mod resources;
 mod shared_queue;
 
@@ -31,7 +30,6 @@ pub use crate::js_errors::*;
 pub use crate::module_specifier::*;
 pub use crate::modules::*;
 pub use crate::ops::*;
-pub use crate::plugins::*;
 pub use crate::resources::*;
 
 pub fn v8_version() -> &'static str {

core/plugins.rs

@@ -1,24 +0,0 @@
-// TODO(ry) This plugin module is superfluous. Try to remove definitions for
-// "init_fn!", "PluginInitFn", and "PluginInitContext".
-
-use crate::ops::OpDispatcher;
-
-pub type PluginInitFn = fn(context: &mut dyn PluginInitContext);
-
-pub trait PluginInitContext {
-  fn register_op(
-    &mut self,
-    name: &str,
-    op: Box<OpDispatcher>, // TODO(ry) rename to dispatcher, not op.
-  );
-}
-
-#[macro_export]
-macro_rules! init_fn {
-  ($fn:path) => {
-    #[no_mangle]
-    pub fn deno_plugin_init(context: &mut dyn PluginInitContext) {
-      $fn(context)
-    }
-  };
-}

test_plugin/src/lib.rs

@@ -1,17 +1,16 @@
-#[macro_use]
 extern crate deno_core;
 extern crate futures;
 
+use deno_core::Buf;
 use deno_core::Op;
-use deno_core::PluginInitContext;
-use deno_core::{Buf, ZeroCopyBuf};
+use deno_core::ZeroCopyBuf;
 use futures::future::FutureExt;
 
-fn init(context: &mut dyn PluginInitContext) {
-  context.register_op("testSync", Box::new(op_test_sync));
-  context.register_op("testAsync", Box::new(op_test_async));
+#[no_mangle]
+pub fn deno_plugin_init(isolate: &mut deno_core::Isolate) {
+  isolate.register_op("testSync", op_test_sync);
+  isolate.register_op("testAsync", op_test_async);
 }
-init_fn!(init);
 
 pub fn op_test_sync(
   _isolate: &mut deno_core::Isolate,

test_plugin/tests/integration_tests.rs

@@ -1,3 +1,7 @@
+// To run this test manually:
+//   cd test_plugin
+//   ../target/debug/deno --allow-plugin tests/test.js debug
+
 // TODO(ry) Re-enable this test on windows. It is flaky for an unknown reason.
 #![cfg(not(windows))]
 
@@ -38,11 +42,7 @@ fn basic() {
     println!("stderr {}", stderr);
   }
   assert!(output.status.success());
-  let expected = if cfg!(target_os = "windows") {
-    "Hello from plugin. data: test | zero_copy: test\nPlugin Sync Response: test\r\nHello from plugin. data: test | zero_copy: test\nPlugin Async Response: test\r\n"
-  } else {
-    "Hello from plugin. data: test | zero_copy: test\nPlugin Sync Response: test\nHello from plugin. data: test | zero_copy: test\nPlugin Async Response: test\n"
-  };
+  let expected = "Hello from plugin. data: test | zero_copy: test\nPlugin Sync Response: test\nHello from plugin. data: test | zero_copy: test\nPlugin Async Response: test\n";
   assert_eq!(stdout, expected);
   assert_eq!(stderr, "");
 }

test_plugin/tests/test.js

@@ -17,14 +17,21 @@ const filename = `../target/${Deno.args[0]}/${filenamePrefix}${filenameBase}${fi
 // in runTestClose() below.
 const resourcesPre = Deno.resources();
 
-const plugin = Deno.openPlugin(filename);
+const rid = Deno.openPlugin(filename);
 
-const { testSync, testAsync } = plugin.ops;
+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 textDecoder = new TextDecoder();
 
 function runTestSync() {
-  const response = testSync.dispatch(
+  const response = Deno.core.dispatch(
+    testSync,
     new Uint8Array([116, 101, 115, 116]),
     new Uint8Array([116, 101, 115, 116])
   );
@@ -32,12 +39,13 @@ function runTestSync() {
   console.log(`Plugin Sync Response: ${textDecoder.decode(response)}`);
 }
 
-testAsync.setAsyncHandler((response) => {
+Deno.core.setAsyncHandler(testAsync, (response) => {
   console.log(`Plugin Async Response: ${textDecoder.decode(response)}`);
 });
 
 function runTestAsync() {
-  const response = testAsync.dispatch(
+  const response = Deno.core.dispatch(
+    testAsync,
     new Uint8Array([116, 101, 115, 116]),
     new Uint8Array([116, 101, 115, 116])
   );
@@ -50,22 +58,22 @@ function runTestAsync() {
 function runTestOpCount() {
   const start = Deno.metrics();
 
-  testSync.dispatch(new Uint8Array([116, 101, 115, 116]));
+  Deno.core.dispatch(testSync, new Uint8Array([116, 101, 115, 116]));
 
   const end = Deno.metrics();
 
-  if (end.opsCompleted - start.opsCompleted !== 2) {
+  if (end.opsCompleted - start.opsCompleted !== 1) {
     // one op for the plugin and one for Deno.metrics
     throw new Error("The opsCompleted metric is not correct!");
   }
-  if (end.opsDispatched - start.opsDispatched !== 2) {
+  if (end.opsDispatched - start.opsDispatched !== 1) {
     // one op for the plugin and one for Deno.metrics
     throw new Error("The opsDispatched metric is not correct!");
   }
 }
 
 function runTestPluginClose() {
-  plugin.close();
+  Deno.close(rid);
 
   const resourcesPost = Deno.resources();