From c909faf9e6cd2964398da7c0852d0229cdd1a22b Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Fri, 30 Jul 2021 13:36:43 +0200 Subject: [PATCH] chore(core): use oneshot channel in mod_evaluate() (#11556) Oneshot is more appropriate because mod_evaluate() only sends a single value. It also makes it easier to use it correctly. As an embedder, I wasn't sure if I'm expected to drain the channel or not. --- core/modules.rs | 10 +++++----- core/runtime.rs | 15 +++++++-------- runtime/web_worker.rs | 4 ++-- runtime/worker.rs | 5 ++--- 4 files changed, 16 insertions(+), 18 deletions(-) diff --git a/core/modules.rs b/core/modules.rs index 7655361079..3c8976c848 100644 --- a/core/modules.rs +++ b/core/modules.rs @@ -906,7 +906,7 @@ mod tests { let a_id_fut = runtime.load_module(&spec, None); let a_id = futures::executor::block_on(a_id_fut).expect("Failed to load"); - runtime.mod_evaluate(a_id); + let _ = runtime.mod_evaluate(a_id); futures::executor::block_on(runtime.run_event_loop(false)).unwrap(); let l = loads.lock(); assert_eq!( @@ -1076,7 +1076,7 @@ mod tests { runtime.instantiate_module(mod_a).unwrap(); assert_eq!(dispatch_count.load(Ordering::Relaxed), 0); - runtime.mod_evaluate(mod_a); + let _ = runtime.mod_evaluate(mod_a); assert_eq!(dispatch_count.load(Ordering::Relaxed), 1); } @@ -1366,7 +1366,7 @@ mod tests { let result = runtime.load_module(&spec, None).await; assert!(result.is_ok()); let circular1_id = result.unwrap(); - runtime.mod_evaluate(circular1_id); + let _ = runtime.mod_evaluate(circular1_id); runtime.run_event_loop(false).await.unwrap(); let l = loads.lock(); @@ -1439,7 +1439,7 @@ mod tests { println!(">> result {:?}", result); assert!(result.is_ok()); let redirect1_id = result.unwrap(); - runtime.mod_evaluate(redirect1_id); + let _ = runtime.mod_evaluate(redirect1_id); runtime.run_event_loop(false).await.unwrap(); let l = loads.lock(); assert_eq!( @@ -1588,7 +1588,7 @@ mod tests { let main_id = futures::executor::block_on(main_id_fut).expect("Failed to load"); - runtime.mod_evaluate(main_id); + let _ = runtime.mod_evaluate(main_id); futures::executor::block_on(runtime.run_event_loop(false)).unwrap(); let l = loads.lock(); diff --git a/core/runtime.rs b/core/runtime.rs index fc7a7228c6..8476e5729b 100644 --- a/core/runtime.rs +++ b/core/runtime.rs @@ -22,7 +22,7 @@ use crate::OpPayload; use crate::OpResult; use crate::OpState; use crate::PromiseId; -use futures::channel::mpsc; +use futures::channel::oneshot; use futures::future::poll_fn; use futures::future::FutureExt; use futures::stream::FuturesUnordered; @@ -96,7 +96,7 @@ struct DynImportModEvaluate { struct ModEvaluate { promise: v8::Global, - sender: mpsc::Sender>, + sender: oneshot::Sender>, } #[derive(Default, Clone)] @@ -978,7 +978,7 @@ impl JsRuntime { pub fn mod_evaluate( &mut self, id: ModuleId, - ) -> mpsc::Receiver> { + ) -> oneshot::Receiver> { let state_rc = Self::state(self.v8_isolate()); let module_map_rc = Self::module_map(self.v8_isolate()); let scope = &mut self.handle_scope(); @@ -991,7 +991,7 @@ impl JsRuntime { let mut status = module.get_status(); assert_eq!(status, v8::ModuleStatus::Instantiated); - let (sender, receiver) = mpsc::channel(1); + let (sender, receiver) = oneshot::channel(); // IMPORTANT: Top-level-await is enabled, which means that return value // of module evaluation is a promise. @@ -1238,7 +1238,6 @@ impl JsRuntime { let scope = &mut self.handle_scope(); let promise = module_evaluation.promise.get(scope); - let mut sender = module_evaluation.sender.clone(); let promise_state = promise.state(); match promise_state { @@ -1250,7 +1249,7 @@ impl JsRuntime { v8::PromiseState::Fulfilled => { scope.perform_microtask_checkpoint(); // Receiver end might have been already dropped, ignore the result - let _ = sender.try_send(Ok(())); + let _ = module_evaluation.sender.send(Ok(())); } v8::PromiseState::Rejected => { let exception = promise.result(scope); @@ -1259,7 +1258,7 @@ impl JsRuntime { .map_err(|err| attach_handle_to_error(scope, err, exception)) .unwrap_err(); // Receiver end might have been already dropped, ignore the result - let _ = sender.try_send(Err(err1)); + let _ = module_evaluation.sender.send(Err(err1)); } } } @@ -1955,7 +1954,7 @@ pub mod tests { ) .unwrap(); - runtime.mod_evaluate(module_id); + let _ = runtime.mod_evaluate(module_id); futures::executor::block_on(runtime.run_event_loop(false)).unwrap(); let _snapshot = runtime.snapshot(); diff --git a/runtime/web_worker.rs b/runtime/web_worker.rs index 344bb73c04..773fce80f7 100644 --- a/runtime/web_worker.rs +++ b/runtime/web_worker.rs @@ -457,7 +457,7 @@ impl WebWorker { let mut receiver = self.js_runtime.mod_evaluate(id); tokio::select! { - maybe_result = receiver.next() => { + maybe_result = &mut receiver => { debug!("received worker module evaluate {:#?}", maybe_result); // If `None` is returned it means that runtime was destroyed before // evaluation was complete. This can happen in Web Worker when `self.close()` @@ -470,7 +470,7 @@ impl WebWorker { return Ok(()); } event_loop_result?; - let maybe_result = receiver.next().await; + let maybe_result = receiver.await; maybe_result.unwrap_or(Ok(())) } } diff --git a/runtime/worker.rs b/runtime/worker.rs index f7287cfbb8..94edd6f1e5 100644 --- a/runtime/worker.rs +++ b/runtime/worker.rs @@ -7,7 +7,6 @@ use crate::ops; use crate::permissions::Permissions; use deno_broadcast_channel::InMemoryBroadcastChannel; use deno_core::error::AnyError; -use deno_core::futures::stream::StreamExt; use deno_core::futures::Future; use deno_core::located_script_name; use deno_core::serde_json; @@ -218,14 +217,14 @@ impl MainWorker { self.wait_for_inspector_session(); let mut receiver = self.js_runtime.mod_evaluate(id); tokio::select! { - maybe_result = receiver.next() => { + maybe_result = &mut receiver => { debug!("received module evaluate {:#?}", maybe_result); maybe_result.expect("Module evaluation result not provided.") } event_loop_result = self.run_event_loop(false) => { event_loop_result?; - let maybe_result = receiver.next().await; + let maybe_result = receiver.await; maybe_result.expect("Module evaluation result not provided.") } }