Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core): prevent isolate drop for CLI main worker #18059

Merged
merged 12 commits into from
Mar 8, 2023
1 change: 1 addition & 0 deletions cli/standalone.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
2 changes: 2 additions & 0 deletions cli/worker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 && ps.options.coverage_dir().is_none(),
};

let mut worker = MainWorker::bootstrap_from_options(
Expand Down Expand Up @@ -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)
Expand Down
4 changes: 4 additions & 0 deletions core/gotham_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,10 @@ impl GothamState {
pub fn take<T: 'static>(&mut self) -> T {
self.try_take().unwrap_or_else(|| missing::<T>())
}

pub fn clear(&mut self) {
self.data.clear();
}
}

fn missing<T: 'static>() -> ! {
Expand Down
4 changes: 4 additions & 0 deletions core/ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
18 changes: 18 additions & 0 deletions core/runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,9 @@ pub struct JsRuntime {
event_loop_middlewares: Vec<Box<OpEventLoopFn>>,
// 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 {
Expand Down Expand Up @@ -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)]
Expand Down Expand Up @@ -338,6 +345,16 @@ impl Drop for JsRuntime {
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() {
// 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);
}
}
}
}

Expand Down Expand Up @@ -674,6 +691,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
Expand Down
1 change: 1 addition & 0 deletions runtime/examples/hello_runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down
3 changes: 3 additions & 0 deletions runtime/worker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ pub struct WorkerOptions {
/// `WebAssembly.Module` objects cannot be serialized.
pub compiled_wasm_module_store: Option<CompiledWasmModuleStore>,
pub stdio: Stdio,
pub leak_isolate: bool,
}

impl Default for WorkerOptions {
Expand Down Expand Up @@ -177,6 +178,7 @@ impl Default for WorkerOptions {
startup_snapshot: Default::default(),
bootstrap: Default::default(),
stdio: Default::default(),
leak_isolate: false,
}
}
}
Expand Down Expand Up @@ -298,6 +300,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()
});

Expand Down