Make inspector more robust, add --inspect-brk support (#4552)

cli/compilers/ts.rs

@@ -255,8 +255,9 @@ impl TsCompiler {
   fn setup_worker(global_state: GlobalState) -> CompilerWorker {
     let entry_point =
       ModuleSpecifier::resolve_url_or_path("./__$deno$ts_compiler.ts").unwrap();
-    let worker_state = State::new(global_state.clone(), None, entry_point)
-      .expect("Unable to create worker state");
+    let worker_state =
+      State::new(global_state.clone(), None, entry_point, DebugType::Internal)
+        .expect("Unable to create worker state");
 
     // Count how many times we start the compiler worker.
     global_state.compiler_starts.fetch_add(1, Ordering::SeqCst);

cli/compilers/wasm.rs

@@ -56,8 +56,9 @@ impl WasmCompiler {
     let entry_point =
       ModuleSpecifier::resolve_url_or_path("./__$deno$wasm_compiler.ts")
         .unwrap();
-    let worker_state = State::new(global_state.clone(), None, entry_point)
-      .expect("Unable to create worker state");
+    let worker_state =
+      State::new(global_state.clone(), None, entry_point, DebugType::Internal)
+        .expect("Unable to create worker state");
 
     // Count how many times we start the compiler worker.
     global_state.compiler_starts.fetch_add(1, Ordering::SeqCst);

cli/flags.rs

@@ -7,6 +7,7 @@ use clap::ArgMatches;
 use clap::SubCommand;
 use log::Level;
 use std::collections::HashSet;
+use std::net::SocketAddr;
 use std::path::{Path, PathBuf};
 
 /// Creates vector of strings, Vec<String>
@@ -107,8 +108,8 @@ pub struct Flags {
   pub no_prompts: bool,
   pub no_remote: bool,
   pub cached_only: bool,
-  pub inspect: Option<String>,
-  pub inspect_brk: Option<String>,
+  pub inspect: Option<SocketAddr>,
+  pub inspect_brk: Option<SocketAddr>,
   pub seed: Option<u64>,
   pub v8_flags: Option<Vec<String>>,
 
@@ -1021,6 +1022,7 @@ fn ca_file_arg<'a, 'b>() -> Arg<'a, 'b> {
     .help("Load certificate authority from PEM encoded file")
     .takes_value(true)
 }
+
 fn ca_file_arg_parse(flags: &mut Flags, matches: &clap::ArgMatches) {
   flags.ca_file = matches.value_of("cert").map(ToOwned::to_owned);
 }
@@ -1035,7 +1037,8 @@ fn inspect_args<'a, 'b>(app: App<'a, 'b>) -> App<'a, 'b> {
         .min_values(0)
         .max_values(1)
         .require_equals(true)
-        .takes_value(true),
+        .takes_value(true)
+        .validator(inspect_arg_validate),
     )
     .arg(
       Arg::with_name("inspect-brk")
@@ -1047,26 +1050,34 @@ fn inspect_args<'a, 'b>(app: App<'a, 'b>) -> App<'a, 'b> {
         .min_values(0)
         .max_values(1)
         .require_equals(true)
-        .takes_value(true),
+        .takes_value(true)
+        .validator(inspect_arg_validate),
     )
 }
 
+fn inspect_arg_validate(val: String) -> Result<(), String> {
+  match val.parse::<SocketAddr>() {
+    Ok(_) => Ok(()),
+    Err(e) => Err(e.to_string()),
+  }
+}
+
 fn inspect_arg_parse(flags: &mut Flags, matches: &clap::ArgMatches) {
-  const DEFAULT: &str = "127.0.0.1:9229";
+  let default = || "127.0.0.1:9229".parse::<SocketAddr>().unwrap();
   flags.inspect = if matches.is_present("inspect") {
     if let Some(host) = matches.value_of("inspect") {
-      Some(host.to_string())
+      Some(host.parse().unwrap())
     } else {
-      Some(DEFAULT.to_string())
+      Some(default())
     }
   } else {
     None
   };
   flags.inspect_brk = if matches.is_present("inspect-brk") {
     if let Some(host) = matches.value_of("inspect-brk") {
-      Some(host.to_string())
+      Some(host.parse().unwrap())
     } else {
-      Some(DEFAULT.to_string())
+      Some(default())
     }
   } else {
     None
@@ -2390,7 +2401,7 @@ mod tests {
           code: "const foo = 'bar'".to_string(),
           as_typescript: false,
         },
-        inspect: Some("127.0.0.1:9229".to_string()),
+        inspect: Some("127.0.0.1:9229".parse().unwrap()),
         allow_net: true,
         allow_env: true,
         allow_run: true,
@@ -2499,7 +2510,7 @@ mod tests {
       r.unwrap(),
       Flags {
         subcommand: DenoSubcommand::Repl {},
-        inspect: Some("127.0.0.1:9229".to_string()),
+        inspect: Some("127.0.0.1:9229".parse().unwrap()),
         allow_read: true,
         allow_write: true,
         allow_net: true,
@@ -2556,27 +2567,7 @@ mod tests {
         subcommand: DenoSubcommand::Run {
           script: "foo.js".to_string(),
         },
-        inspect: Some("127.0.0.1:9229".to_string()),
-        ..Flags::default()
-      }
-    );
-  }
-
-  #[test]
-  fn inspect_custom_host() {
-    let r = flags_from_vec_safe(svec![
-      "deno",
-      "run",
-      "--inspect=deno.land:80",
-      "foo.js"
-    ]);
-    assert_eq!(
-      r.unwrap(),
-      Flags {
-        subcommand: DenoSubcommand::Run {
-          script: "foo.js".to_string(),
-        },
-        inspect: Some("deno.land:80".to_string()),
+        inspect: Some("127.0.0.1:9229".parse().unwrap()),
         ..Flags::default()
       }
     );

cli/global_state.rs

@@ -9,7 +9,6 @@ use crate::deno_dir;
 use crate::file_fetcher::SourceFileFetcher;
 use crate::flags;
 use crate::http_cache;
-use crate::inspector::InspectorServer;
 use crate::lockfile::Lockfile;
 use crate::msg;
 use crate::permissions::DenoPermissions;
@@ -43,7 +42,6 @@ pub struct GlobalStateInner {
   pub wasm_compiler: WasmCompiler,
   pub lockfile: Option<Mutex<Lockfile>>,
   pub compiler_starts: AtomicUsize,
-  pub inspector_server: Option<InspectorServer>,
   compile_lock: AsyncMutex<()>,
 }
 
@@ -84,16 +82,7 @@ impl GlobalState {
       None
     };
 
-    let inspector_server = if let Some(ref host) = flags.inspect {
-      Some(InspectorServer::new(host, false))
-    } else if let Some(ref host) = flags.inspect_brk {
-      Some(InspectorServer::new(host, true))
-    } else {
-      None
-    };
-
     let inner = GlobalStateInner {
-      inspector_server,
       dir,
       permissions: DenoPermissions::from_flags(&flags),
       flags,

cli/lib.rs

@@ -70,6 +70,7 @@ use crate::file_fetcher::SourceFile;
 use crate::global_state::GlobalState;
 use crate::msg::MediaType;
 use crate::ops::io::get_stdio;
+use crate::state::DebugType;
 use crate::state::State;
 use crate::worker::MainWorker;
 use deno_core::v8_set_flags;
@@ -132,7 +133,7 @@ fn create_main_worker(
   global_state: GlobalState,
   main_module: ModuleSpecifier,
 ) -> Result<MainWorker, ErrBox> {
-  let state = State::new(global_state, None, main_module)?;
+  let state = State::new(global_state, None, main_module, DebugType::Main)?;
 
   {
     let mut s = state.borrow_mut();

cli/state.rs

@@ -33,6 +33,16 @@ use std::str;
 use std::thread::JoinHandle;
 use std::time::Instant;
 
+#[derive(Copy, Clone, Eq, PartialEq)]
+pub enum DebugType {
+  /// Can be debugged, will wait for debugger when --inspect-brk given.
+  Main,
+  /// Can be debugged, never waits for debugger.
+  Dependent,
+  /// No inspector instance is created.
+  Internal,
+}
+
 #[derive(Clone)]
 pub struct State(Rc<RefCell<StateInner>>);
 
@@ -59,6 +69,7 @@ pub struct StateInner {
   pub seeded_rng: Option<StdRng>,
   pub resource_table: ResourceTable,
   pub target_lib: TargetLib,
+  pub debug_type: DebugType,
 }
 
 impl State {
@@ -230,6 +241,7 @@ impl State {
     global_state: GlobalState,
     shared_permissions: Option<DenoPermissions>,
     main_module: ModuleSpecifier,
+    debug_type: DebugType,
   ) -> Result<Self, ErrBox> {
     let import_map: Option<ImportMap> =
       match global_state.flags.import_map_path.as_ref() {
@@ -259,9 +271,9 @@ impl State {
       next_worker_id: 0,
       start_time: Instant::now(),
       seeded_rng,
-
       resource_table: ResourceTable::default(),
       target_lib: TargetLib::Main,
+      debug_type,
     }));
 
     Ok(Self(state))
@@ -295,9 +307,9 @@ impl State {
       next_worker_id: 0,
       start_time: Instant::now(),
       seeded_rng,
-
       resource_table: ResourceTable::default(),
       target_lib: TargetLib::Worker,
+      debug_type: DebugType::Dependent,
     }));
 
     Ok(Self(state))
@@ -370,6 +382,7 @@ impl State {
       GlobalState::mock(vec!["deno".to_string()]),
       None,
       module_specifier,
+      DebugType::Main,
     )
     .unwrap()
   }

cli/tests/inspector2.js

+console.log("hello from the script");

cli/tests/integration_tests.rs

@@ -7,6 +7,8 @@ extern crate nix;
 extern crate pty;
 extern crate tempfile;
 
+use futures::prelude::*;
+use std::io::BufRead;
 use std::process::Command;
 use tempfile::TempDir;
 
@@ -1999,11 +2001,9 @@ fn test_permissions_net_listen_allow_localhost() {
   assert!(!err.contains(util::PERMISSION_DENIED_PATTERN));
 }
 
-#[cfg(not(target_os = "linux"))] // TODO(ry) broken on github actions.
 fn extract_ws_url_from_stderr(
   stderr: &mut std::process::ChildStderr,
 ) -> url::Url {
-  use std::io::BufRead;
   let mut stderr = std::io::BufReader::new(stderr);
   let mut stderr_first_line = String::from("");
   let _ = stderr.read_line(&mut stderr_first_line).unwrap();
@@ -2026,7 +2026,7 @@ async fn inspector_connect() {
     .arg("run")
     // Warning: each inspector test should be on its own port to avoid
     // conflicting with another inspector test.
-    .arg("--inspect=127.0.0.1:9229")
+    .arg("--inspect=127.0.0.1:9228")
     .arg(script)
     .stderr(std::process::Stdio::piped())
     .spawn()
@@ -2042,6 +2042,86 @@ async fn inspector_connect() {
   child.kill().unwrap();
 }
 
+enum TestStep {
+  StdOut(&'static str),
+  WsRecv(&'static str),
+  WsSend(&'static str),
+}
+
+#[tokio::test]
+async fn inspector_break_on_first_line() {
+  let script = deno::test_util::root_path()
+    .join("cli")
+    .join("tests")
+    .join("inspector2.js");
+  let mut child = util::deno_cmd()
+    .arg("run")
+    // Warning: each inspector test should be on its own port to avoid
+    // conflicting with another inspector test.
+    .arg("--inspect-brk=127.0.0.1:9229")
+    .arg(script)
+    .stdout(std::process::Stdio::piped())
+    .stderr(std::process::Stdio::piped())
+    .spawn()
+    .unwrap();
+
+  let stderr = child.stderr.as_mut().unwrap();
+  let ws_url = extract_ws_url_from_stderr(stderr);
+  let (socket, response) = tokio_tungstenite::connect_async(ws_url)
+    .await
+    .expect("Can't connect");
+  assert_eq!(response.status(), 101); // Switching protocols.
+
+  let (mut socket_tx, mut socket_rx) = socket.split();
+
+  let stdout = child.stdout.as_mut().unwrap();
+  let mut stdout_lines = std::io::BufReader::new(stdout).lines();
+
+  use TestStep::*;
+  let test_steps = vec![
+    WsSend(r#"{"id":1,"method":"Runtime.enable"}"#),
+    WsSend(r#"{"id":2,"method":"Debugger.enable"}"#),
+    WsRecv(
+      r#"{"method":"Runtime.executionContextCreated","params":{"context":{"id":1,"#,
+    ),
+    WsRecv(r#"{"id":1,"result":{}}"#),
+    WsRecv(r#"{"id":2,"result":{"debuggerId":"#),
+    WsSend(r#"{"id":3,"method":"Runtime.runIfWaitingForDebugger"}"#),
+    WsRecv(r#"{"id":3,"result":{}}"#),
+    WsRecv(r#"{"method":"Debugger.paused","#),
+    WsSend(
+      r#"{"id":5,"method":"Runtime.evaluate","params":{"expression":"Deno.core.print(\"hello from the inspector\\n\")","contextId":1,"includeCommandLineAPI":true,"silent":false,"returnByValue":true}}"#,
+    ),
+    WsRecv(r#"{"id":5,"result":{"result":{"type":"undefined"}}}"#),
+    StdOut("hello from the inspector"),
+    WsSend(r#"{"id":6,"method":"Debugger.resume"}"#),
+    WsRecv(r#"{"id":6,"result":{}}"#),
+    StdOut("hello from the script"),
+  ];
+
+  for step in test_steps {
+    match step {
+      StdOut(s) => match stdout_lines.next() {
+        Some(Ok(line)) => assert_eq!(line, s),
+        other => panic!(other),
+      },
+      WsRecv(s) => loop {
+        let msg = match socket_rx.next().await {
+          Some(Ok(msg)) => msg.to_string(),
+          other => panic!(other),
+        };
+        if !msg.starts_with(r#"{"method":"Debugger.scriptParsed","#) {
+          assert!(msg.starts_with(s));
+          break;
+        }
+      },
+      WsSend(s) => socket_tx.send(s.into()).await.unwrap(),
+    }
+  }
+
+  child.kill().unwrap();
+}
+
 #[cfg(not(target_os = "linux"))] // TODO(ry) broken on github actions.
 #[tokio::test]
 async fn inspector_pause() {
@@ -2059,7 +2139,6 @@ async fn inspector_pause() {
     .spawn()
     .unwrap();
   let ws_url = extract_ws_url_from_stderr(child.stderr.as_mut().unwrap());
-  println!("ws_url {}", ws_url);
   // We use tokio_tungstenite as a websocket client because warp (which is
   // a dependency of Deno) uses it.
   let (mut socket, _) = tokio_tungstenite::connect_async(ws_url)
@@ -2082,7 +2161,6 @@ async fn inspector_pause() {
     unreachable!()
   }
 
-  use futures::sink::SinkExt;
   socket
     .send(r#"{"id":6,"method":"Debugger.enable"}"#.into())
     .await

cli/worker.rs

 // Copyright 2018-2020 the Deno authors. All rights reserved. MIT license.
 use crate::fmt_errors::JSError;
+use crate::inspector::DenoInspector;
 use crate::ops;
+use crate::state::DebugType;
 use crate::state::State;
 use deno_core;
 use deno_core::Buf;
@@ -97,7 +99,7 @@ pub struct Worker {
   pub waker: AtomicWaker,
   pub(crate) internal_channels: WorkerChannelsInternal,
   external_channels: WorkerHandle,
-  inspector: Option<Box<crate::inspector::DenoInspector>>,
+  inspector: Option<Box<DenoInspector>>,
 }
 
 impl Worker {
@@ -107,10 +109,18 @@ impl Worker {
 
     let global_state = state.borrow().global_state.clone();
 
-    let inspector = global_state
-      .inspector_server
-      .as_ref()
-      .map(|s| s.add_inspector(&mut *isolate));
+    let inspect = global_state.flags.inspect.as_ref();
+    let inspect_brk = global_state.flags.inspect_brk.as_ref();
+    let inspector = inspect
+      .or(inspect_brk)
+      .and_then(|host| match state.borrow().debug_type {
+        DebugType::Main if inspect_brk.is_some() => Some((host, true)),
+        DebugType::Main | DebugType::Dependent => Some((host, false)),
+        DebugType::Internal => None,
+      })
+      .map(|(host, wait_for_debugger)| {
+        DenoInspector::new(&mut isolate, *host, wait_for_debugger)
+      });
 
     isolate.set_js_error_create_fn(move |core_js_error| {
       JSError::create(core_js_error, &global_state.ts_compiler)
@@ -287,8 +297,13 @@ mod tests {
     let module_specifier =
       ModuleSpecifier::resolve_url_or_path(&p.to_string_lossy()).unwrap();
     let global_state = GlobalState::new(flags::Flags::default()).unwrap();
-    let state =
-      State::new(global_state, None, module_specifier.clone()).unwrap();
+    let state = State::new(
+      global_state,
+      None,
+      module_specifier.clone(),
+      DebugType::Main,
+    )
+    .unwrap();
     let state_ = state.clone();
     tokio_util::run_basic(async move {
       let mut worker =
@@ -316,8 +331,13 @@ mod tests {
     let module_specifier =
       ModuleSpecifier::resolve_url_or_path(&p.to_string_lossy()).unwrap();
     let global_state = GlobalState::new(flags::Flags::default()).unwrap();
-    let state =
-      State::new(global_state, None, module_specifier.clone()).unwrap();
+    let state = State::new(
+      global_state,
+      None,
+      module_specifier.clone(),
+      DebugType::Main,
+    )
+    .unwrap();
     let state_ = state.clone();
     tokio_util::run_basic(async move {
       let mut worker =
@@ -354,8 +374,13 @@ mod tests {
       ..flags::Flags::default()
     };
     let global_state = GlobalState::new(flags).unwrap();
-    let state =
-      State::new(global_state.clone(), None, module_specifier.clone()).unwrap();
+    let state = State::new(
+      global_state.clone(),
+      None,
+      module_specifier.clone(),
+      DebugType::Main,
+    )
+    .unwrap();
     let mut worker = MainWorker::new(
       "TEST".to_string(),
       startup_data::deno_isolate_init(),