Make --inspect-brk pause on the first line of _user_ code (#5250)

5 files changed, 95 insertions, 85 deletions

diff --git a/cli/inspector.rs b/cli/inspector.rs
index 9ccf4cd49..b67bb89f5 100644
--- a/cli/inspector.rs
+++ b/cli/inspector.rs
@@ -374,7 +374,6 @@ impl DenoInspector {
   pub fn new(
     isolate: &mut deno_core::CoreIsolate,
     host: SocketAddr,
-    wait_for_debugger: bool,
   ) -> Box<Self> {
     let deno_core::CoreIsolate {
       v8_isolate,
@@ -406,7 +405,7 @@ impl DenoInspector {
         v8::inspector::V8Inspector::create(scope, unsafe { &mut *self_ptr });
 
       let sessions = InspectorSessions::new(self_ptr, new_websocket_rx);
-      let flags = InspectorFlags::new(wait_for_debugger);
+      let flags = InspectorFlags::new();
       let waker = InspectorWaker::new(scope.isolate().thread_safe_handle());
 
       Self {
@@ -426,9 +425,6 @@ impl DenoInspector {
     self_.context_created(context, Self::CONTEXT_GROUP_ID, context_name);
 
     // Register this inspector with the server thread.
-    // Note: poll_sessions() might block if we need to wait for a
-    // debugger front-end to connect. Therefore the server thread must to be
-    // nofified *before* polling.
     InspectorServer::register_inspector(info);
 
     // Poll the session handler so we will get notified whenever there is
@@ -469,14 +465,9 @@ impl DenoInspector {
             replace(&mut self.flags.borrow_mut().session_handshake_done, false);
           match poll_result {
             Poll::Pending if handshake_done => {
-              let mut session = sessions.handshake.take().unwrap();
-              if replace(
-                &mut self.flags.borrow_mut().waiting_for_session,
-                false,
-              ) {
-                session.break_on_first_statement();
-              }
+              let session = sessions.handshake.take().unwrap();
               sessions.established.push(session);
+              take(&mut self.flags.borrow_mut().waiting_for_session);
             }
             Poll::Ready(_) => sessions.handshake = None,
             Poll::Pending => break,
@@ -547,6 +538,21 @@ impl DenoInspector {
       };
     }
   }
+
+  /// This function blocks the thread until at least one inspector client has
+  /// established a websocket connection and successfully completed the
+  /// handshake. After that, it instructs V8 to pause at the next statement.
+  pub fn wait_for_session_and_break_on_next_statement(&mut self) {
+    loop {
+      match self.sessions.get_mut().established.iter_mut().next() {
+        Some(session) => break session.break_on_next_statement(),
+        None => {
+          self.flags.get_mut().waiting_for_session = true;
+          let _ = self.poll_sessions(None).unwrap();
+        }
+      };
+    }
+  }
 }
 
 #[derive(Default)]
@@ -557,11 +563,8 @@ struct InspectorFlags {
 }
 
 impl InspectorFlags {
-  fn new(waiting_for_session: bool) -> RefCell<Self> {
-    let self_ = Self {
-      waiting_for_session,
-      ..Default::default()
-    };
+  fn new() -> RefCell<Self> {
+    let self_ = Self::default();
     RefCell::new(self_)
   }
 }
@@ -753,7 +756,7 @@ impl DenoInspectorSession {
     let _ = self.websocket_tx.unbounded_send(msg);
   }
 
-  pub fn break_on_first_statement(&mut self) {
+  pub fn break_on_next_statement(&mut self) {
     let reason = v8::inspector::StringView::from(&b"debugCommand"[..]);
     let detail = v8::inspector::StringView::empty();
     self.schedule_pause_on_next_statement(reason, detail);
diff --git a/cli/main.rs b/cli/main.rs
index 0b88d7346..5b90e7134 100644
--- a/cli/main.rs
+++ b/cli/main.rs
@@ -74,7 +74,6 @@ use crate::msg::MediaType;
 use crate::op_error::OpError;
 use crate::ops::io::get_stdio;
 use crate::permissions::Permissions;
-use crate::state::DebugType;
 use crate::state::State;
 use crate::tsc::TargetLib;
 use crate::worker::MainWorker;
@@ -140,7 +139,7 @@ fn create_main_worker(
   global_state: GlobalState,
   main_module: ModuleSpecifier,
 ) -> Result<MainWorker, ErrBox> {
-  let state = State::new(global_state, None, main_module, DebugType::Main)?;
+  let state = State::new(global_state, None, main_module, false)?;
 
   let mut worker = MainWorker::new(
     "main".to_string(),
diff --git a/cli/state.rs b/cli/state.rs
index 9501ed286..38ea5d750 100644
--- a/cli/state.rs
+++ b/cli/state.rs
@@ -3,6 +3,7 @@ use crate::file_fetcher::SourceFileFetcher;
 use crate::global_state::GlobalState;
 use crate::global_timer::GlobalTimer;
 use crate::import_map::ImportMap;
+use crate::inspector::DenoInspector;
 use crate::metrics::Metrics;
 use crate::op_error::OpError;
 use crate::ops::JsonOp;
@@ -11,6 +12,7 @@ use crate::permissions::Permissions;
 use crate::tsc::TargetLib;
 use crate::web_worker::WebWorkerHandle;
 use deno_core::Buf;
+use deno_core::CoreIsolate;
 use deno_core::ErrBox;
 use deno_core::ModuleLoadId;
 use deno_core::ModuleLoader;
@@ -31,15 +33,6 @@ use std::rc::Rc;
 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>>);
@@ -66,8 +59,9 @@ pub struct StateInner {
   pub start_time: Instant,
   pub seeded_rng: Option<StdRng>,
   pub target_lib: TargetLib,
-  pub debug_type: DebugType,
   pub is_main: bool,
+  pub is_internal: bool,
+  pub inspector: Option<Box<DenoInspector>>,
 }
 
 impl State {
@@ -370,7 +364,7 @@ impl State {
     global_state: GlobalState,
     shared_permissions: Option<Permissions>,
     main_module: ModuleSpecifier,
-    debug_type: DebugType,
+    is_internal: bool,
   ) -> Result<Self, ErrBox> {
     let import_map: Option<ImportMap> =
       match global_state.flags.import_map_path.as_ref() {
@@ -406,8 +400,9 @@ impl State {
       start_time: Instant::now(),
       seeded_rng,
       target_lib: TargetLib::Main,
-      debug_type,
       is_main: true,
+      is_internal,
+      inspector: None,
     }));
 
     Ok(Self(state))
@@ -442,8 +437,9 @@ impl State {
       start_time: Instant::now(),
       seeded_rng,
       target_lib: TargetLib::Worker,
-      debug_type: DebugType::Dependent,
       is_main: false,
+      is_internal: false,
+      inspector: None,
     }));
 
     Ok(Self(state))
@@ -512,6 +508,35 @@ impl State {
     }
   }
 
+  pub fn maybe_init_inspector(&self, isolate: &mut CoreIsolate) {
+    let mut state = self.borrow_mut();
+
+    if state.is_internal {
+      return;
+    };
+
+    let inspector_host = {
+      let global_state = &state.global_state;
+      match global_state
+        .flags
+        .inspect
+        .or(global_state.flags.inspect_brk)
+      {
+        Some(host) => host,
+        None => return,
+      }
+    };
+
+    let inspector = DenoInspector::new(isolate, inspector_host);
+    state.inspector.replace(inspector);
+  }
+
+  #[inline]
+  pub fn should_inspector_break_on_first_statement(&self) -> bool {
+    let state = self.borrow();
+    state.inspector.is_some() && state.is_main
+  }
+
   #[cfg(test)]
   pub fn mock(main_module: &str) -> State {
     let module_specifier = ModuleSpecifier::resolve_url_or_path(main_module)
@@ -520,7 +545,7 @@ impl State {
       GlobalState::mock(vec!["deno".to_string()]),
       None,
       module_specifier,
-      DebugType::Main,
+      false,
     )
     .unwrap()
   }
diff --git a/cli/tsc.rs b/cli/tsc.rs
index f71fe3747..ba898b456 100644
--- a/cli/tsc.rs
+++ b/cli/tsc.rs
@@ -15,7 +15,6 @@ use crate::permissions::Permissions;
 use crate::source_maps::SourceMapGetter;
 use crate::startup_data;
 use crate::state::State;
-use crate::state::*;
 use crate::tokio_util;
 use crate::version;
 use crate::web_worker::WebWorker;
@@ -351,13 +350,9 @@ impl TsCompiler {
   ) -> CompilerWorker {
     let entry_point =
       ModuleSpecifier::resolve_url_or_path("./__$deno$ts_compiler.ts").unwrap();
-    let worker_state = State::new(
-      global_state.clone(),
-      Some(permissions),
-      entry_point,
-      DebugType::Internal,
-    )
-    .expect("Unable to create worker state");
+    let worker_state =
+      State::new(global_state.clone(), Some(permissions), entry_point, true)
+        .expect("Unable to create worker state");
 
     // Count how many times we start the compiler worker.
     global_state.compiler_starts.fetch_add(1, Ordering::SeqCst);
diff --git a/cli/worker.rs b/cli/worker.rs
index f5eae9378..e91758e10 100644
--- a/cli/worker.rs
+++ b/cli/worker.rs
@@ -2,7 +2,6 @@
 use crate::fmt_errors::JSError;
 use crate::inspector::DenoInspector;
 use crate::ops;
-use crate::state::DebugType;
 use crate::state::State;
 use deno_core::Buf;
 use deno_core::ErrBox;
@@ -13,6 +12,7 @@ use futures::channel::mpsc;
 use futures::future::FutureExt;
 use futures::stream::StreamExt;
 use futures::task::AtomicWaker;
+use std::cell::RefMut;
 use std::env;
 use std::future::Future;
 use std::ops::Deref;
@@ -92,7 +92,6 @@ pub struct Worker {
   pub waker: AtomicWaker,
   pub(crate) internal_channels: WorkerChannelsInternal,
   external_channels: WorkerHandle,
-  pub(crate) inspector: Option<Box<DenoInspector>>,
 }
 
 impl Worker {
@@ -100,21 +99,9 @@ impl Worker {
     let loader = Rc::new(state.clone());
     let mut isolate = deno_core::EsIsolate::new(loader, startup_data, false);
 
-    let global_state = state.borrow().global_state.clone();
-
-    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)
-      });
+    state.maybe_init_inspector(&mut isolate);
 
+    let global_state = state.borrow().global_state.clone();
     isolate.set_js_error_create_fn(move |core_js_error| {
       JSError::create(core_js_error, &global_state.ts_compiler)
     });
@@ -128,7 +115,6 @@ impl Worker {
       waker: AtomicWaker::new(),
       internal_channels,
       external_channels,
-      inspector,
     }
   }
 
@@ -163,6 +149,7 @@ impl Worker {
     module_specifier: &ModuleSpecifier,
   ) -> Result<(), ErrBox> {
     let id = self.preload_module(module_specifier).await?;
+    self.wait_for_inspector_session();
     self.isolate.mod_evaluate(id)
   }
 
@@ -177,6 +164,7 @@ impl Worker {
       .isolate
       .load_module(module_specifier, Some(code))
       .await?;
+    self.wait_for_inspector_session();
     self.isolate.mod_evaluate(id)
   }
 
@@ -184,13 +172,29 @@ impl Worker {
   pub fn thread_safe_handle(&self) -> WorkerHandle {
     self.external_channels.clone()
   }
+
+  #[inline(always)]
+  fn inspector(&self) -> RefMut<Option<Box<DenoInspector>>> {
+    let state = self.state.borrow_mut();
+    RefMut::map(state, |s| &mut s.inspector)
+  }
+
+  fn wait_for_inspector_session(&self) {
+    if self.state.should_inspector_break_on_first_statement() {
+      self
+        .inspector()
+        .as_mut()
+        .unwrap()
+        .wait_for_session_and_break_on_next_statement()
+    }
+  }
 }
 
 impl Drop for Worker {
   fn drop(&mut self) {
     // The Isolate object must outlive the Inspector object, but this is
     // currently not enforced by the type system.
-    self.inspector.take();
+    self.inspector().take();
   }
 }
 
@@ -200,10 +204,8 @@ impl Future for Worker {
   fn poll(self: Pin<&mut Self>, cx: &mut Context) -> Poll<Self::Output> {
     let inner = self.get_mut();
 
-    if let Some(deno_inspector) = inner.inspector.as_mut() {
-      // We always poll the inspector if it exists.
-      let _ = deno_inspector.poll_unpin(cx);
-    }
+    // We always poll the inspector if it exists.
+    let _ = inner.inspector().as_mut().map(|i| i.poll_unpin(cx));
     inner.waker.register(cx.waker());
     inner.isolate.poll_unpin(cx)
   }
@@ -293,13 +295,8 @@ 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(),
-      DebugType::Main,
-    )
-    .unwrap();
+    let state =
+      State::new(global_state, None, module_specifier.clone(), false).unwrap();
     let state_ = state.clone();
     tokio_util::run_basic(async move {
       let mut worker =
@@ -327,13 +324,8 @@ 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(),
-      DebugType::Main,
-    )
-    .unwrap();
+    let state =
+      State::new(global_state, None, module_specifier.clone(), false).unwrap();
     let state_ = state.clone();
     tokio_util::run_basic(async move {
       let mut worker =
@@ -370,13 +362,9 @@ mod tests {
       ..flags::Flags::default()
     };
     let global_state = GlobalState::new(flags).unwrap();
-    let state = State::new(
-      global_state.clone(),
-      None,
-      module_specifier.clone(),
-      DebugType::Main,
-    )
-    .unwrap();
+    let state =
+      State::new(global_state.clone(), None, module_specifier.clone(), false)
+        .unwrap();
     let mut worker = MainWorker::new(
       "TEST".to_string(),
       startup_data::deno_isolate_init(),