diff --git a/Cargo.lock b/Cargo.lock index 69fe6374d..26c8f63b8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4212,6 +4212,7 @@ dependencies = [ name = "ide-dap" version = "0.1.37" dependencies = [ + "futures", "ide-core", "ide-jsonrpc", "ide-language", diff --git a/crates/ide-dap/Cargo.toml b/crates/ide-dap/Cargo.toml index 9cb2f2a6c..757915cca 100644 --- a/crates/ide-dap/Cargo.toml +++ b/crates/ide-dap/Cargo.toml @@ -6,6 +6,7 @@ edition.workspace = true license.workspace = true [dependencies] +futures.workspace = true tempfile.workspace = true ide-core.workspace = true ide-language.workspace = true diff --git a/crates/ide-dap/src/lib.rs b/crates/ide-dap/src/lib.rs index 8428ac8d1..6971794a2 100644 --- a/crates/ide-dap/src/lib.rs +++ b/crates/ide-dap/src/lib.rs @@ -19,6 +19,7 @@ mod session; use adapter::{AdapterLauncher, AdapterSpec, ProcessLauncher}; use connection::{Connection, DapError}; +use futures::future::join_all; use ide_core::{DebugSessionId, IdeError, IdeResult, SharedSink}; use ide_language::LanguageRegistry; use parking_lot::Mutex; @@ -510,6 +511,18 @@ pub(crate) fn capabilities_from(caps: &Value) -> DebugCapabilities { } } +/// States of a file's line breakpoints that no session has been sent. +fn unsent(breakpoints: &[Breakpoint]) -> Vec { + breakpoints + .iter() + .map(|b| BreakpointState { + line: b.line, + verified: false, + message: (!b.enabled).then(|| "Disabled".to_owned()), + }) + .collect() +} + /// States for breakpoints no session has verified yet. fn unverified(enabled: impl Iterator) -> Vec { enabled @@ -877,12 +890,20 @@ impl DebugManager { let _ = tokio::time::timeout(INIT_TIMEOUT, initialized.wait_for(|v| *v)).await; } - let files: Vec<(PathBuf, Vec)> = - self.breakpoints.lock().iter().map(|(p, b)| (p.clone(), b.clone())).collect(); + // A fresh adapter has no breakpoints, so a file with none enabled needs no request; the + // others go out together, as VS Code sends them, instead of one round trip per file. + let (files, idle): (Vec<_>, Vec<_>) = self + .breakpoints + .lock() + .iter() + .map(|(p, b)| (p.clone(), b.clone())) + .partition(|(_, b)| b.iter().any(|b| b.enabled)); + let sent = join_all(files.iter().map(|(path, bps)| s.send_breakpoints(path, bps, None))).await; // Breakpoint states follow the owner's first session. let first = self.live_sessions().into_iter().find(|f| f.owner.key == s.owner.key).map(|f| f.id) == Some(s.id); - for (path, bps) in files { - match s.send_breakpoints(&path, &bps, None).await { + let idle = idle.into_iter().map(|(path, bps)| (path, Ok(unsent(&bps)))); + for (path, result) in files.into_iter().map(|(path, _)| path).zip(sent).chain(idle) { + match result { Ok(states) => { if first { self.states.lock().insert(path.clone(), states); @@ -990,16 +1011,7 @@ impl DebugManager { Err(e) => tracing::debug!("setBreakpoints on session {} failed: {e}", s.id.0), } } - let states = result.unwrap_or_else(|| { - breakpoints - .iter() - .map(|b| BreakpointState { - line: b.line, - verified: false, - message: (!b.enabled).then(|| "Disabled".to_owned()), - }) - .collect() - }); + let states = result.unwrap_or_else(|| unsent(&breakpoints)); self.states.lock().insert(path, states.clone()); Ok(states) } @@ -1327,13 +1339,26 @@ impl DebugManager { /// Sends empty breakpoint lists (line, function, data) to `session` until its next /// stop, when the stored ones are restored. `except` is left to the caller. async fn suspend_breakpoints(&self, session: &Arc, except: Option<&Path>) -> IdeResult<()> { - let files: Vec = self.breakpoints.lock().keys().cloned().collect(); - for path in files { - if Some(path.as_path()) == except { - continue; + // While paused the adapter holds the stored lists: only files with something enabled change. + let files: Vec = self + .breakpoints + .lock() + .iter() + .filter(|(path, bps)| Some(path.as_path()) != except && bps.iter().any(|b| b.enabled)) + .map(|(path, _)| path.clone()) + .collect(); + let sent = join_all(files.iter().map(|path| session.send_breakpoints(path, &[], None))).await; + let mut failed = None; + for (path, result) in files.into_iter().zip(sent) { + match result { + Ok(_) => { + session.pending_restore.lock().files.insert(path); + } + Err(e) => failed = failed.or(Some(e)), } - session.send_breakpoints(&path, &[], None).await?; - session.pending_restore.lock().files.insert(path); + } + if let Some(e) = failed { + return Err(e.into()); } if !self.function_breakpoints.lock().is_empty() && session.supports("supportsFunctionBreakpoints") { session.send_function_breakpoints(&[]).await?; @@ -1349,9 +1374,11 @@ impl DebugManager { /// Re-sends what [`suspend_breakpoints`](Self::suspend_breakpoints) / run to cursor replaced. pub(crate) async fn restore_breakpoints(&self, session: &Arc) { let pending = std::mem::take(&mut *session.pending_restore.lock()); - for path in pending.files { - let bps = self.stored_breakpoints(&path); - if let Err(e) = session.send_breakpoints(&path, &bps, None).await { + let files: Vec<(PathBuf, Vec)> = + pending.files.into_iter().map(|path| (path.clone(), self.stored_breakpoints(&path))).collect(); + let sent = join_all(files.iter().map(|(path, bps)| session.send_breakpoints(path, bps, None))).await; + for ((path, _), result) in files.iter().zip(sent) { + if let Err(e) = result { tracing::debug!("restoring breakpoints of {} failed: {e}", path.display()); } } diff --git a/crates/ide-dap/src/tests.rs b/crates/ide-dap/src/tests.rs index e86cb6fc9..d82a7e673 100644 --- a/crates/ide-dap/src/tests.rs +++ b/crates/ide-dap/src/tests.rs @@ -17,6 +17,8 @@ struct Script { start_debugging: bool, ignore_configuration_done: bool, source_path: Option, + /// Answer `setBreakpoints` only once this many are waiting (a client that waits for each never gets one). + hold_set_breakpoints: usize, } struct FakeLauncher { @@ -49,6 +51,7 @@ async fn fake_adapter(stream let mut r = BufReader::new(r); let mut seq = 1000; let mut pending_launch: Option = None; + let mut held: Vec = Vec::new(); macro_rules! send { ($v:expr) => {{ let mut v: Value = $v; @@ -101,14 +104,20 @@ async fn fake_adapter(stream send!(json!({"type": "event", "event": "initialized"})); } "setBreakpoints" => { - let bps: Vec = msg["arguments"]["breakpoints"] - .as_array() - .unwrap() - .iter() - .enumerate() - .map(|(i, b)| json!({"id": i + 1, "verified": true, "line": b["line"]})) - .collect(); - ok!(msg, json!({"breakpoints": bps})); + held.push(msg); + if held.len() < script.hold_set_breakpoints { + continue; + } + for msg in std::mem::take(&mut held) { + let bps: Vec = msg["arguments"]["breakpoints"] + .as_array() + .unwrap() + .iter() + .enumerate() + .map(|(i, b)| json!({"id": i + 1, "verified": true, "line": b["line"]})) + .collect(); + ok!(msg, json!({"breakpoints": bps})); + } } "setExceptionBreakpoints" => ok!(msg, json!({})), "setFunctionBreakpoints" | "setDataBreakpoints" => { @@ -921,6 +930,7 @@ async fn force_stepping_suspends_breakpoints_until_the_next_stop() { let b = PathBuf::from("/p/b.py"); h.manager.set_breakpoints(a.clone(), vec![bp(1)]).await.unwrap(); h.manager.set_breakpoints(b.clone(), vec![bp(3)]).await.unwrap(); + h.manager.set_breakpoints(PathBuf::from("/p/off.py"), vec![Breakpoint { enabled: false, ..bp(5) }]).await.unwrap(); h.manager.set_function_breakpoints(vec![function_bp("run", true)]).await.unwrap(); let id = h.start(config()).await.unwrap(); h.wait_event(|e| matches!(e, DebugEvent::Stopped { .. })).await; @@ -949,6 +959,24 @@ async fn force_stepping_suspends_breakpoints_until_the_next_stop() { h.manager.run_to(id, 1, a.clone(), 9, true).await.unwrap(); assert_eq!(sent_for("/p/a.py").last().unwrap(), &json!([{"line": 10}])); assert_eq!(sent_for("/p/b.py").last().unwrap(), &json!([])); + assert!(sent_for("/p/off.py").is_empty(), "nothing enabled there, nothing to suspend or restore"); +} + +#[tokio::test] +async fn startup_sends_every_file_at_once_and_skips_files_with_nothing_enabled() { + let h = harness(Script { hold_set_breakpoints: 2, ..Script::default() }); + h.manager.set_breakpoints(PathBuf::from("/p/a.py"), vec![bp(1)]).await.unwrap(); + h.manager.set_breakpoints(PathBuf::from("/p/b.py"), vec![bp(3)]).await.unwrap(); + let off = PathBuf::from("/p/off.py"); + h.manager.set_breakpoints(off.clone(), vec![Breakpoint { enabled: false, ..bp(5) }]).await.unwrap(); + // Waiting for each answer before the next request would stall until the request timeout. + tokio::time::timeout(Duration::from_secs(5), h.start(config())).await.expect("requests pipelined").unwrap(); + let paths: Vec = requests(&h, "setBreakpoints").into_iter().map(|r| r["source"]["path"].clone()).collect(); + assert_eq!(paths, [json!("/p/a.py"), json!("/p/b.py")]); + assert_eq!( + h.manager.breakpoint_states(&off), + [BreakpointState { line: 5, verified: false, message: Some("Disabled".into()) }] + ); } #[tokio::test] diff --git a/src/features/debugger/__tests__/editor.test.ts b/src/features/debugger/__tests__/editor.test.ts index dd42f3542..b95e467e8 100644 --- a/src/features/debugger/__tests__/editor.test.ts +++ b/src/features/debugger/__tests__/editor.test.ts @@ -3,8 +3,14 @@ import { EditorState } from "@codemirror/state"; import { EditorView, GutterMarker, gutter, lineNumbers } from "@codemirror/view"; import { breakpointGutter, lookOf } from "../editor/breakpointGutter"; import { executionPoint } from "../editor/execution"; -import { breakpointsOf, loadBreakpoints, toggleBreakpoint, updateBreakpoint } from "../breakpoints/store"; -import { adoptSession, useExecutionPoint, useDebug, handleDebugEvent } from "../state"; +import { + breakpointsOf, + loadBreakpoints, + toggleBreakpoint, + updateBreakpoint, + useBreakpoints, +} from "../breakpoints/store"; +import { adoptSession, useExecutionPoint, useDebug, handleDebugEvent, type DebugSession } from "../state"; import { getContext } from "@/platform/context"; import styles from "../editor/editor.module.css"; @@ -53,6 +59,38 @@ describe("breakpoint gutter", () => { }); }); +describe("breakpoint gutter redraws", () => { + it("follows a session starting and ending, not every update of a running one", async () => { + localStorage.clear(); + loadBreakpoints("/proj"); + useDebug.setState({ sessions: {} }); + const view = makeView("a\nb"); + toggleBreakpoint(ctx.path, 0); + const id = breakpointsOf(ctx.path)[0]!.id; + useBreakpoints.setState({ status: { [id]: { verified: true, message: null, line: 0 } } }); + await flush(); + expect(view.dom.querySelector(`.${styles.verified}`)).toBeNull(); + + const session = { id: 9, status: "running" } as DebugSession; + useDebug.setState({ sessions: { 9: session } }); + await flush(); + expect(view.dom.querySelector(`.${styles.verified}`)).not.toBeNull(); + + const reads = vi.spyOn(useBreakpoints, "getState"); + useDebug.setState({ sessions: { 9: { ...session, status: "paused" } } }); + useBreakpoints.setState({ + status: { [id]: useBreakpoints.getState().status[id]!, other: { verified: true, message: null, line: 3 } }, + }); + reads.mockClear(); + await flush(); + expect(reads).not.toHaveBeenCalled(); + + useDebug.setState({ sessions: { 9: { ...session, status: "terminated" } } }); + await flush(); + expect(view.dom.querySelector(`.${styles.verified}`)).toBeNull(); + }); +}); + describe("output class gutter", () => { it("disables ordinary, temporary and logging breakpoints and explains why", () => { loadBreakpoints("/proj"); diff --git a/src/features/debugger/editor/breakpointGutter.ts b/src/features/debugger/editor/breakpointGutter.ts index cb7447501..c094b88dd 100644 --- a/src/features/debugger/editor/breakpointGutter.ts +++ b/src/features/debugger/editor/breakpointGutter.ts @@ -186,14 +186,19 @@ export function breakpointGutter(ctx: EditorDocContext): Extension { private unsubs: Array<() => void>; private pending = false; private destroyed = false; + private running = sessionsRunning(); constructor(readonly view: EditorView) { this.unsubs = [ useBreakpoints.subscribe((s, prev) => { - if (s.byFile[path] !== prev.byFile[path] || s.status !== prev.status || s.muted !== prev.muted) - this.scheduleRebuild(); + // States arrive per file: another file's don't change this one's markers. + const states = (s.byFile[path] ?? []).some((b) => s.status[b.id] !== prev.status[b.id]); + if (s.byFile[path] !== prev.byFile[path] || states || s.muted !== prev.muted) this.scheduleRebuild(); }), + // The markers only show whether a session runs, while each step updates `sessions` several times. useDebug.subscribe((s, prev) => { - if (s.sessions !== prev.sessions) this.scheduleRebuild(); + if (s.sessions === prev.sessions || sessionsRunning() === this.running) return; + this.running = !this.running; + this.scheduleRebuild(); }), useSpecialBreakpoints.subscribe((s, prev) => { if (s.methods !== prev.methods || s.fields !== prev.fields || s.status !== prev.status)