From fd631b4f926cba7b201042de5a861097b5de8f3b Mon Sep 17 00:00:00 2001 From: Divy Srivastava Date: Tue, 7 Mar 2023 08:51:57 +0530 Subject: [PATCH 01/11] feat(core): prevent isolate drop for CLI main worker --- cli/standalone.rs | 1 + cli/worker.rs | 2 ++ core/runtime.rs | 15 +++++++++++++-- runtime/worker.rs | 2 ++ 4 files changed, 18 insertions(+), 2 deletions(-) diff --git a/cli/standalone.rs b/cli/standalone.rs index 352e04e572cbd0..7d13f91a8430e8 100644 --- a/cli/standalone.rs +++ b/cli/standalone.rs @@ -302,6 +302,7 @@ pub async fn run( shared_array_buffer_store: None, compiled_wasm_module_store: None, stdio: Default::default(), + leak_isolate: true, }; let mut worker = MainWorker::bootstrap_from_options( main_module.clone(), diff --git a/cli/worker.rs b/cli/worker.rs index 7ef90d79fdfd9c..3a8cfcd72480a8 100644 --- a/cli/worker.rs +++ b/cli/worker.rs @@ -550,6 +550,7 @@ async fn create_main_worker_internal( shared_array_buffer_store: Some(ps.shared_array_buffer_store.clone()), compiled_wasm_module_store: Some(ps.compiled_wasm_module_store.clone()), stdio, + leak_isolate: !bench_or_test, }; let mut worker = MainWorker::bootstrap_from_options( @@ -780,6 +781,7 @@ mod tests { shared_array_buffer_store: None, compiled_wasm_module_store: None, stdio: Default::default(), + leak_isolate: false, }; MainWorker::bootstrap_from_options(main_module, permissions, options) diff --git a/core/runtime.rs b/core/runtime.rs index 1d74c9ede00658..94da6c37b5f633 100644 --- a/core/runtime.rs +++ b/core/runtime.rs @@ -93,6 +93,9 @@ pub struct JsRuntime { event_loop_middlewares: Vec>, // Marks if this is considered the top-level runtime. Used only be inspector. is_main: bool, + // Marks if it's OK to leak the current isolate. Use only by the + // CLI main worker. + leak_isolate: bool, } pub(crate) struct DynImportModEvaluate { @@ -305,6 +308,10 @@ pub struct RuntimeOptions { /// Describe if this is the main runtime instance, used by debuggers in some /// situation - like disconnecting when program finishes running. pub is_main: bool, + + /// Whether it is OK to leak the V8 isolate. Only to be used by CLI + /// top-level runtime. + pub leak_isolate: bool, } #[derive(Copy, Clone, PartialEq, Eq)] @@ -335,8 +342,11 @@ impl SnapshotOptions { impl Drop for JsRuntime { fn drop(&mut self) { - if let Some(v8_isolate) = self.v8_isolate.as_mut() { - Self::drop_state_and_module_map(v8_isolate); + if let Some(mut v8_isolate) = self.v8_isolate.take() { + Self::drop_state_and_module_map(&mut v8_isolate); + if self.leak_isolate { + std::mem::forget(v8_isolate); + } } } } @@ -674,6 +684,7 @@ impl JsRuntime { state: state_rc, module_map: Some(module_map_rc), is_main: options.is_main, + leak_isolate: options.leak_isolate, }; // Init resources and ops before extensions to make sure they are diff --git a/runtime/worker.rs b/runtime/worker.rs index 66497489d79112..4ab41d07c8de7e 100644 --- a/runtime/worker.rs +++ b/runtime/worker.rs @@ -141,6 +141,7 @@ pub struct WorkerOptions { /// `WebAssembly.Module` objects cannot be serialized. pub compiled_wasm_module_store: Option, pub stdio: Stdio, + pub leak_isolate: bool, } impl Default for WorkerOptions { @@ -298,6 +299,7 @@ impl MainWorker { extensions_with_js: options.extensions_with_js, inspector: options.maybe_inspector_server.is_some(), is_main: true, + leak_isolate: options.leak_isolate, ..Default::default() }); From 6d44c58420f446eba04dc633c308a86ffe211183 Mon Sep 17 00:00:00 2001 From: Divy Srivastava Date: Tue, 7 Mar 2023 08:53:37 +0530 Subject: [PATCH 02/11] default impl --- runtime/worker.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/runtime/worker.rs b/runtime/worker.rs index 4ab41d07c8de7e..12502a1ef1d2fd 100644 --- a/runtime/worker.rs +++ b/runtime/worker.rs @@ -178,6 +178,7 @@ impl Default for WorkerOptions { startup_snapshot: Default::default(), bootstrap: Default::default(), stdio: Default::default(), + leak_isolate: false, } } } From b719f3aceb5c3f27f57c58717b9713bf17a47618 Mon Sep 17 00:00:00 2001 From: Divy Srivastava Date: Tue, 7 Mar 2023 08:59:47 +0530 Subject: [PATCH 03/11] fix --- runtime/examples/hello_runtime.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/runtime/examples/hello_runtime.rs b/runtime/examples/hello_runtime.rs index fca7ff2c9c94d8..a47e9c908e13b8 100644 --- a/runtime/examples/hello_runtime.rs +++ b/runtime/examples/hello_runtime.rs @@ -66,6 +66,7 @@ async fn main() -> Result<(), AnyError> { shared_array_buffer_store: None, compiled_wasm_module_store: None, stdio: Default::default(), + leak_isolate: true, }; let js_path = From 36ee1aab353d750fab3382e750f38d80f8a4076b Mon Sep 17 00:00:00 2001 From: Divy Srivastava Date: Tue, 7 Mar 2023 20:20:28 +0530 Subject: [PATCH 04/11] maybe fix --- cli/worker.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/worker.rs b/cli/worker.rs index 3a8cfcd72480a8..e0344cf605843a 100644 --- a/cli/worker.rs +++ b/cli/worker.rs @@ -550,7 +550,7 @@ async fn create_main_worker_internal( shared_array_buffer_store: Some(ps.shared_array_buffer_store.clone()), compiled_wasm_module_store: Some(ps.compiled_wasm_module_store.clone()), stdio, - leak_isolate: !bench_or_test, + leak_isolate: !bench_or_test || ps.options.coverage_dir().is_none(), }; let mut worker = MainWorker::bootstrap_from_options( From 91fd0c78c892325767eaf4660446424b3d841352 Mon Sep 17 00:00:00 2001 From: Divy Srivastava Date: Tue, 7 Mar 2023 20:56:41 +0530 Subject: [PATCH 05/11] x --- cli/worker.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/worker.rs b/cli/worker.rs index e0344cf605843a..17d1d3c432efd4 100644 --- a/cli/worker.rs +++ b/cli/worker.rs @@ -550,7 +550,7 @@ async fn create_main_worker_internal( shared_array_buffer_store: Some(ps.shared_array_buffer_store.clone()), compiled_wasm_module_store: Some(ps.compiled_wasm_module_store.clone()), stdio, - leak_isolate: !bench_or_test || ps.options.coverage_dir().is_none(), + leak_isolate: !bench_or_test && ps.options.coverage_dir().is_none(), }; let mut worker = MainWorker::bootstrap_from_options( From 5f0cb4458729e4ffee097588251cb1e10ecf2c1f Mon Sep 17 00:00:00 2001 From: Divy Srivastava Date: Tue, 7 Mar 2023 23:28:27 +0530 Subject: [PATCH 06/11] x --- cli/worker.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cli/worker.rs b/cli/worker.rs index 17d1d3c432efd4..18b5d2e4794670 100644 --- a/cli/worker.rs +++ b/cli/worker.rs @@ -502,7 +502,7 @@ async fn create_main_worker_internal( let mut extensions = ops::cli_exts(ps.clone()); extensions.append(&mut custom_extensions); - let options = WorkerOptions { + let mut options = WorkerOptions { bootstrap: BootstrapOptions { args: ps.options.argv().clone(), cpu_count: std::thread::available_parallelism() @@ -550,7 +550,7 @@ async fn create_main_worker_internal( shared_array_buffer_store: Some(ps.shared_array_buffer_store.clone()), compiled_wasm_module_store: Some(ps.compiled_wasm_module_store.clone()), stdio, - leak_isolate: !bench_or_test && ps.options.coverage_dir().is_none(), + leak_isolate: !bench_or_test || ps.options.coverage_dir().is_none(), }; let mut worker = MainWorker::bootstrap_from_options( From ad81614292a5f0e489369758090faeca600c465b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Tue, 7 Mar 2023 19:33:39 +0100 Subject: [PATCH 07/11] don't take isolate if not leaking --- core/runtime.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/core/runtime.rs b/core/runtime.rs index 94da6c37b5f633..f01f750037a6a0 100644 --- a/core/runtime.rs +++ b/core/runtime.rs @@ -342,9 +342,11 @@ impl SnapshotOptions { impl Drop for JsRuntime { fn drop(&mut self) { - if let Some(mut v8_isolate) = self.v8_isolate.take() { + if let Some(mut v8_isolate) = self.v8_isolate.as_mut() { Self::drop_state_and_module_map(&mut v8_isolate); - if self.leak_isolate { + } + if self.leak_isolate { + if let Some(v8_isolate) = self.v8_isolate.take() { std::mem::forget(v8_isolate); } } From e65690c708c5cdcb38df638f6f2e84bb9033e89a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Tue, 7 Mar 2023 19:36:55 +0100 Subject: [PATCH 08/11] lint --- cli/worker.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/worker.rs b/cli/worker.rs index 18b5d2e4794670..e0344cf605843a 100644 --- a/cli/worker.rs +++ b/cli/worker.rs @@ -502,7 +502,7 @@ async fn create_main_worker_internal( let mut extensions = ops::cli_exts(ps.clone()); extensions.append(&mut custom_extensions); - let mut options = WorkerOptions { + let options = WorkerOptions { bootstrap: BootstrapOptions { args: ps.options.argv().clone(), cpu_count: std::thread::available_parallelism() From 0f90a29f12b612882273c421e4fa475cefcd3d68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Tue, 7 Mar 2023 20:39:58 +0100 Subject: [PATCH 09/11] lint again :) --- core/runtime.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/runtime.rs b/core/runtime.rs index f01f750037a6a0..98d6c63a69aa0d 100644 --- a/core/runtime.rs +++ b/core/runtime.rs @@ -342,8 +342,8 @@ impl SnapshotOptions { impl Drop for JsRuntime { fn drop(&mut self) { - if let Some(mut v8_isolate) = self.v8_isolate.as_mut() { - Self::drop_state_and_module_map(&mut v8_isolate); + if let Some(v8_isolate) = self.v8_isolate.as_mut() { + Self::drop_state_and_module_map(v8_isolate); } if self.leak_isolate { if let Some(v8_isolate) = self.v8_isolate.take() { From 0bbb357d8e7dfd8616df21d9fd9fd94357f86acb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Wed, 8 Mar 2023 14:09:30 +0100 Subject: [PATCH 10/11] fix condition --- cli/worker.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/worker.rs b/cli/worker.rs index e0344cf605843a..17d1d3c432efd4 100644 --- a/cli/worker.rs +++ b/cli/worker.rs @@ -550,7 +550,7 @@ async fn create_main_worker_internal( shared_array_buffer_store: Some(ps.shared_array_buffer_store.clone()), compiled_wasm_module_store: Some(ps.compiled_wasm_module_store.clone()), stdio, - leak_isolate: !bench_or_test || ps.options.coverage_dir().is_none(), + leak_isolate: !bench_or_test && ps.options.coverage_dir().is_none(), }; let mut worker = MainWorker::bootstrap_from_options( From 7f329ad1cf45a8dc549a745b663a13afb8fe4626 Mon Sep 17 00:00:00 2001 From: Divy Srivastava Date: Wed, 8 Mar 2023 21:52:47 +0530 Subject: [PATCH 11/11] clear gotham state --- core/gotham_state.rs | 4 ++++ core/ops.rs | 4 ++++ core/runtime.rs | 5 +++++ 3 files changed, 13 insertions(+) diff --git a/core/gotham_state.rs b/core/gotham_state.rs index 422499f2ffc1a8..43019eabba7bec 100644 --- a/core/gotham_state.rs +++ b/core/gotham_state.rs @@ -76,6 +76,10 @@ impl GothamState { pub fn take(&mut self) -> T { self.try_take().unwrap_or_else(|| missing::()) } + + pub fn clear(&mut self) { + self.data.clear(); + } } fn missing() -> ! { diff --git a/core/ops.rs b/core/ops.rs index ca465c821e28e9..b2442634e55129 100644 --- a/core/ops.rs +++ b/core/ops.rs @@ -179,6 +179,10 @@ impl OpState { tracker: OpsTracker::new(ops_count), } } + + pub fn clear_state(&mut self) { + self.gotham_state.clear(); + } } impl Deref for OpState { diff --git a/core/runtime.rs b/core/runtime.rs index 98d6c63a69aa0d..1ee7886b566e4e 100644 --- a/core/runtime.rs +++ b/core/runtime.rs @@ -347,6 +347,11 @@ impl Drop for JsRuntime { } if self.leak_isolate { if let Some(v8_isolate) = self.v8_isolate.take() { + // Clear the GothamState. This allows final env cleanup hooks to run. + // Note: that OpState is cloned for every OpCtx, so we can't just drop + // one reference to it. + let rc_state = self.op_state(); + rc_state.borrow_mut().clear_state(); std::mem::forget(v8_isolate); } }