From 3c0a8da4250cf8be831407e449757da015d9e739 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Fri, 2 May 2025 14:45:43 +0300 Subject: [PATCH] Fix tasks not being stopped on reruns (#29786) Follow-up of https://github.com/zed-industries/zed/pull/28993 * Tone down tasks' cancellation logging * Fix task terminals' leak, disallowing to fully cancel the task by dropping the terminal off the pane: https://github.com/zed-industries/zed/blob/f619d5f02af100c34e286af294a42a01dcfb238c/crates/terminal_view/src/terminal_panel.rs#L1464-L1471 Release Notes: - Fixed tasks not being stopped on reruns --- crates/debugger_ui/src/debugger_panel.rs | 7 ++- crates/terminal/src/terminal.rs | 5 +- crates/terminal_view/src/terminal_panel.rs | 54 ++++++++++++---------- crates/vim/src/command.rs | 19 ++++++-- crates/workspace/src/tasks.rs | 28 +++++++---- crates/workspace/src/workspace.rs | 2 +- 6 files changed, 73 insertions(+), 42 deletions(-) diff --git a/crates/debugger_ui/src/debugger_panel.rs b/crates/debugger_ui/src/debugger_panel.rs index 6ddf563f06f01306d7c68fd2daa008ec6c6e3f23..79cadce8a825e2e126fe152ac9d8ec3a09f28f44 100644 --- a/crates/debugger_ui/src/debugger_panel.rs +++ b/crates/debugger_ui/src/debugger_panel.rs @@ -3,7 +3,7 @@ use crate::{ StepInto, StepOut, StepOver, Stop, ToggleIgnoreBreakpoints, persistence, }; use crate::{new_session_modal::NewSessionModal, session::DebugSession}; -use anyhow::{Result, anyhow}; +use anyhow::{Context as _, Result, anyhow}; use collections::HashMap; use command_palette_hooks::CommandPaletteFilter; use dap::StartDebuggingRequestArguments; @@ -547,7 +547,10 @@ impl DebugPanel { cx.background_spawn(async move { match terminal_task { Ok(pid_task) => match pid_task.await { - Ok(Some(pid)) => sender.send(Ok(pid.as_u32())).await?, + Ok(Some(pid)) => sender + .send(Ok(pid.as_u32())) + .await + .context("task cancelled")?, Ok(None) => { sender .send(Err(anyhow!( diff --git a/crates/terminal/src/terminal.rs b/crates/terminal/src/terminal.rs index 3502c7988a7bd6e8a2ebcc5ddb99c7d79efef9a1..8f7981789a95a29fab4641da136ad1da7a8b8684 100644 --- a/crates/terminal/src/terminal.rs +++ b/crates/terminal/src/terminal.rs @@ -46,7 +46,7 @@ use smol::channel::{Receiver, Sender}; use task::{HideStrategy, Shell, TaskId}; use terminal_settings::{AlternateScroll, CursorShape, TerminalSettings}; use theme::{ActiveTheme, Theme}; -use util::{ResultExt, paths::home_dir, truncate_and_trailoff}; +use util::{paths::home_dir, truncate_and_trailoff}; use std::{ cmp::{self, min}, @@ -1858,8 +1858,7 @@ impl Terminal { if let Some(task) = self.task() { if task.status == TaskStatus::Running { let completion_receiver = task.completion_rx.clone(); - return cx - .spawn(async move |_| completion_receiver.recv().await.log_err().flatten()); + return cx.spawn(async move |_| completion_receiver.recv().await.ok().flatten()); } else if let Ok(status) = task.completion_rx.try_recv() { return Task::ready(status); } diff --git a/crates/terminal_view/src/terminal_panel.rs b/crates/terminal_view/src/terminal_panel.rs index ad2a55e705bf95b1fd4929b2fad108eb84f64f4b..e2675fb11c24ad25a7e117bea51be60f177152f5 100644 --- a/crates/terminal_view/src/terminal_panel.rs +++ b/crates/terminal_view/src/terminal_panel.rs @@ -483,7 +483,7 @@ impl TerminalPanel { task: &SpawnInTerminal, window: &mut Window, cx: &mut Context, - ) -> Task>> { + ) -> Task>> { let Ok(is_local) = self .workspace .update(cx, |workspace, cx| workspace.project().read(cx).is_local()) @@ -552,12 +552,12 @@ impl TerminalPanel { cx.spawn(async move |_, _| rx.await?) } - pub fn spawn_in_new_terminal( + fn spawn_in_new_terminal( &mut self, spawn_task: SpawnInTerminal, window: &mut Window, cx: &mut Context, - ) -> Task>> { + ) -> Task>> { let reveal = spawn_task.reveal; let reveal_target = spawn_task.reveal_target; let kind = TerminalKind::Task(spawn_task); @@ -652,7 +652,7 @@ impl TerminalPanel { kind: TerminalKind, window: &mut Window, cx: &mut Context, - ) -> Task>> { + ) -> Task>> { if !is_enabled_in_workspace(workspace, cx) { return Task::ready(Err(anyhow!( "terminal not yet supported for remote projects" @@ -680,7 +680,7 @@ impl TerminalPanel { }); workspace.add_item_to_active_pane(Box::new(terminal_view), None, true, window, cx); })?; - Ok(terminal) + Ok(terminal.downgrade()) }) } @@ -690,7 +690,7 @@ impl TerminalPanel { reveal_strategy: RevealStrategy, window: &mut Window, cx: &mut Context, - ) -> Task>> { + ) -> Task>> { let workspace = self.workspace.clone(); cx.spawn_in(window, async move |terminal_panel, cx| { if workspace.update(cx, |workspace, cx| !is_enabled_in_workspace(workspace, cx))? { @@ -735,11 +735,12 @@ impl TerminalPanel { pane.add_item(terminal_view, true, focus, None, window, cx); }); - Ok(terminal) + Ok(terminal.downgrade()) })?; - terminal_panel.update(cx, |this, cx| { - this.pending_terminals_to_add = this.pending_terminals_to_add.saturating_sub(1); - this.serialize(cx) + terminal_panel.update(cx, |terminal_panel, cx| { + terminal_panel.pending_terminals_to_add = + terminal_panel.pending_terminals_to_add.saturating_sub(1); + terminal_panel.serialize(cx) })?; result }) @@ -802,7 +803,7 @@ impl TerminalPanel { terminal_to_replace: Entity, window: &mut Window, cx: &mut Context, - ) -> Task>> { + ) -> Task>> { let reveal = spawn_task.reveal; let reveal_target = spawn_task.reveal_target; let window_handle = window.window_handle(); @@ -884,7 +885,7 @@ impl TerminalPanel { RevealStrategy::Never => {} } - Ok(new_terminal) + Ok(new_terminal.downgrade()) }) } @@ -1458,22 +1459,25 @@ impl workspace::TerminalProvider for TerminalProvider { task: SpawnInTerminal, window: &mut Window, cx: &mut App, - ) -> Task> { - let this = self.0.clone(); + ) -> Task>> { + let terminal_panel = self.0.clone(); window.spawn(cx, async move |cx| { - let terminal = this + let terminal = terminal_panel .update_in(cx, |terminal_panel, window, cx| { terminal_panel.spawn_task(&task, window, cx) - })? - .await?; - let Some(exit_code) = terminal - .read_with(cx, |terminal, cx| terminal.wait_for_completed_task(cx))? - .await - else { - return Err(anyhow!("Task cancelled")); - }; - - Ok(exit_code) + }) + .ok()? + .await; + match terminal { + Ok(terminal) => { + let exit_status = terminal + .read_with(cx, |terminal, cx| terminal.wait_for_completed_task(cx)) + .ok()? + .await?; + Some(Ok(exit_status)) + } + Err(e) => Some(Err(e)), + } }) } } diff --git a/crates/vim/src/command.rs b/crates/vim/src/command.rs index 0bba323571c15b7ca20c3a58c3cd8cc6c1cf09b9..fad43666abd3a35d47f347454f4cad0e5068f90b 100644 --- a/crates/vim/src/command.rs +++ b/crates/vim/src/command.rs @@ -1466,9 +1466,22 @@ impl ShellExec { show_command: false, show_rerun: false, }; - workspace - .spawn_in_terminal(spawn_in_terminal, window, cx) - .detach_and_log_err(cx); + + let task_status = workspace.spawn_in_terminal(spawn_in_terminal, window, cx); + cx.background_spawn(async move { + match task_status.await { + Some(Ok(status)) => { + if status.success() { + log::debug!("Vim shell exec succeeded"); + } else { + log::debug!("Vim shell exec failed, code: {:?}", status.code()); + } + } + Some(Err(e)) => log::error!("Vim shell exec failed: {e}"), + None => log::debug!("Vim shell exec got cancelled"), + } + }) + .detach(); }); return; }; diff --git a/crates/workspace/src/tasks.rs b/crates/workspace/src/tasks.rs index 64f7d4d6076e8c8b7cec089e7b637f12395e913a..455140d590a9cdf1b10eb168b2dcaad7da9c8068 100644 --- a/crates/workspace/src/tasks.rs +++ b/crates/workspace/src/tasks.rs @@ -1,7 +1,7 @@ use std::process::ExitStatus; -use anyhow::{Result, anyhow}; -use gpui::{Context, Task}; +use anyhow::{Context as _, Result}; +use gpui::{AppContext, Context, Task}; use project::TaskSourceKind; use remote::ConnectionState; use task::{DebugTaskDefinition, ResolvedTask, SpawnInTerminal, TaskContext, TaskTemplate}; @@ -68,9 +68,21 @@ impl Workspace { } if let Some(terminal_provider) = self.terminal_provider.as_ref() { - terminal_provider - .spawn(spawn_in_terminal, window, cx) - .detach_and_log_err(cx); + let task_status = terminal_provider.spawn(spawn_in_terminal, window, cx); + cx.background_spawn(async move { + match task_status.await { + Some(Ok(status)) => { + if status.success() { + log::debug!("Task spawn succeeded"); + } else { + log::debug!("Task spawn failed, code: {:?}", status.code()); + } + } + Some(Err(e)) => log::error!("Task spawn failed: {e}"), + None => log::debug!("Task spawn got cancelled"), + } + }) + .detach(); } } } @@ -93,7 +105,7 @@ impl Workspace { workspace.spawn_in_terminal(task.resolved.unwrap(), window, cx) })?; - let exit_code = task.await?; + let exit_code = task.await.transpose()?.context("task cancelled")?; if !exit_code.success() { return anyhow::Ok(()); } @@ -134,11 +146,11 @@ impl Workspace { spawn_in_terminal: SpawnInTerminal, window: &mut Window, cx: &mut Context, - ) -> Task> { + ) -> Task>> { if let Some(terminal_provider) = self.terminal_provider.as_ref() { terminal_provider.spawn(spawn_in_terminal, window, cx) } else { - Task::ready(Err(anyhow!("No terminal provider"))) + Task::ready(None) } } } diff --git a/crates/workspace/src/workspace.rs b/crates/workspace/src/workspace.rs index fa60d644fa123ddd342d027755b2bb3712846ead..5daea8ddd61bab18bf96b585d634501e96a85283 100644 --- a/crates/workspace/src/workspace.rs +++ b/crates/workspace/src/workspace.rs @@ -136,7 +136,7 @@ pub trait TerminalProvider { task: SpawnInTerminal, window: &mut Window, cx: &mut App, - ) -> Task>; + ) -> Task>>; } pub trait DebuggerProvider {