From 060dd3ae82caf7cab57dfb40a295028887ab70bb Mon Sep 17 00:00:00 2001 From: Nayeem Rahman Date: Mon, 5 Jul 2021 17:59:49 +0100 Subject: [PATCH] fix(core): Delay deadlock detection for dynamic imports (#11282) --- cli/tests/integration/run_tests.rs | 6 +++ cli/tests/top_level_await_nested.out | 5 +++ cli/tests/top_level_await_nested/a.js | 3 ++ cli/tests/top_level_await_nested/b.js | 1 + cli/tests/top_level_await_nested/main.js | 3 ++ core/runtime.rs | 47 ++++++++++++------------ 6 files changed, 41 insertions(+), 24 deletions(-) create mode 100644 cli/tests/top_level_await_nested.out create mode 100644 cli/tests/top_level_await_nested/a.js create mode 100644 cli/tests/top_level_await_nested/b.js create mode 100644 cli/tests/top_level_await_nested/main.js diff --git a/cli/tests/integration/run_tests.rs b/cli/tests/integration/run_tests.rs index 374cd473bb..14e212dd7f 100644 --- a/cli/tests/integration/run_tests.rs +++ b/cli/tests/integration/run_tests.rs @@ -866,6 +866,12 @@ itest!(top_level_await_circular { exit_code: 1, }); +// Regression test for https://github.com/denoland/deno/issues/11238. +itest!(top_level_await_nested { + args: "run --allow-read top_level_await_nested/main.js", + output: "top_level_await_nested.out", +}); + itest!(top_level_await_unresolved { args: "run top_level_await_unresolved.js", output: "top_level_await_unresolved.out", diff --git a/cli/tests/top_level_await_nested.out b/cli/tests/top_level_await_nested.out new file mode 100644 index 0000000000..8a1218a102 --- /dev/null +++ b/cli/tests/top_level_await_nested.out @@ -0,0 +1,5 @@ +1 +2 +3 +4 +5 diff --git a/cli/tests/top_level_await_nested/a.js b/cli/tests/top_level_await_nested/a.js new file mode 100644 index 0000000000..74837d4ba1 --- /dev/null +++ b/cli/tests/top_level_await_nested/a.js @@ -0,0 +1,3 @@ +console.log(2); +await import("./b.js"); +console.log(4); diff --git a/cli/tests/top_level_await_nested/b.js b/cli/tests/top_level_await_nested/b.js new file mode 100644 index 0000000000..3bd241b50e --- /dev/null +++ b/cli/tests/top_level_await_nested/b.js @@ -0,0 +1 @@ +console.log(3); diff --git a/cli/tests/top_level_await_nested/main.js b/cli/tests/top_level_await_nested/main.js new file mode 100644 index 0000000000..ed46a47175 --- /dev/null +++ b/cli/tests/top_level_await_nested/main.js @@ -0,0 +1,3 @@ +console.log(1); +await import("./a.js"); +console.log(5); diff --git a/core/runtime.rs b/core/runtime.rs index f156b60f38..48003c811d 100644 --- a/core/runtime.rs +++ b/core/runtime.rs @@ -32,7 +32,6 @@ use futures::Future; use std::any::Any; use std::cell::RefCell; use std::collections::HashMap; -use std::collections::VecDeque; use std::convert::TryFrom; use std::ffi::c_void; use std::mem::forget; @@ -107,8 +106,11 @@ pub(crate) struct JsRuntimeState { pub(crate) js_wasm_streaming_cb: Option>, pub(crate) pending_promise_exceptions: HashMap, v8::Global>, - pending_dyn_mod_evaluate: VecDeque, + pending_dyn_mod_evaluate: Vec, pending_mod_evaluate: Option, + /// A counter used to delay our dynamic import deadlock detection by one spin + /// of the event loop. + dyn_module_evaluate_idle_counter: u32, pub(crate) js_error_create_fn: Rc, pub(crate) pending_ops: FuturesUnordered, pub(crate) pending_unref_ops: FuturesUnordered, @@ -283,8 +285,9 @@ impl JsRuntime { isolate.set_slot(Rc::new(RefCell::new(JsRuntimeState { global_context: Some(global_context), pending_promise_exceptions: HashMap::new(), - pending_dyn_mod_evaluate: VecDeque::new(), + pending_dyn_mod_evaluate: vec![], pending_mod_evaluate: None, + dyn_module_evaluate_idle_counter: 0, js_recv_cb: None, js_macrotask_cb: None, js_wasm_streaming_cb: None, @@ -660,7 +663,7 @@ impl JsRuntime { // Top level module self.evaluate_pending_module(); - let state = state_rc.borrow(); + let mut state = state_rc.borrow_mut(); let module_map = module_map_rc.borrow(); let has_pending_ops = !state.pending_ops.is_empty(); @@ -720,7 +723,7 @@ impl JsRuntime { || has_pending_background_tasks { // pass, will be polled again - } else { + } else if state.dyn_module_evaluate_idle_counter >= 1 { let mut msg = "Dynamically imported module evaluation is still pending but there are no pending ops. This situation is often caused by unresolved promise. Pending dynamic modules:\n".to_string(); for pending_evaluate in &state.pending_dyn_mod_evaluate { @@ -730,6 +733,12 @@ Pending dynamic modules:\n".to_string(); msg.push_str(&format!("- {}", module_info.name.as_str())); } return Poll::Ready(Err(generic_error(msg))); + } else { + // Delay the above error by one spin of the event loop. A dynamic import + // evaluation may complete during this, in which case the counter will + // reset. + state.dyn_module_evaluate_idle_counter += 1; + state.waker.wake(); } } @@ -903,9 +912,7 @@ impl JsRuntime { module: module_global, }; - state - .pending_dyn_mod_evaluate - .push_back(dyn_import_mod_evaluate); + state.pending_dyn_mod_evaluate.push(dyn_import_mod_evaluate); } else { assert!(status == v8::ModuleStatus::Errored); } @@ -1021,6 +1028,7 @@ impl JsRuntime { } fn dynamic_import_resolve(&mut self, id: ModuleLoadId, mod_id: ModuleId) { + let state_rc = Self::state(self.v8_isolate()); let module_map_rc = Self::module_map(self.v8_isolate()); let scope = &mut self.handle_scope(); @@ -1047,6 +1055,7 @@ impl JsRuntime { // will reach into `ModuleMap` from within the isolate. let module_namespace = module.get_module_namespace(); resolver.resolve(scope, module_namespace).unwrap(); + state_rc.borrow_mut().dyn_module_evaluate_idle_counter = 0; scope.perform_microtask_checkpoint(); } @@ -1214,18 +1223,12 @@ impl JsRuntime { fn evaluate_dyn_imports(&mut self) { let state_rc = Self::state(self.v8_isolate()); - - loop { - let maybe_pending_dyn_evaluate = - state_rc.borrow_mut().pending_dyn_mod_evaluate.pop_front(); - - if maybe_pending_dyn_evaluate.is_none() { - break; - } - + let mut still_pending = vec![]; + let pending = + std::mem::take(&mut state_rc.borrow_mut().pending_dyn_mod_evaluate); + for pending_dyn_evaluate in pending { let maybe_result = { let scope = &mut self.handle_scope(); - let pending_dyn_evaluate = maybe_pending_dyn_evaluate.unwrap(); let module_id = pending_dyn_evaluate.module_id; let promise = pending_dyn_evaluate.promise.get(scope); @@ -1234,10 +1237,7 @@ impl JsRuntime { match promise_state { v8::PromiseState::Pending => { - state_rc - .borrow_mut() - .pending_dyn_mod_evaluate - .push_back(pending_dyn_evaluate); + still_pending.push(pending_dyn_evaluate); None } v8::PromiseState::Fulfilled => { @@ -1262,10 +1262,9 @@ impl JsRuntime { self.dynamic_import_reject(dyn_import_id, err1); } } - } else { - break; } } + state_rc.borrow_mut().pending_dyn_mod_evaluate = still_pending; } /// Asynchronously load specified module and all of its dependencies