From f64a44810e1fc732cd5a0aecec541c6c8a289806 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Sat, 26 Sep 2020 18:16:33 +0200 Subject: [PATCH] refactor: combine MainWorker::new and MainWorker::create (#7693) * combine MainWorker::new and MainWorker::create * remove compiler_starts fields * make op_state types explicit for readability --- cli/global_state.rs | 3 - cli/main.rs | 17 ++-- cli/tsc.rs | 6 -- cli/web_worker.rs | 1 - cli/worker.rs | 214 +++++++++++++++----------------------------- 5 files changed, 81 insertions(+), 160 deletions(-) diff --git a/cli/global_state.rs b/cli/global_state.rs index 590f97a769..3fe755ff15 100644 --- a/cli/global_state.rs +++ b/cli/global_state.rs @@ -24,7 +24,6 @@ use std::env; use std::fs; use std::io; use std::rc::Rc; -use std::sync::atomic::AtomicUsize; use std::sync::Arc; use std::sync::Mutex; @@ -48,7 +47,6 @@ pub struct GlobalState { pub file_fetcher: SourceFileFetcher, pub ts_compiler: TsCompiler, pub lockfile: Option>, - pub compiler_starts: AtomicUsize, pub maybe_import_map: Option, pub maybe_inspector_server: Option>, } @@ -109,7 +107,6 @@ impl GlobalState { lockfile, maybe_import_map, maybe_inspector_server, - compiler_starts: AtomicUsize::new(0), }; Ok(Arc::new(global_state)) } diff --git a/cli/main.rs b/cli/main.rs index 903dceaba0..94afbaebae 100644 --- a/cli/main.rs +++ b/cli/main.rs @@ -194,7 +194,7 @@ async fn install_command( ) -> Result<(), AnyError> { let global_state = GlobalState::new(flags.clone())?; let main_module = ModuleSpecifier::resolve_url_or_path(&module_url)?; - let mut worker = MainWorker::create(&global_state, main_module.clone())?; + let mut worker = MainWorker::new(&global_state, main_module.clone()); // First, fetch and compile the module; this step ensures that the module exists. worker.preload_module(&main_module).await?; installer::install(flags, &module_url, args, name, root, force) @@ -226,7 +226,7 @@ async fn cache_command( let main_module = ModuleSpecifier::resolve_url_or_path("./$deno$cache.ts").unwrap(); let global_state = GlobalState::new(flags)?; - let mut worker = MainWorker::create(&global_state, main_module.clone())?; + let mut worker = MainWorker::new(&global_state, main_module.clone()); for file in files { let specifier = ModuleSpecifier::resolve_url_or_path(&file)?; @@ -248,7 +248,7 @@ async fn eval_command( let main_module = ModuleSpecifier::resolve_url_or_path("./$deno$eval.ts").unwrap(); let global_state = GlobalState::new(flags)?; - let mut worker = MainWorker::create(&global_state, main_module.clone())?; + let mut worker = MainWorker::new(&global_state, main_module.clone()); let main_module_url = main_module.as_url().to_owned(); // Create a dummy source file. let source_code = if print { @@ -429,7 +429,7 @@ async fn run_repl(flags: Flags) -> Result<(), AnyError> { let main_module = ModuleSpecifier::resolve_url_or_path("./$deno$repl.ts").unwrap(); let global_state = GlobalState::new(flags)?; - let mut worker = MainWorker::create(&global_state, main_module)?; + let mut worker = MainWorker::new(&global_state, main_module); loop { (&mut *worker).await?; } @@ -439,8 +439,7 @@ async fn run_from_stdin(flags: Flags) -> Result<(), AnyError> { let global_state = GlobalState::new(flags.clone())?; let main_module = ModuleSpecifier::resolve_url_or_path("./$deno$stdin.ts").unwrap(); - let mut worker = - MainWorker::create(&global_state.clone(), main_module.clone())?; + let mut worker = MainWorker::new(&global_state.clone(), main_module.clone()); let mut source = Vec::new(); std::io::stdin().read_to_end(&mut source)?; @@ -504,7 +503,7 @@ async fn run_with_watch(flags: Flags, script: String) -> Result<(), AnyError> { let gs = GlobalState::new(flags.clone()).unwrap(); let main_module = main_module.clone(); async move { - let mut worker = MainWorker::create(&gs, main_module.clone())?; + let mut worker = MainWorker::new(&gs, main_module.clone()); debug!("main_module {}", main_module); worker.execute_module(&main_module).await?; worker.execute("window.dispatchEvent(new Event('load'))")?; @@ -529,7 +528,7 @@ async fn run_command(flags: Flags, script: String) -> Result<(), AnyError> { let main_module = ModuleSpecifier::resolve_url_or_path(&script)?; let global_state = GlobalState::new(flags.clone())?; - let mut worker = MainWorker::create(&global_state, main_module.clone())?; + let mut worker = MainWorker::new(&global_state, main_module.clone()); debug!("main_module {}", main_module); worker.execute_module(&main_module).await?; worker.execute("window.dispatchEvent(new Event('load'))")?; @@ -571,7 +570,7 @@ async fn test_command( ); let main_module = ModuleSpecifier::resolve_url(&test_file_url.to_string()).unwrap(); - let mut worker = MainWorker::create(&global_state, main_module.clone())?; + let mut worker = MainWorker::new(&global_state, main_module.clone()); // Create a dummy source file. let source_file = SourceFile { filename: test_file_url.to_file_path().unwrap(), diff --git a/cli/tsc.rs b/cli/tsc.rs index 61d877e2dd..245247f132 100644 --- a/cli/tsc.rs +++ b/cli/tsc.rs @@ -44,7 +44,6 @@ use std::ops::Deref; use std::path::Path; use std::path::PathBuf; use std::str; -use std::sync::atomic::Ordering; use std::sync::Arc; use std::sync::Mutex; use swc_common::comments::Comment; @@ -959,11 +958,6 @@ fn execute_in_tsc( global_state: Arc, req: String, ) -> Result { - // TODO(bartlomieju): this is only used in testing, I'm not sure it's - // worth keeping around - // Count how many times we start the compiler worker. - global_state.compiler_starts.fetch_add(1, Ordering::SeqCst); - let mut js_runtime = JsRuntime::new(RuntimeOptions { startup_snapshot: Some(js::compiler_isolate_init()), ..Default::default() diff --git a/cli/web_worker.rs b/cli/web_worker.rs index 587bc055d7..cb2a8b87eb 100644 --- a/cli/web_worker.rs +++ b/cli/web_worker.rs @@ -101,7 +101,6 @@ impl WebWorker { global_state, loader, false, - false, ); let terminated = Arc::new(AtomicBool::new(false)); diff --git a/cli/worker.rs b/cli/worker.rs index 821c51a338..2fc02c6eec 100644 --- a/cli/worker.rs +++ b/cli/worker.rs @@ -7,7 +7,8 @@ use crate::js; use crate::metrics::Metrics; use crate::ops; use crate::ops::io::get_stdio; -use crate::ops::timers; +use crate::ops::timers::GlobalTimer; +use crate::ops::timers::StartTime; use crate::ops::worker_host::WorkerId; use crate::ops::worker_host::WorkersTable; use crate::permissions::Permissions; @@ -23,6 +24,7 @@ use deno_core::ModuleId; use deno_core::ModuleSpecifier; use deno_core::RuntimeOptions; use deno_core::Snapshot; +use deno_fetch::reqwest; use rand::rngs::StdRng; use rand::SeedableRng; use std::env; @@ -93,9 +95,8 @@ fn create_channels() -> (WorkerChannelsInternal, WorkerHandle) { /// This struct is meant to be used as a base struct for concrete /// type of worker that registers set of ops. /// -/// Currently there are three types of workers: +/// Currently there are two types of workers: /// - `MainWorker` -/// - `CompilerWorker` /// - `WebWorker` pub struct Worker { pub name: String, @@ -108,7 +109,6 @@ pub struct Worker { } impl Worker { - #[allow(clippy::too_many_arguments)] pub fn new( name: String, startup_snapshot: Option, @@ -117,7 +117,6 @@ impl Worker { global_state: Arc, state: Rc, is_main: bool, - is_internal: bool, ) -> Self { let global_state_ = global_state.clone(); @@ -134,39 +133,35 @@ impl Worker { let mut op_state = op_state.borrow_mut(); op_state.get_error_class_fn = &crate::errors::get_error_class_name; - let ca_file = global_state.flags.ca_file.as_deref(); - let client = crate::http_util::create_http_client(ca_file).unwrap(); - op_state.put(client); + op_state.put::(GlobalTimer::default()); + op_state.put::(StartTime::now()); + op_state.put::(Default::default()); + op_state.put::(WorkersTable::default()); + op_state.put::(WorkerId::default()); + op_state.put::(permissions); + op_state.put::(main_module); + op_state.put::>(global_state.clone()); - op_state.put(timers::GlobalTimer::default()); - op_state.put(timers::StartTime::now()); + op_state.put::({ + let ca_file = global_state.flags.ca_file.as_deref(); + crate::http_util::create_http_client(ca_file).unwrap() + }); if let Some(seed) = global_state.flags.seed { - op_state.put(StdRng::seed_from_u64(seed)); + let rng = StdRng::seed_from_u64(seed); + op_state.put::(rng); } - - op_state.put(Metrics::default()); - - op_state.put(WorkersTable::default()); - op_state.put(WorkerId::default()); - - op_state.put(permissions); - - op_state.put(main_module); - op_state.put(global_state.clone()); } - let inspector = if is_internal { - None - } else if let Some(inspector_server) = &global_state.maybe_inspector_server - { - Some(DenoInspector::new( - &mut isolate, - Some(inspector_server.clone()), - )) - } else { - None - }; + let inspector = + if let Some(inspector_server) = &global_state.maybe_inspector_server { + Some(DenoInspector::new( + &mut isolate, + Some(inspector_server.clone()), + )) + } else { + None + }; let should_break_on_first_statement = inspector.is_some() && is_main @@ -294,24 +289,19 @@ impl DerefMut for Worker { pub struct MainWorker(Worker); impl MainWorker { - // TODO(ry) combine MainWorker::new and MainWorker::create. - fn new( - name: String, - startup_snapshot: Option, - permissions: Permissions, + pub fn new( + global_state: &Arc, main_module: ModuleSpecifier, - global_state: Arc, ) -> Self { let loader = CliModuleLoader::new(global_state.maybe_import_map.clone()); let mut worker = Worker::new( - name, - startup_snapshot, - permissions, + "main".to_string(), + Some(js::deno_isolate_init()), + global_state.permissions.clone(), main_module, - global_state, + global_state.clone(), loader, true, - false, ); { ops::runtime::init(&mut worker); @@ -342,20 +332,6 @@ impl MainWorker { ops::tty::init(&mut worker); ops::worker_host::init(&mut worker); } - Self(worker) - } - - pub fn create( - global_state: &Arc, - main_module: ModuleSpecifier, - ) -> Result { - let mut worker = MainWorker::new( - "main".to_string(), - Some(js::deno_isolate_init()), - global_state.permissions.clone(), - main_module, - global_state.clone(), - ); { let op_state = worker.op_state(); let mut op_state = op_state.borrow_mut(); @@ -371,8 +347,10 @@ impl MainWorker { t.add("stderr", Box::new(stream)); } } - worker.execute("bootstrap.mainRuntime()")?; - Ok(worker) + worker + .execute("bootstrap.mainRuntime()") + .expect("Failed to execute bootstrap script"); + Self(worker) } } @@ -392,71 +370,57 @@ impl DerefMut for MainWorker { #[cfg(test)] mod tests { use super::*; - use crate::flags; + use crate::flags::DenoSubcommand; + use crate::flags::Flags; use crate::global_state::GlobalState; - use crate::js; - use crate::tokio_util; - use std::sync::atomic::Ordering; - #[test] - fn execute_mod_esm_imports_a() { + fn create_test_worker() -> MainWorker { + let main_module = + ModuleSpecifier::resolve_url_or_path("./hello.js").unwrap(); + let flags = Flags { + subcommand: DenoSubcommand::Run { + script: main_module.to_string(), + }, + ..Default::default() + }; + let global_state = GlobalState::mock(vec!["deno".to_string()], Some(flags)); + MainWorker::new(&global_state, main_module) + } + + #[tokio::test] + async fn execute_mod_esm_imports_a() { let p = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")) .parent() .unwrap() .join("cli/tests/esm_imports_a.js"); let module_specifier = ModuleSpecifier::resolve_url_or_path(&p.to_string_lossy()).unwrap(); - let global_state = GlobalState::new(flags::Flags::default()).unwrap(); - let global_state_ = global_state.clone(); - tokio_util::run_basic(async { - let mut worker = MainWorker::new( - "TEST".to_string(), - None, - global_state.permissions.clone(), - module_specifier.clone(), - global_state_, - ); - let result = worker.execute_module(&module_specifier).await; - if let Err(err) = result { - eprintln!("execute_mod err {:?}", err); - } - if let Err(e) = (&mut *worker).await { - panic!("Future got unexpected error: {:?}", e); - } - }); - // Check that we didn't start the compiler. - assert_eq!(global_state.compiler_starts.load(Ordering::SeqCst), 0); + let mut worker = create_test_worker(); + let result = worker.execute_module(&module_specifier).await; + if let Err(err) = result { + eprintln!("execute_mod err {:?}", err); + } + if let Err(e) = (&mut *worker).await { + panic!("Future got unexpected error: {:?}", e); + } } - #[test] - fn execute_mod_circular() { + #[tokio::test] + async fn execute_mod_circular() { let p = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")) .parent() .unwrap() .join("tests/circular1.ts"); let module_specifier = ModuleSpecifier::resolve_url_or_path(&p.to_string_lossy()).unwrap(); - let global_state = GlobalState::new(flags::Flags::default()).unwrap(); - let global_state_ = global_state.clone(); - tokio_util::run_basic(async { - let mut worker = MainWorker::new( - "TEST".to_string(), - None, - global_state_.permissions.clone(), - module_specifier.clone(), - global_state_, - ); - let result = worker.execute_module(&module_specifier).await; - if let Err(err) = result { - eprintln!("execute_mod err {:?}", err); - } - if let Err(e) = (&mut *worker).await { - panic!("Future got unexpected error: {:?}", e); - } - }); - - // Check that we didn't start the compiler. - assert_eq!(global_state.compiler_starts.load(Ordering::SeqCst), 0); + let mut worker = create_test_worker(); + let result = worker.execute_module(&module_specifier).await; + if let Err(err) = result { + eprintln!("execute_mod err {:?}", err); + } + if let Err(e) = (&mut *worker).await { + panic!("Future got unexpected error: {:?}", e); + } } #[tokio::test] @@ -468,22 +432,7 @@ mod tests { .join("cli/tests/006_url_imports.ts"); let module_specifier = ModuleSpecifier::resolve_url_or_path(&p.to_string_lossy()).unwrap(); - let flags = flags::Flags { - subcommand: flags::DenoSubcommand::Run { - script: module_specifier.to_string(), - }, - reload: true, - ..flags::Flags::default() - }; - let global_state = GlobalState::new(flags).unwrap(); - let mut worker = MainWorker::new( - "TEST".to_string(), - Some(js::deno_isolate_init()), - global_state.permissions.clone(), - module_specifier.clone(), - global_state.clone(), - ); - worker.execute("bootstrap.mainRuntime()").unwrap(); + let mut worker = create_test_worker(); let result = worker.execute_module(&module_specifier).await; if let Err(err) = result { eprintln!("execute_mod err {:?}", err); @@ -491,23 +440,6 @@ mod tests { if let Err(e) = (&mut *worker).await { panic!("Future got unexpected error: {:?}", e); } - // Check that we've only invoked the compiler once. - assert_eq!(global_state.compiler_starts.load(Ordering::SeqCst), 1); - } - - fn create_test_worker() -> MainWorker { - let main_module = - ModuleSpecifier::resolve_url_or_path("./hello.js").unwrap(); - let global_state = GlobalState::mock(vec!["deno".to_string()], None); - let mut worker = MainWorker::new( - "TEST".to_string(), - Some(js::deno_isolate_init()), - Permissions::allow_all(), - main_module, - global_state, - ); - worker.execute("bootstrap.mainRuntime()").unwrap(); - worker } #[tokio::test]