From 212e60222d68824cfac1e5a9ef3dcdb0e5b52ef8 Mon Sep 17 00:00:00 2001 From: eliassjogreen Date: Thu, 29 Apr 2021 20:33:50 +0200 Subject: [PATCH 01/16] feat: align plugin api with Extension --- Cargo.lock | 2 + core/extensions.rs | 8 +- core/lib.rs | 1 - core/plugin_api.rs | 22 ----- runtime/ops/plugin.rs | 103 ++++------------------- test_plugin/src/lib.rs | 110 ++++++++++++++++--------- test_plugin/tests/integration_tests.rs | 7 +- test_plugin/tests/test.js | 109 ++++++++++++++++-------- 8 files changed, 169 insertions(+), 193 deletions(-) delete mode 100644 core/plugin_api.rs diff --git a/Cargo.lock b/Cargo.lock index 86a873675079b2..265c25d1353efc 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 134aab5d41c79b..f25e279f52bc38 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 37055bcc8d7095..fb1c918c9aead7 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; mod zero_copy_buf; diff --git a/core/plugin_api.rs b/core/plugin_api.rs deleted file mode 100644 index d1dda160f498eb..00000000000000 --- 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::OpResponse; -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/ops/plugin.rs b/runtime/ops/plugin.rs index c265c757f4701b..c597050750aae1 100644 --- a/runtime/ops/plugin.rs +++ b/runtime/ops/plugin.rs @@ -1,14 +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::plugin_api; +use deno_core::Extension; use deno_core::JsRuntime; -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,10 +11,9 @@ use dlopen::symbor::Library; use log::debug; use std::borrow::Cow; use std::path::PathBuf; -use std::pin::Pin; use std::rc::Rc; -use std::task::Context; -use std::task::Poll; + +pub type InitFn = fn() -> Extension; pub fn init(rt: &mut JsRuntime) { super::reg_sync(rt, "op_open_plugin", op_open_plugin); @@ -42,22 +35,27 @@ pub fn op_open_plugin( let plugin_resource = PluginResource::new(&plugin_lib); let rid; - let deno_plugin_init; + let init; { rid = state.resource_table.add(plugin_resource); - deno_plugin_init = *unsafe { + init = *unsafe { state .resource_table .get::(rid) .unwrap() .lib - .symbol::("deno_plugin_init") + .symbol::("init") .unwrap() }; } - let mut interface = PluginInterface::new(state, &plugin_lib); - deno_plugin_init(&mut interface); + let mut extension = init(); + + 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) } @@ -70,6 +68,10 @@ impl Resource for PluginResource { fn name(&self) -> Cow { "plugin".into() } + + fn close(self: Rc) { + unimplemented!(); + } } impl PluginResource { @@ -77,74 +79,3 @@ impl PluginResource { 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, buf| { - let mut state = state_rc.borrow_mut(); - let mut interface = PluginInterface::new(&mut state, &plugin_lib); - 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(); - } -} diff --git a/test_plugin/src/lib.rs b/test_plugin/src/lib.rs index 51ed6d499e4ac7..9a9feee78590ef 100644 --- a/test_plugin/src/lib.rs +++ b/test_plugin/src/lib.rs @@ -1,55 +1,89 @@ // 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::OpResponse; -use deno_core::plugin_api::ZeroCopyBuf; -use futures::future::FutureExt; +use std::rc::Rc; +use std::cell::RefCell; +use std::borrow::Cow; + +use deno_core::{ResourceId, 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::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(); println!("zero_copy: {}", buf_str); } - let result = b"test"; - let result_box: Box<[u8]> = Box::new(*result); - Op::Sync(OpResponse::Buffer(result_box)) + + 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."); +) -> Result { + println!("Hello from async plugin op."); + + if let Some(buf) = zero_copy { + let buf_str = std::str::from_utf8(&buf[..]).unwrap(); + println!("zero_copy: {}", buf_str); } - 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, OpResponse::Buffer(result_box)) - }; - - Op::Async(fut.boxed()) + + 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() + } +} + +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).unwrap().0.clone()) } diff --git a/test_plugin/tests/integration_tests.rs b/test_plugin/tests/integration_tests.rs index b0e1c6a74a506b..e7de1b2b7d3924 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 = @@ -39,7 +34,7 @@ fn basic() { println!("stderr {}", stderr); } 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\nSynced ops cache\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!\nOps dispatched count!\n"; assert_eq!(stdout, expected); assert_eq!(stderr, ""); } diff --git a/test_plugin/tests/test.js b/test_plugin/tests/test.js index 90e6b43cadfb7d..f51dc158cbbab2 100644 --- a/test_plugin/tests/test.js +++ b/test_plugin/tests/test.js @@ -13,72 +13,107 @@ if (Deno.build.os === "darwin") { filenameSuffix = ".dylib"; } -const filename = `../target/${ +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 { testSync, testAsync } = Deno.core.ops(); -if (!(testSync > 0)) { - throw "bad op id for testSync"; -} -if (!(testAsync > 0)) { - throw "bad op id for testAsync"; +const pluginRid = Deno.openPlugin(filename); +console.log(`Plugin rid: ${pluginRid}`); + +Deno.core.syncOpsCache(); +console.log("Synced ops cache"); + +const { + op_test_sync, + op_test_async, + op_test_resource_table_add, + op_test_resource_table_get, +} = Deno.core.ops(); + +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"); } -const textDecoder = new TextDecoder(); - 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}`); -Deno.core.setAsyncHandler(testAsync, (response) => { - console.log(`Plugin Async Response: ${textDecoder.decode(response)}`); -}); + if (result !== "test") { + throw new Error("op_test_sync returned an unexpected value!"); + } +} -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!"); + + if (end.opsDispatched - start.opsDispatched !== 1) { throw new Error("The opsDispatched metric is not correct!"); } + console.log("Ops dispatched count!"); } function runTestPluginClose() { + // Closing does not yet work, should it even? Deno.close(rid); const resourcesPost = Deno.resources(); @@ -92,10 +127,12 @@ Before: ${preStr} After: ${postStr}`, ); } + console.log("Correct number of resources"); } runTestSync(); -runTestAsync(); +await runTestAsync(); +runTestResourceTable(); runTestOpCount(); -runTestPluginClose(); +// runTestPluginClose(); From f755398f542cf6a8df6a9fa0fe9eb978975e273e Mon Sep 17 00:00:00 2001 From: eliassjogreen Date: Thu, 29 Apr 2021 20:36:15 +0200 Subject: [PATCH 02/16] chore: fmt --- test_plugin/src/lib.rs | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/test_plugin/src/lib.rs b/test_plugin/src/lib.rs index 9a9feee78590ef..a56a7f1b6d319c 100644 --- a/test_plugin/src/lib.rs +++ b/test_plugin/src/lib.rs @@ -1,16 +1,16 @@ // Copyright 2018-2021 the Deno authors. All rights reserved. MIT license. -use std::rc::Rc; -use std::cell::RefCell; use std::borrow::Cow; +use std::cell::RefCell; +use std::rc::Rc; -use deno_core::{ResourceId, 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::ZeroCopyBuf; +use deno_core::{error::AnyError, ResourceId}; #[no_mangle] pub fn init() -> Extension { @@ -18,8 +18,14 @@ pub fn init() -> Extension { .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)), + ( + "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() } @@ -85,5 +91,12 @@ fn op_test_resource_table_get( ) -> Result { println!("Hello from resource_table.get plugin op."); - Ok(state.resource_table.get::(rid).unwrap().0.clone()) + Ok( + state + .resource_table + .get::(rid) + .unwrap() + .0 + .clone(), + ) } From 679e2999a0977328b2c61854f1dc190e79a22e74 Mon Sep 17 00:00:00 2001 From: eliassjogreen Date: Thu, 29 Apr 2021 20:51:26 +0200 Subject: [PATCH 03/16] fix: lint --- test_plugin/tests/test.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test_plugin/tests/test.js b/test_plugin/tests/test.js index f51dc158cbbab2..be2bba41ef191e 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"; From 006a4c8f3be21628b2a0f42e777155112c211647 Mon Sep 17 00:00:00 2001 From: eliassjogreen Date: Mon, 3 May 2021 02:02:13 +0200 Subject: [PATCH 04/16] chore: merge --- runtime/ops/plugin.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/runtime/ops/plugin.rs b/runtime/ops/plugin.rs index c597050750aae1..a382e0fd8d0c43 100644 --- a/runtime/ops/plugin.rs +++ b/runtime/ops/plugin.rs @@ -1,6 +1,6 @@ // Copyright 2018-2021 the Deno authors. All rights reserved. MIT license. use crate::permissions::Permissions; -use deno_core::error::AnyError; +use deno_core::error::{AnyError, anyhow}; use deno_core::Extension; use deno_core::JsRuntime; use deno_core::OpState; @@ -51,6 +51,14 @@ pub fn op_open_plugin( let mut extension = init(); + if !extension.init_js().is_empty() { + return Err(anyhow!("Plugins do not support loading js")); + } + + if extension.init_middleware().is_some() { + return Err(anyhow!("Plugins do not support middleware")); + } + extension.init_state(state)?; let ops = extension.init_ops().unwrap_or_default(); for (name, opfn) in ops { From 53f3ae2da4d1b2b4c7aa6addf736bd46d60fea37 Mon Sep 17 00:00:00 2001 From: eliassjogreen Date: Mon, 3 May 2021 02:06:07 +0200 Subject: [PATCH 05/16] panic instead of returning an error --- runtime/ops/plugin.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/runtime/ops/plugin.rs b/runtime/ops/plugin.rs index a382e0fd8d0c43..33b598aae0a35c 100644 --- a/runtime/ops/plugin.rs +++ b/runtime/ops/plugin.rs @@ -1,6 +1,6 @@ // Copyright 2018-2021 the Deno authors. All rights reserved. MIT license. use crate::permissions::Permissions; -use deno_core::error::{AnyError, anyhow}; +use deno_core::error::{anyhow, AnyError}; use deno_core::Extension; use deno_core::JsRuntime; use deno_core::OpState; @@ -52,11 +52,11 @@ pub fn op_open_plugin( let mut extension = init(); if !extension.init_js().is_empty() { - return Err(anyhow!("Plugins do not support loading js")); + panic!("Plugins do not support loading js"); } if extension.init_middleware().is_some() { - return Err(anyhow!("Plugins do not support middleware")); + panic!("Plugins do not support middleware"); } extension.init_state(state)?; From a1b630f95a7311ef78ad29bc6703cc2ddb61bc8a Mon Sep 17 00:00:00 2001 From: eliassjogreen Date: Mon, 3 May 2021 14:21:32 +0200 Subject: [PATCH 06/16] fix --- runtime/ops/plugin.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/runtime/ops/plugin.rs b/runtime/ops/plugin.rs index 3555b119648684..5a0054df648320 100644 --- a/runtime/ops/plugin.rs +++ b/runtime/ops/plugin.rs @@ -1,8 +1,8 @@ // Copyright 2018-2021 the Deno authors. All rights reserved. MIT license. use crate::permissions::Permissions; -use deno_core::error::{anyhow, AnyError}; +use deno_core::error::AnyError; +use deno_core::op_sync; use deno_core::Extension; -use deno_core::JsRuntime; use deno_core::OpState; use deno_core::Resource; use deno_core::ResourceId; From b81a043af501609055cb0995c6d84680c22d993d Mon Sep 17 00:00:00 2001 From: eliassjogreen Date: Mon, 3 May 2021 16:13:32 +0200 Subject: [PATCH 07/16] fix lint errors --- test_plugin/src/lib.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/test_plugin/src/lib.rs b/test_plugin/src/lib.rs index a56a7f1b6d319c..af76bbb6873156 100644 --- a/test_plugin/src/lib.rs +++ b/test_plugin/src/lib.rs @@ -4,13 +4,15 @@ use std::borrow::Cow; use std::cell::RefCell; use std::rc::Rc; +use deno_core::error::anyhow; +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; -use deno_core::{error::AnyError, ResourceId}; #[no_mangle] pub fn init() -> Extension { @@ -38,7 +40,7 @@ fn op_test_sync( 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); } @@ -74,6 +76,7 @@ impl Resource for TestResource { } } +#[allow(clippy::unnecessary_wraps)] fn op_test_resource_table_add( state: &mut OpState, text: String, @@ -91,12 +94,9 @@ fn op_test_resource_table_get( ) -> Result { println!("Hello from resource_table.get plugin op."); - Ok( - state - .resource_table - .get::(rid) - .unwrap() - .0 - .clone(), - ) + if let Some(resource) = state.resource_table.get::(rid) { + Ok(resource.0.clone()) + } else { + Err(anyhow!("Could not find TestResource with id {}", rid)) + } } From 5722f99f51560ba853507839a5015545d76a6d2b Mon Sep 17 00:00:00 2001 From: eliassjogreen Date: Mon, 3 May 2021 16:14:29 +0200 Subject: [PATCH 08/16] another fix --- test_plugin/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test_plugin/src/lib.rs b/test_plugin/src/lib.rs index af76bbb6873156..1f84ec1f9b59e5 100644 --- a/test_plugin/src/lib.rs +++ b/test_plugin/src/lib.rs @@ -55,7 +55,7 @@ async fn op_test_async( println!("Hello from async 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); } From 8b47cfdac3af062df1b7c39a981307a551847c20 Mon Sep 17 00:00:00 2001 From: eliassjogreen Date: Mon, 3 May 2021 16:21:13 +0200 Subject: [PATCH 09/16] use bad_resource_id --- test_plugin/src/lib.rs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/test_plugin/src/lib.rs b/test_plugin/src/lib.rs index 1f84ec1f9b59e5..178c1568ff7a94 100644 --- a/test_plugin/src/lib.rs +++ b/test_plugin/src/lib.rs @@ -4,8 +4,8 @@ use std::borrow::Cow; use std::cell::RefCell; use std::rc::Rc; -use deno_core::error::anyhow; use deno_core::error::AnyError; +use deno_core::error::bad_resource_id; use deno_core::op_async; use deno_core::op_sync; use deno_core::Extension; @@ -94,9 +94,12 @@ fn op_test_resource_table_get( ) -> Result { println!("Hello from resource_table.get plugin op."); - if let Some(resource) = state.resource_table.get::(rid) { - Ok(resource.0.clone()) - } else { - Err(anyhow!("Could not find TestResource with id {}", rid)) - } + Ok( + state + .resource_table + .get::(rid) + .ok_or_else(bad_resource_id)? + .0 + .clone(), + ) } From 2bdcf066234c275bf008660f8912ec1f81e62b72 Mon Sep 17 00:00:00 2001 From: eliassjogreen Date: Mon, 3 May 2021 16:52:10 +0200 Subject: [PATCH 10/16] format --- test_plugin/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test_plugin/src/lib.rs b/test_plugin/src/lib.rs index 178c1568ff7a94..105071751b3cd8 100644 --- a/test_plugin/src/lib.rs +++ b/test_plugin/src/lib.rs @@ -4,8 +4,8 @@ use std::borrow::Cow; use std::cell::RefCell; use std::rc::Rc; -use deno_core::error::AnyError; 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; From 7aea64c2f46f29fd4ba798997b2325f2e79365cf Mon Sep 17 00:00:00 2001 From: eliassjogreen Date: Mon, 3 May 2021 18:59:55 +0200 Subject: [PATCH 11/16] fix test_plugin filename --- runtime/ops/plugin.rs | 25 ++++++++++--------------- test_plugin/tests/integration_tests.rs | 2 +- test_plugin/tests/test.js | 6 +++--- 3 files changed, 14 insertions(+), 19 deletions(-) diff --git a/runtime/ops/plugin.rs b/runtime/ops/plugin.rs index 5a0054df648320..6c20ab8f3f697f 100644 --- a/runtime/ops/plugin.rs +++ b/runtime/ops/plugin.rs @@ -35,21 +35,16 @@ pub fn op_open_plugin( debug!("Loading Plugin: {:#?}", filename); let plugin_lib = Library::open(filename).map(Rc::new)?; let plugin_resource = PluginResource::new(&plugin_lib); - - let rid; - let init; - { - rid = state.resource_table.add(plugin_resource); - init = *unsafe { - state - .resource_table - .get::(rid) - .unwrap() - .lib - .symbol::("init") - .unwrap() - }; - } + let rid = state.resource_table.add(plugin_resource); + let init = *unsafe { + state + .resource_table + .get::(rid) + .unwrap() + .lib + .symbol::("init") + .unwrap() + }; let mut extension = init(); diff --git a/test_plugin/tests/integration_tests.rs b/test_plugin/tests/integration_tests.rs index e7de1b2b7d3924..242342dc804c7e 100644 --- a/test_plugin/tests/integration_tests.rs +++ b/test_plugin/tests/integration_tests.rs @@ -34,7 +34,7 @@ fn basic() { println!("stderr {}", stderr); } assert!(output.status.success()); - let expected = "Plugin rid: 3\nSynced ops cache\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!\nOps dispatched count!\n"; + let expected = "Plugin rid: 3\nSynced ops cache\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 be2bba41ef191e..fc38ee5755d661 100644 --- a/test_plugin/tests/test.js +++ b/test_plugin/tests/test.js @@ -14,7 +14,7 @@ if (Deno.build.os === "darwin") { filenameSuffix = ".dylib"; } -const filename = `./target/${ +const filename = `../target/${ Deno.args[0] }/${filenamePrefix}${filenameBase}${filenameSuffix}`; @@ -105,12 +105,12 @@ function runTestOpCount() { if (end.opsCompleted - start.opsCompleted !== 1) { throw new Error("The opsCompleted metric is not correct!"); } - console.log("Ops completed count!"); + 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!"); + console.log("Ops dispatched count is correct!"); } function runTestPluginClose() { From dc744e5ce9b977230cce8c928d08bce9dd46ef98 Mon Sep 17 00:00:00 2001 From: eliassjogreen Date: Mon, 3 May 2021 21:09:05 +0200 Subject: [PATCH 12/16] reorder some stuff and try to fix stuff --- runtime/ops/plugin.rs | 15 +-------------- test_plugin/tests/integration_tests.rs | 1 + 2 files changed, 2 insertions(+), 14 deletions(-) diff --git a/runtime/ops/plugin.rs b/runtime/ops/plugin.rs index 6c20ab8f3f697f..eb124a3df5b921 100644 --- a/runtime/ops/plugin.rs +++ b/runtime/ops/plugin.rs @@ -35,17 +35,8 @@ pub fn op_open_plugin( debug!("Loading Plugin: {:#?}", filename); let plugin_lib = Library::open(filename).map(Rc::new)?; let plugin_resource = PluginResource::new(&plugin_lib); + let init = *unsafe { plugin_resource.lib.symbol::("init") }?; let rid = state.resource_table.add(plugin_resource); - let init = *unsafe { - state - .resource_table - .get::(rid) - .unwrap() - .lib - .symbol::("init") - .unwrap() - }; - let mut extension = init(); if !extension.init_js().is_empty() { @@ -73,10 +64,6 @@ impl Resource for PluginResource { fn name(&self) -> Cow { "plugin".into() } - - fn close(self: Rc) { - unimplemented!(); - } } impl PluginResource { diff --git a/test_plugin/tests/integration_tests.rs b/test_plugin/tests/integration_tests.rs index 242342dc804c7e..1859fb72bcb274 100644 --- a/test_plugin/tests/integration_tests.rs +++ b/test_plugin/tests/integration_tests.rs @@ -33,6 +33,7 @@ fn basic() { println!("stdout {}", stdout); println!("stderr {}", stderr); } + println!("{:?}", output.status); assert!(output.status.success()); let expected = "Plugin rid: 3\nSynced ops cache\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); From d954230c72cae1d33b3dad79848353ded1eef877 Mon Sep 17 00:00:00 2001 From: eliassjogreen Date: Wed, 5 May 2021 00:44:05 +0200 Subject: [PATCH 13/16] change the PluginResource struct a tiny bit --- runtime/ops/plugin.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/runtime/ops/plugin.rs b/runtime/ops/plugin.rs index eb124a3df5b921..ccd546cf61f5f0 100644 --- a/runtime/ops/plugin.rs +++ b/runtime/ops/plugin.rs @@ -35,7 +35,7 @@ pub fn op_open_plugin( debug!("Loading Plugin: {:#?}", filename); let plugin_lib = Library::open(filename).map(Rc::new)?; let plugin_resource = PluginResource::new(&plugin_lib); - let init = *unsafe { plugin_resource.lib.symbol::("init") }?; + let init = *unsafe { plugin_resource.0.symbol::("init") }?; let rid = state.resource_table.add(plugin_resource); let mut extension = init(); @@ -56,9 +56,7 @@ pub fn op_open_plugin( Ok(rid) } -struct PluginResource { - lib: Rc, -} +struct PluginResource(Rc); impl Resource for PluginResource { fn name(&self) -> Cow { @@ -68,6 +66,6 @@ impl Resource for PluginResource { impl PluginResource { fn new(lib: &Rc) -> Self { - Self { lib: lib.clone() } + Self(lib.clone()) } } From 16d48a6dbf8a95be37a8437500683008ae33c04e Mon Sep 17 00:00:00 2001 From: eliassjogreen Date: Wed, 5 May 2021 20:42:10 +0200 Subject: [PATCH 14/16] fix: segfault by leaking library --- runtime/ops/plugin.rs | 7 +++++++ test_plugin/tests/test.js | 4 ++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/runtime/ops/plugin.rs b/runtime/ops/plugin.rs index ccd546cf61f5f0..63e8649f94a6f2 100644 --- a/runtime/ops/plugin.rs +++ b/runtime/ops/plugin.rs @@ -10,6 +10,7 @@ use deno_core::ZeroCopyBuf; use dlopen::symbor::Library; use log::debug; use std::borrow::Cow; +use std::mem; use std::path::PathBuf; use std::rc::Rc; @@ -35,6 +36,8 @@ pub fn op_open_plugin( debug!("Loading Plugin: {:#?}", filename); let plugin_lib = Library::open(filename).map(Rc::new)?; let plugin_resource = PluginResource::new(&plugin_lib); + mem::forget(plugin_lib); + let init = *unsafe { plugin_resource.0.symbol::("init") }?; let rid = state.resource_table.add(plugin_resource); let mut extension = init(); @@ -62,6 +65,10 @@ impl Resource for PluginResource { fn name(&self) -> Cow { "plugin".into() } + + fn close(self: Rc) { + unimplemented!(); + } } impl PluginResource { diff --git a/test_plugin/tests/test.js b/test_plugin/tests/test.js index fc38ee5755d661..897d66c57e521c 100644 --- a/test_plugin/tests/test.js +++ b/test_plugin/tests/test.js @@ -114,8 +114,8 @@ function runTestOpCount() { } function runTestPluginClose() { - // Closing does not yet work, should it even? - Deno.close(rid); + // Closing does not yet work + Deno.close(pluginRid); const resourcesPost = Deno.resources(); From 909cfb41a96b746cff6cc12d236b935433a9d7f7 Mon Sep 17 00:00:00 2001 From: eliassjogreen Date: Wed, 5 May 2021 22:30:22 +0200 Subject: [PATCH 15/16] inline syncOpsCache --- runtime/js/40_plugins.js | 4 +++- test_plugin/tests/integration_tests.rs | 2 +- test_plugin/tests/test.js | 3 --- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/runtime/js/40_plugins.js b/runtime/js/40_plugins.js index e9a3142b439a68..0796fd5cee4a9c 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/test_plugin/tests/integration_tests.rs b/test_plugin/tests/integration_tests.rs index 1859fb72bcb274..203a8badedef24 100644 --- a/test_plugin/tests/integration_tests.rs +++ b/test_plugin/tests/integration_tests.rs @@ -35,7 +35,7 @@ fn basic() { } println!("{:?}", output.status); assert!(output.status.success()); - let expected = "Plugin rid: 3\nSynced ops cache\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"; + 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 897d66c57e521c..716b2ff9ac2023 100644 --- a/test_plugin/tests/test.js +++ b/test_plugin/tests/test.js @@ -23,9 +23,6 @@ const resourcesPre = Deno.resources(); const pluginRid = Deno.openPlugin(filename); console.log(`Plugin rid: ${pluginRid}`); -Deno.core.syncOpsCache(); -console.log("Synced ops cache"); - const { op_test_sync, op_test_async, From 9e66b468fc1bc550ede44043c8f578d9b5e42cbd Mon Sep 17 00:00:00 2001 From: eliassjogreen Date: Thu, 6 May 2021 23:12:57 +0200 Subject: [PATCH 16/16] comments --- runtime/ops/plugin.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/runtime/ops/plugin.rs b/runtime/ops/plugin.rs index 63e8649f94a6f2..a4ec0eece746f1 100644 --- a/runtime/ops/plugin.rs +++ b/runtime/ops/plugin.rs @@ -14,6 +14,13 @@ use std::mem; use std::path::PathBuf; use std::rc::Rc; +/// 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 { @@ -36,6 +43,8 @@ pub fn op_open_plugin( debug!("Loading Plugin: {:#?}", filename); let plugin_lib = Library::open(filename).map(Rc::new)?; let plugin_resource = PluginResource::new(&plugin_lib); + + // Forgets the plugin_lib value to prevent segfaults when the process exits mem::forget(plugin_lib); let init = *unsafe { plugin_resource.0.symbol::("init") }?;