From 4ed1428c3401c9e6dc4d737bd7c9a50840054696 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elias=20Sj=C3=B6green?= Date: Fri, 7 May 2021 15:45:07 +0200 Subject: [PATCH] fix: align plugin api with Extension (#10427) --- Cargo.lock | 2 + core/extensions.rs | 8 +- core/lib.rs | 1 - core/plugin_api.rs | 22 ---- runtime/js/40_plugins.js | 4 +- runtime/ops/plugin.rs | 135 +++++++------------------ test_plugin/src/lib.rs | 128 ++++++++++++++++------- test_plugin/tests/integration_tests.rs | 8 +- test_plugin/tests/test.js | 105 ++++++++++++------- 9 files changed, 204 insertions(+), 209 deletions(-) delete mode 100644 core/plugin_api.rs diff --git a/Cargo.lock b/Cargo.lock index a911638f1d..c62cbbfd87 100644 --- a/Cargo.lock +++ b/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" diff --git a/core/extensions.rs b/core/extensions.rs index 9ad2b36974..e38d11fc98 100644 --- a/core/extensions.rs +++ b/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 { + pub fn init_js(&self) -> Vec { 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> { + pub fn init_ops(&mut self) -> Option> { // 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> { + pub fn init_middleware(&mut self) -> Option> { self.middleware_fn.take() } } diff --git a/core/lib.rs b/core/lib.rs index 4e7345c402..1f75b0ee30 100644 --- a/core/lib.rs +++ b/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; diff --git a/core/plugin_api.rs b/core/plugin_api.rs deleted file mode 100644 index 9f37df6f3c..0000000000 --- a/core/plugin_api.rs +++ /dev/null @@ -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) -> Op; - -pub trait Interface { - fn register_op(&mut self, name: &str, dispatcher: DispatchOpFn) -> OpId; -} diff --git a/runtime/js/40_plugins.js b/runtime/js/40_plugins.js index e9a3142b43..0796fd5cee 100644 --- a/runtime/js/40_plugins.js +++ b/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 = { diff --git a/runtime/ops/plugin.rs b/runtime/ops/plugin.rs index 17d39405fe..a4ec0eece7 100644 --- a/runtime/ops/plugin.rs +++ b/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::(rid) - .unwrap() - .lib - .symbol::("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::("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, -} +struct PluginResource(Rc); impl Resource for PluginResource { fn name(&self) -> Cow { "plugin".into() } + + fn close(self: Rc) { + unimplemented!(); + } } impl PluginResource { fn new(lib: &Rc) -> Self { - Self { lib: lib.clone() } - } -} - -struct PluginInterface<'a> { - state: &'a mut OpState, - plugin_lib: &'a Rc, -} - -impl<'a> PluginInterface<'a> { - fn new(state: &'a mut OpState, plugin_lib: &'a Rc) -> 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 = 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) = 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, - _plugin_lib: Rc, -} - -impl PluginOpAsyncFuture { - fn new(plugin_lib: &Rc, fut: OpAsyncFuture) -> Pin> { - let wrapped_fut = Self { - fut: Some(fut), - _plugin_lib: plugin_lib.clone(), - }; - Box::pin(wrapped_fut) - } -} - -impl Future for PluginOpAsyncFuture { - type Output = ::Output; - fn poll(mut self: Pin<&mut Self>, ctx: &mut Context) -> Poll { - self.fut.as_mut().unwrap().poll_unpin(ctx) - } -} - -impl Drop for PluginOpAsyncFuture { - fn drop(&mut self) { - self.fut.take(); + Self(lib.clone()) } } diff --git a/test_plugin/src/lib.rs b/test_plugin/src/lib.rs index 71ba698fcb..105071751b 100644 --- a/test_plugin/src/lib.rs +++ b/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, -) -> Op { - if zero_copy.is_some() { - println!("Hello from plugin."); - } +) -> Result { + 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>, + _args: (), zero_copy: Option, -) -> 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::>(); - 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 { + 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::>(); + 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 { + "TestResource".into() + } +} + +#[allow(clippy::unnecessary_wraps)] +fn op_test_resource_table_add( + state: &mut OpState, + text: String, + _zero_copy: Option, +) -> Result { + 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, +) -> Result { + println!("Hello from resource_table.get plugin op."); + + Ok( + state + .resource_table + .get::(rid) + .ok_or_else(bad_resource_id)? + .0 + .clone(), + ) } diff --git a/test_plugin/tests/integration_tests.rs b/test_plugin/tests/integration_tests.rs index b0e1c6a74a..203a8baded 100644 --- a/test_plugin/tests/integration_tests.rs +++ b/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, ""); } diff --git a/test_plugin/tests/test.js b/test_plugin/tests/test.js index 90e6b43cad..716b2ff9ac 100644 --- a/test_plugin/tests/test.js +++ b/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();