From 6c0ea88f5b4034b75466776ac65e13d72d5b1183 Mon Sep 17 00:00:00 2001 From: Cole Miller Date: Wed, 11 Jun 2025 01:23:38 -0400 Subject: [PATCH] debugger: Make sure debuggees are killed when quitting Zed (#32186) Closes #31373 We kill the DAP process in our `on_app_quit` handler, but the debuggee might not be killed. Try to make this more reliable by making the DAP process its own process group leader, and killing that entire process group when quitting Zed. I also considered going through the normal DAP shutdown sequence here, but that seems dicey in a quit handler. There's also the DAP `ProcessEvent` but it seems we can't rely on that as e.g. the JS DAP doesn't send it. Release Notes: - Debugger Beta: Fixed debuggee processes not getting cleaned up when quitting Zed. --- Cargo.lock | 1 + crates/dap/Cargo.toml | 3 + crates/dap/src/transport.rs | 116 ++++++++++++++++++++++++++---------- 3 files changed, 88 insertions(+), 32 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a26d4d19c0..9663b5c6c0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4027,6 +4027,7 @@ dependencies = [ "gpui", "http_client", "language", + "libc", "log", "node_runtime", "parking_lot", diff --git a/crates/dap/Cargo.toml b/crates/dap/Cargo.toml index 162c17d6f0..ee963a4f83 100644 --- a/crates/dap/Cargo.toml +++ b/crates/dap/Cargo.toml @@ -51,6 +51,9 @@ telemetry.workspace = true util.workspace = true workspace-hack.workspace = true +[target.'cfg(not(windows))'.dependencies] +libc.workspace = true + [dev-dependencies] async-pipe.workspace = true gpui = { workspace = true, features = ["test-support"] } diff --git a/crates/dap/src/transport.rs b/crates/dap/src/transport.rs index 60f608b9ff..0a73dda789 100644 --- a/crates/dap/src/transport.rs +++ b/crates/dap/src/transport.rs @@ -12,7 +12,6 @@ use smol::{ io::{AsyncBufReadExt as _, AsyncWriteExt, BufReader}, lock::Mutex, net::{TcpListener, TcpStream}, - process::Child, }; use std::{ collections::HashMap, @@ -22,7 +21,7 @@ use std::{ time::Duration, }; use task::TcpArgumentsTemplate; -use util::{ConnectionResult, ResultExt as _}; +use util::ConnectionResult; use crate::{adapters::DebugAdapterBinary, debugger_settings::DebuggerSettings}; @@ -102,7 +101,7 @@ impl Transport { } } - async fn kill(&self) -> Result<()> { + async fn kill(&self) { match self { Transport::Stdio(stdio_transport) => stdio_transport.kill().await, Transport::Tcp(tcp_transport) => tcp_transport.kill().await, @@ -537,7 +536,7 @@ impl TransportDelegate { current_requests.clear(); pending_requests.clear(); - let _ = self.transport.kill().await.log_err(); + self.transport.kill().await; drop(current_requests); drop(pending_requests); @@ -598,8 +597,6 @@ impl TcpTransport { let port = connection_args.port; let mut command = util::command::new_std_command(&binary.command); - util::set_pre_exec_to_start_new_session(&mut command); - let mut command = smol::process::Command::from(command); if let Some(cwd) = &binary.cwd { command.current_dir(cwd); @@ -608,14 +605,7 @@ impl TcpTransport { command.args(&binary.arguments); command.envs(&binary.envs); - command - .stdin(Stdio::null()) - .stdout(Stdio::piped()) - .stderr(Stdio::piped()) - .kill_on_drop(true); - - let mut process = command - .spawn() + let mut process = Child::spawn(command, Stdio::null()) .with_context(|| "failed to start debug adapter.")?; let address = SocketAddrV4::new(host, port); @@ -635,7 +625,7 @@ impl TcpTransport { Ok(stream) => return Ok((process, stream.split())), Err(_) => { if let Ok(Some(_)) = process.try_status() { - let output = process.output().await?; + let output = process.into_inner().output().await?; let output = if output.stderr.is_empty() { String::from_utf8_lossy(&output.stdout).to_string() } else { @@ -679,10 +669,15 @@ impl TcpTransport { true } - async fn kill(&self) -> Result<()> { - self.process.lock().await.kill()?; + async fn kill(&self) { + let mut process = self.process.lock().await; + Child::kill(&mut process); + } +} - Ok(()) +impl Drop for TcpTransport { + fn drop(&mut self) { + self.process.get_mut().kill(); } } @@ -694,8 +689,6 @@ impl StdioTransport { #[allow(dead_code, reason = "This is used in non test builds of Zed")] async fn start(binary: &DebugAdapterBinary, _: AsyncApp) -> Result<(TransportPipe, Self)> { let mut command = util::command::new_std_command(&binary.command); - util::set_pre_exec_to_start_new_session(&mut command); - let mut command = smol::process::Command::from(command); if let Some(cwd) = &binary.cwd { command.current_dir(cwd); @@ -704,13 +697,7 @@ impl StdioTransport { command.args(&binary.arguments); command.envs(&binary.envs); - command - .stdin(Stdio::piped()) - .stdout(Stdio::piped()) - .stderr(Stdio::piped()) - .kill_on_drop(true); - - let mut process = command.spawn().with_context(|| { + let mut process = Child::spawn(command, Stdio::piped()).with_context(|| { format!( "failed to spawn command `{} {}`.", binary.command, @@ -751,9 +738,15 @@ impl StdioTransport { false } - async fn kill(&self) -> Result<()> { - self.process.lock().await.kill()?; - Ok(()) + async fn kill(&self) { + let mut process = self.process.lock().await; + Child::kill(&mut process); + } +} + +impl Drop for StdioTransport { + fn drop(&mut self) { + self.process.get_mut().kill(); } } @@ -927,7 +920,66 @@ impl FakeTransport { false } - async fn kill(&self) -> Result<()> { - Ok(()) + async fn kill(&self) {} +} + +struct Child { + process: smol::process::Child, +} + +impl std::ops::Deref for Child { + type Target = smol::process::Child; + + fn deref(&self) -> &Self::Target { + &self.process + } +} + +impl std::ops::DerefMut for Child { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.process + } +} + +impl Child { + fn into_inner(self) -> smol::process::Child { + self.process + } + + #[cfg(not(windows))] + fn spawn(mut command: std::process::Command, stdin: Stdio) -> Result { + util::set_pre_exec_to_start_new_session(&mut command); + let process = smol::process::Command::from(command) + .stdin(stdin) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn()?; + Ok(Self { process }) + } + + #[cfg(windows)] + fn spawn(command: std::process::Command, stdin: Stdio) -> Result { + // TODO(windows): create a job object and add the child process handle to it, + // see https://learn.microsoft.com/en-us/windows/win32/procthread/job-objects + let process = smol::process::Command::from(command) + .stdin(stdin) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn()?; + Ok(Self { process }) + } + + #[cfg(not(windows))] + fn kill(&mut self) { + let pid = self.process.id(); + unsafe { + libc::killpg(pid as i32, libc::SIGKILL); + } + } + + #[cfg(windows)] + fn kill(&mut self) { + // TODO(windows): terminate the job object in kill + let _ = self.process.kill(); } }