From aa81a46d58205c5d1006590597ea4a5bb92d825a Mon Sep 17 00:00:00 2001 From: Wez Furlong Date: Fri, 2 Feb 2024 17:41:41 -0700 Subject: [PATCH] mux: allow specifying cache policy for process information This allows individual call sites to be able to force an immediate resolution of the process information. This should help to address https://github.com/wez/wezterm/issues/4811 wherein the problem seems to be that the cwd used for a new spawn is taken from a stale read. --- docs/changelog.md | 2 + lua-api-crates/mux/src/pane.rs | 9 ++-- mux/src/lib.rs | 7 +++- mux/src/localpane.rs | 42 ++++++++++--------- mux/src/pane.rs | 17 ++++++-- mux/src/tab.rs | 4 +- mux/src/termwiztermtab.rs | 5 ++- wezterm-client/src/pane/clientpane.rs | 6 +-- wezterm-gui/src/overlay/copy.rs | 8 ++-- wezterm-gui/src/overlay/quickselect.rs | 7 ++-- wezterm-gui/src/termwindow/mod.rs | 8 ++-- wezterm-mux-server-impl/src/sessionhandler.rs | 4 +- 12 files changed, 71 insertions(+), 48 deletions(-) diff --git a/docs/changelog.md b/docs/changelog.md index 7ceca3dff..aba98fd87 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -42,6 +42,8 @@ As features stabilize some brief notes about them will accumulate here. * Tab bar wouldn't immediately reflect the result of calling `tab:set_title`. #4941 * Command Palette: Missing space between keycaps on macOS. #4885 +* macOS: stale/invalid cwd used when spawning new panes when shell integration + is NOT in use. #4811 ### 20240128-202157-1e552d76 diff --git a/lua-api-crates/mux/src/pane.rs b/lua-api-crates/mux/src/pane.rs index a3334dbe8..c160cb222 100644 --- a/lua-api-crates/mux/src/pane.rs +++ b/lua-api-crates/mux/src/pane.rs @@ -1,6 +1,7 @@ use super::*; use luahelper::{dynamic_to_lua_value, from_lua, to_lua}; use mlua::Value; +use mux::pane::CachePolicy; use std::cmp::Ordering; use std::sync::Arc; use termwiz::cell::SemanticType; @@ -147,7 +148,9 @@ impl UserData for MuxPane { methods.add_method("get_current_working_dir", |_, this, _: ()| { let mux = get_mux()?; let pane = this.resolve(&mux)?; - Ok(pane.get_current_working_dir().map(|url| Url { url })) + Ok(pane + .get_current_working_dir(CachePolicy::FetchImmediate) + .map(|url| Url { url })) }); methods.add_method("get_metadata", |lua, this, _: ()| { @@ -160,13 +163,13 @@ impl UserData for MuxPane { methods.add_method("get_foreground_process_name", |_, this, _: ()| { let mux = get_mux()?; let pane = this.resolve(&mux)?; - Ok(pane.get_foreground_process_name()) + Ok(pane.get_foreground_process_name(CachePolicy::FetchImmediate)) }); methods.add_method("get_foreground_process_info", |_, this, _: ()| { let mux = get_mux()?; let pane = this.resolve(&mux)?; - Ok(pane.get_foreground_process_info()) + Ok(pane.get_foreground_process_info(CachePolicy::AllowStale)) }); methods.add_method("get_cursor_position", |_, this, _: ()| { diff --git a/mux/src/lib.rs b/mux/src/lib.rs index a2eb06fe5..4f6b330f1 100644 --- a/mux/src/lib.rs +++ b/mux/src/lib.rs @@ -1,5 +1,5 @@ use crate::client::{ClientId, ClientInfo}; -use crate::pane::{Pane, PaneId}; +use crate::pane::{CachePolicy, Pane, PaneId}; use crate::tab::{SplitRequest, Tab, TabId}; use crate::window::{Window, WindowId}; use anyhow::{anyhow, Context, Error}; @@ -1131,11 +1131,12 @@ impl Mux { command_dir: Option, pane: Option>, target_domain: DomainId, + policy: CachePolicy, ) -> Option { command_dir.or_else(|| { match pane { Some(pane) if pane.domain_id() == target_domain => pane - .get_current_working_dir() + .get_current_working_dir(policy) .and_then(|url| { percent_decode_str(url.path()) .decode_utf8() @@ -1193,6 +1194,7 @@ impl Mux { command_dir, Some(Arc::clone(¤t_pane)), domain.domain_id(), + CachePolicy::FetchImmediate, ), }, other => other, @@ -1338,6 +1340,7 @@ impl Mux { None => None, }, domain.domain_id(), + CachePolicy::FetchImmediate, ); let tab = domain diff --git a/mux/src/localpane.rs b/mux/src/localpane.rs index 3621f6a25..09f728780 100644 --- a/mux/src/localpane.rs +++ b/mux/src/localpane.rs @@ -1,7 +1,7 @@ use crate::domain::DomainId; use crate::pane::{ - CloseReason, ForEachPaneLogicalLine, LogicalLine, Pane, PaneId, Pattern, SearchResult, - WithPaneLines, + CachePolicy, CloseReason, ForEachPaneLogicalLine, LogicalLine, Pane, PaneId, Pattern, + SearchResult, WithPaneLines, }; use crate::renderable::*; use crate::tmux::{TmuxDomain, TmuxDomainState}; @@ -447,7 +447,7 @@ impl Pane for LocalPane { // If the title is the default pane title, then try to spice // things up a bit by returning the process basename instead if title == "wezterm" { - if let Some(proc_name) = self.get_foreground_process_name() { + if let Some(proc_name) = self.get_foreground_process_name(CachePolicy::AllowStale) { let proc_name = std::path::Path::new(&proc_name); if let Some(name) = proc_name.file_name() { return name.to_string_lossy().to_string(); @@ -501,12 +501,12 @@ impl Pane for LocalPane { } } - fn get_current_working_dir(&self) -> Option { + fn get_current_working_dir(&self, policy: CachePolicy) -> Option { self.terminal .lock() .get_current_dir() .cloned() - .or_else(|| self.divine_current_working_dir()) + .or_else(|| self.divine_current_working_dir(policy)) } fn tty_name(&self) -> Option { @@ -522,19 +522,19 @@ impl Pane for LocalPane { } } - fn get_foreground_process_info(&self) -> Option { + fn get_foreground_process_info(&self, policy: CachePolicy) -> Option { #[cfg(unix)] if let Some(pid) = self.pty.lock().process_group_leader() { return LocalProcessInfo::with_root_pid(pid as u32); } - self.divine_foreground_process() + self.divine_foreground_process(policy) } - fn get_foreground_process_name(&self) -> Option { + fn get_foreground_process_name(&self, policy: CachePolicy) -> Option { #[cfg(unix)] { - let leader = self.get_leader(); + let leader = self.get_leader(policy); if let Some(path) = &leader.path { return Some(path.to_string_lossy().to_string()); } @@ -542,7 +542,7 @@ impl Pane for LocalPane { } #[cfg(windows)] - if let Some(fg) = self.divine_foreground_process() { + if let Some(fg) = self.divine_foreground_process(policy) { return Some(fg.executable.to_string_lossy().to_string()); } @@ -551,7 +551,7 @@ impl Pane for LocalPane { } fn can_close_without_prompting(&self, _reason: CloseReason) -> bool { - if let Some(info) = self.divine_process_list(true) { + if let Some(info) = self.divine_process_list(CachePolicy::FetchImmediate) { log::trace!( "can_close_without_prompting? procs in pane {:#?}", info.root @@ -1015,10 +1015,12 @@ impl LocalPane { } #[cfg(unix)] - fn get_leader(&self) -> CachedLeaderInfo { + fn get_leader(&self, policy: CachePolicy) -> CachedLeaderInfo { let mut leader = self.leader.lock(); - if let Some(info) = leader.as_mut() { + if policy == CachePolicy::FetchImmediate { + leader.replace(CachedLeaderInfo::new(self.pty.lock().as_raw_fd())); + } else if let Some(info) = leader.as_mut() { // If stale, queue up some work in another thread to update. // Right now, we'll return the stale data. if info.expired() && info.can_update() { @@ -1038,10 +1040,10 @@ impl LocalPane { (*leader).clone().unwrap() } - fn divine_current_working_dir(&self) -> Option { + fn divine_current_working_dir(&self, policy: CachePolicy) -> Option { #[cfg(unix)] { - let leader = self.get_leader(); + let leader = self.get_leader(policy); if let Some(path) = &leader.current_working_dir { return Url::parse(&format!("file://localhost{}", path.display())).ok(); } @@ -1049,7 +1051,7 @@ impl LocalPane { } #[cfg(windows)] - if let Some(fg) = self.divine_foreground_process() { + if let Some(fg) = self.divine_foreground_process(policy) { // Since windows paths typically start with something like C:\, // we cannot simply stick `localhost` on the front; we have to // omit the hostname otherwise the url parser is unhappy. @@ -1060,11 +1062,11 @@ impl LocalPane { None } - fn divine_process_list(&self, force_refresh: bool) -> Option> { + fn divine_process_list(&self, policy: CachePolicy) -> Option> { if let ProcessState::Running { pid: Some(pid), .. } = &*self.process.lock() { let mut proc_list = self.proc_list.lock(); - let expired = force_refresh + let expired = policy == CachePolicy::FetchImmediate || proc_list .as_ref() .map(|info| info.updated.elapsed() > PROC_INFO_CACHE_TTL) @@ -1115,8 +1117,8 @@ impl LocalPane { } #[allow(dead_code)] - fn divine_foreground_process(&self) -> Option { - if let Some(info) = self.divine_process_list(false) { + fn divine_foreground_process(&self, policy: CachePolicy) -> Option { + if let Some(info) = self.divine_process_list(policy) { Some(info.foreground.clone()) } else { None diff --git a/mux/src/pane.rs b/mux/src/pane.rs index 98183deae..dcea9ca25 100644 --- a/mux/src/pane.rs +++ b/mux/src/pane.rs @@ -298,11 +298,14 @@ pub trait Pane: Downcast + Send + Sync { None } - fn get_current_working_dir(&self) -> Option; - fn get_foreground_process_name(&self) -> Option { + fn get_current_working_dir(&self, policy: CachePolicy) -> Option; + fn get_foreground_process_name(&self, _policy: CachePolicy) -> Option { None } - fn get_foreground_process_info(&self) -> Option { + fn get_foreground_process_info( + &self, + _policy: CachePolicy, + ) -> Option { None } @@ -316,6 +319,12 @@ pub trait Pane: Downcast + Send + Sync { } impl_downcast!(Pane); +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum CachePolicy { + FetchImmediate, + AllowStale, +} + /// This trait is used to implement/provide a callback that is used together /// with the Pane::with_lines_mut method. /// Ideally we'd simply pass an FnMut with the same signature as the trait @@ -624,7 +633,7 @@ mod test { fn is_alt_screen_active(&self) -> bool { false } - fn get_current_working_dir(&self) -> Option { + fn get_current_working_dir(&self, _policy: CachePolicy) -> Option { None } fn key_down(&self, _: KeyCode, _: KeyModifiers) -> anyhow::Result<()> { diff --git a/mux/src/tab.rs b/mux/src/tab.rs index bb13b86fa..68b3d58ee 100644 --- a/mux/src/tab.rs +++ b/mux/src/tab.rs @@ -254,7 +254,7 @@ fn pane_tree( } Tree::Leaf(pane) => { let dims = pane.get_dimensions(); - let working_dir = pane.get_current_working_dir(); + let working_dir = pane.get_current_working_dir(CachePolicy::AllowStale); let cursor_pos = pane.get_cursor_position(); PaneNode::Leaf(PaneEntry { @@ -2299,7 +2299,7 @@ mod test { fn is_alt_screen_active(&self) -> bool { false } - fn get_current_working_dir(&self) -> Option { + fn get_current_working_dir(&self, _policy: CachePolicy) -> Option { None } } diff --git a/mux/src/termwiztermtab.rs b/mux/src/termwiztermtab.rs index 8e3ab60b2..9118a1536 100644 --- a/mux/src/termwiztermtab.rs +++ b/mux/src/termwiztermtab.rs @@ -5,7 +5,8 @@ use crate::domain::{alloc_domain_id, Domain, DomainId, DomainState}; use crate::pane::{ - alloc_pane_id, CloseReason, ForEachPaneLogicalLine, LogicalLine, Pane, PaneId, WithPaneLines, + alloc_pane_id, CachePolicy, CloseReason, ForEachPaneLogicalLine, LogicalLine, Pane, PaneId, + WithPaneLines, }; use crate::renderable::*; use crate::tab::Tab; @@ -289,7 +290,7 @@ impl Pane for TermWizTerminalPane { self.terminal.lock().is_alt_screen_active() } - fn get_current_working_dir(&self) -> Option { + fn get_current_working_dir(&self, _policy: CachePolicy) -> Option { self.terminal.lock().get_current_dir().cloned() } diff --git a/wezterm-client/src/pane/clientpane.rs b/wezterm-client/src/pane/clientpane.rs index a187620c4..5dbb18346 100644 --- a/wezterm-client/src/pane/clientpane.rs +++ b/wezterm-client/src/pane/clientpane.rs @@ -8,8 +8,8 @@ use config::configuration; use config::keyassignment::ScrollbackEraseMode; use mux::domain::DomainId; use mux::pane::{ - alloc_pane_id, CloseReason, ForEachPaneLogicalLine, LogicalLine, Pane, PaneId, Pattern, - SearchResult, WithPaneLines, + alloc_pane_id, CachePolicy, CloseReason, ForEachPaneLogicalLine, LogicalLine, Pane, PaneId, + Pattern, SearchResult, WithPaneLines, }; use mux::renderable::{RenderableDimensions, StableCursorPosition}; use mux::tab::TabId; @@ -537,7 +537,7 @@ impl Pane for ClientPane { false } - fn get_current_working_dir(&self) -> Option { + fn get_current_working_dir(&self, _policy: CachePolicy) -> Option { self.renderable.lock().inner.borrow().working_dir.clone() } diff --git a/wezterm-gui/src/overlay/copy.rs b/wezterm-gui/src/overlay/copy.rs index 24304f472..c06b0018c 100644 --- a/wezterm-gui/src/overlay/copy.rs +++ b/wezterm-gui/src/overlay/copy.rs @@ -7,8 +7,8 @@ use config::keyassignment::{ }; use mux::domain::DomainId; use mux::pane::{ - ForEachPaneLogicalLine, LogicalLine, Pane, PaneId, Pattern, PerformAssignmentResult, - SearchResult, WithPaneLines, + CachePolicy, ForEachPaneLogicalLine, LogicalLine, Pane, PaneId, Pattern, + PerformAssignmentResult, SearchResult, WithPaneLines, }; use mux::renderable::*; use mux::tab::TabId; @@ -1251,8 +1251,8 @@ impl Pane for CopyOverlay { self.delegate.set_clipboard(clipboard) } - fn get_current_working_dir(&self) -> Option { - self.delegate.get_current_working_dir() + fn get_current_working_dir(&self, policy: CachePolicy) -> Option { + self.delegate.get_current_working_dir(policy) } fn get_cursor_position(&self) -> StableCursorPosition { diff --git a/wezterm-gui/src/overlay/quickselect.rs b/wezterm-gui/src/overlay/quickselect.rs index fc3d16b48..c5b0fcdc6 100644 --- a/wezterm-gui/src/overlay/quickselect.rs +++ b/wezterm-gui/src/overlay/quickselect.rs @@ -4,7 +4,8 @@ use config::keyassignment::{ClipboardCopyDestination, QuickSelectArguments, Scro use config::ConfigHandle; use mux::domain::DomainId; use mux::pane::{ - ForEachPaneLogicalLine, LogicalLine, Pane, PaneId, Pattern, SearchResult, WithPaneLines, + CachePolicy, ForEachPaneLogicalLine, LogicalLine, Pane, PaneId, Pattern, SearchResult, + WithPaneLines, }; use mux::renderable::*; use parking_lot::{MappedMutexGuard, Mutex}; @@ -472,8 +473,8 @@ impl Pane for QuickSelectOverlay { self.delegate.set_clipboard(clipboard) } - fn get_current_working_dir(&self) -> Option { - self.delegate.get_current_working_dir() + fn get_current_working_dir(&self, policy: CachePolicy) -> Option { + self.delegate.get_current_working_dir(policy) } fn get_cursor_position(&self) -> StableCursorPosition { diff --git a/wezterm-gui/src/termwindow/mod.rs b/wezterm-gui/src/termwindow/mod.rs index 16e6236b7..7cb780d4d 100644 --- a/wezterm-gui/src/termwindow/mod.rs +++ b/wezterm-gui/src/termwindow/mod.rs @@ -40,7 +40,9 @@ use config::{ }; use lfucache::*; use mlua::{FromLua, UserData, UserDataFields}; -use mux::pane::{CloseReason, Pane, PaneId, Pattern as MuxPattern, PerformAssignmentResult}; +use mux::pane::{ + CachePolicy, CloseReason, Pane, PaneId, Pattern as MuxPattern, PerformAssignmentResult, +}; use mux::renderable::RenderableDimensions; use mux::tab::{ PositionedPane, PositionedSplit, SplitDirection, SplitRequest, SplitSize as MuxSplitSize, Tab, @@ -284,7 +286,7 @@ impl UserData for PaneInformation { let mut name = None; if let Some(mux) = Mux::try_get() { if let Some(pane) = mux.get_pane(this.pane_id) { - name = pane.get_foreground_process_name(); + name = pane.get_foreground_process_name(CachePolicy::AllowStale); } } match name { @@ -305,7 +307,7 @@ impl UserData for PaneInformation { if let Some(mux) = Mux::try_get() { if let Some(pane) = mux.get_pane(this.pane_id) { return Ok(pane - .get_current_working_dir() + .get_current_working_dir(CachePolicy::AllowStale) .map(|url| url_funcs::Url { url })); } } diff --git a/wezterm-mux-server-impl/src/sessionhandler.rs b/wezterm-mux-server-impl/src/sessionhandler.rs index b9dcc7e1c..405a0166e 100644 --- a/wezterm-mux-server-impl/src/sessionhandler.rs +++ b/wezterm-mux-server-impl/src/sessionhandler.rs @@ -4,7 +4,7 @@ use codec::*; use config::TermConfig; use mux::client::ClientId; use mux::domain::SplitSource; -use mux::pane::{Pane, PaneId}; +use mux::pane::{CachePolicy, Pane, PaneId}; use mux::renderable::{RenderableDimensions, StableCursorPosition}; use mux::tab::TabId; use mux::{Mux, MuxNotification}; @@ -75,7 +75,7 @@ impl PerPane { changed = true; } - let working_dir = pane.get_current_working_dir(); + let working_dir = pane.get_current_working_dir(CachePolicy::AllowStale); if working_dir != self.working_dir { changed = true; }