From 6be3f0e7cadb25763f2840eadefb5ebb991fd8e8 Mon Sep 17 00:00:00 2001 From: Wez Furlong Date: Thu, 6 Jul 2023 07:16:19 -0700 Subject: [PATCH] ssh: rewrite exit_behavior=Close -> CloseOnCleanExit during connection ssh connection or host authentication errors would not be displayed when the config was in its default exit_behavior=Close state. This commit introduces some plumbing to allow a pane to override the effective exit_behavior value, so that ssh sessions can now rewrite it to CloseOnCleanExit while they are in the process of connecting. refs: #3941 --- docs/changelog.md | 4 ++++ mux/src/lib.rs | 6 +++--- mux/src/localpane.rs | 43 ++++++++++++++++++++++++++++++++++++------- mux/src/pane.rs | 5 +++++ mux/src/ssh.rs | 24 +++++++++++++++++++----- pty/src/cmdbuilder.rs | 2 +- pty/src/lib.rs | 9 ++++++--- 7 files changed, 74 insertions(+), 19 deletions(-) diff --git a/docs/changelog.md b/docs/changelog.md index a54547f0527..be7255dcbf7 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -107,6 +107,10 @@ As features stabilize some brief notes about them will accumulate here. * Pasting large amounts of text in helix caused issues. #3683 * Wayland: Copying to clipboard was not always successful when triggered by the keyboard. Thanks to @osandov! #3929 +* `wezterm ssh` connection errors were not shown; the window would close + immediately with the default `exit_behavior` setting. Now ssh sessions change + `exit_behavior="Close"` to `exit_behavior="CloseOnCleanExit"` so that error + information can be displayed. #3941 #### Updated * Bundled harfbuzz to 7.3.0 diff --git a/mux/src/lib.rs b/mux/src/lib.rs index 333165144c0..52f93546de9 100644 --- a/mux/src/lib.rs +++ b/mux/src/lib.rs @@ -280,8 +280,8 @@ fn read_from_pane_pty( // or in the main mux thread. If `true`, this thread will terminate. let dead = Arc::new(AtomicBool::new(false)); - let pane_id = match pane.upgrade() { - Some(pane) => pane.pane_id(), + let (pane_id, exit_behavior) = match pane.upgrade() { + Some(pane) => (pane.pane_id(), pane.exit_behavior()), None => return, }; @@ -333,7 +333,7 @@ fn read_from_pane_pty( } } - match configuration().exit_behavior { + match exit_behavior.unwrap_or_else(|| configuration().exit_behavior) { ExitBehavior::Hold | ExitBehavior::CloseOnCleanExit => { // We don't know if we can unilaterally close // this pane right now, so don't! diff --git a/mux/src/localpane.rs b/mux/src/localpane.rs index 51f6c3e0319..0eb7ef13f81 100644 --- a/mux/src/localpane.rs +++ b/mux/src/localpane.rs @@ -42,7 +42,7 @@ enum ProcessState { Running { child_waiter: Receiver>, pid: Option, - signaller: Box, + signaller: Box, // Whether we've explicitly killed the child killed: bool, }, @@ -115,11 +115,17 @@ impl CachedLeaderInfo { } } +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +pub enum LocalPaneConnectionState { + Connecting, + Connected, +} + pub struct LocalPane { pane_id: PaneId, terminal: Mutex, process: Mutex, - pty: Mutex>, + pty: Mutex>, writer: Mutex>, domain_id: DomainId, tmux_domain: Mutex>>, @@ -213,6 +219,24 @@ impl Pane for LocalPane { self.terminal.lock().user_vars().clone() } + fn exit_behavior(&self) -> Option { + // If we are ssh, and we've not yet fully connected, + // then override exit_behavior so that we can show + // connection issues + let is_ssh_connecting = self + .pty + .lock() + .downcast_mut::() + .map(|s| s.is_connecting()) + .unwrap_or(false); + + if is_ssh_connecting { + Some(ExitBehavior::CloseOnCleanExit) + } else { + None + } + } + fn kill(&self) { let mut proc = self.process.lock(); log::debug!( @@ -263,9 +287,14 @@ impl Pane for LocalPane { .contains(&status.exit_code()), }; - match (configuration().exit_behavior, success, killed) { + match ( + self.exit_behavior() + .unwrap_or_else(|| configuration().exit_behavior), + success, + killed, + ) { (ExitBehavior::Close, _, _) => *proc = ProcessState::Dead, - (ExitBehavior::CloseOnCleanExit, false, false) => { + (ExitBehavior::CloseOnCleanExit, false, _) => { notify = Some(format!( "\r\n⚠️ Process {} didn't exit cleanly\r\n{}.\r\n{}=\"CloseOnCleanExit\"\r\n", self.command_description, @@ -901,10 +930,10 @@ impl AlertHandler for LocalPaneNotifHandler { /// Without this, typing `exit` in `cmd.exe` would keep the pane around /// until something else triggered the mux to prune dead processes. fn split_child( - mut process: Box, + mut process: Box, ) -> ( Receiver>, - Box, + Box, Option, ) { let pid = process.process_id(); @@ -930,7 +959,7 @@ impl LocalPane { pane_id: PaneId, mut terminal: Terminal, process: Box, - pty: Box, + pty: Box, writer: Box, domain_id: DomainId, command_description: String, diff --git a/mux/src/pane.rs b/mux/src/pane.rs index f79360b68f3..98183deaebb 100644 --- a/mux/src/pane.rs +++ b/mux/src/pane.rs @@ -1,5 +1,6 @@ use crate::domain::DomainId; use crate::renderable::*; +use crate::ExitBehavior; use async_trait::async_trait; use config::keyassignment::{KeyAssignment, ScrollbackEraseMode}; use downcast_rs::{impl_downcast, Downcast}; @@ -308,6 +309,10 @@ pub trait Pane: Downcast + Send + Sync { fn tty_name(&self) -> Option { None } + + fn exit_behavior(&self) -> Option { + None + } } impl_downcast!(Pane); diff --git a/mux/src/ssh.rs b/mux/src/ssh.rs index 3c1be80da92..5d0b0ae59e2 100644 --- a/mux/src/ssh.rs +++ b/mux/src/ssh.rs @@ -775,7 +775,7 @@ struct WrappedSshChildKiller { } #[derive(Debug)] -struct WrappedSshChild { +pub(crate) struct WrappedSshChild { status: Option>, rx: Receiver, exited: Option, @@ -791,7 +791,7 @@ impl WrappedSshChild { } Err(TryRecvError::Empty) => {} Err(err) => { - log::error!("WrappedSshChild err: {:#?}", err); + log::debug!("WrappedSshChild::check_connected err: {:#?}", err); self.exited.replace(ExitStatus::with_exit_code(1)); } } @@ -836,7 +836,7 @@ impl portable_pty::Child for WrappedSshChild { } Err(smol::channel::TryRecvError::Empty) => Ok(None), Err(err) => { - log::error!("WrappedSshChild err: {:#?}", err); + log::debug!("WrappedSshChild::try_wait err: {:#?}", err); let status = ExitStatus::with_exit_code(1); self.exited.replace(status.clone()); Ok(Some(status)) @@ -858,7 +858,7 @@ impl portable_pty::Child for WrappedSshChild { self.got_child(c); } Err(err) => { - log::error!("WrappedSshChild err: {:#?}", err); + log::debug!("WrappedSshChild err: {:#?}", err); let status = ExitStatus::with_exit_code(1); self.exited.replace(status.clone()); return Ok(status); @@ -926,10 +926,16 @@ impl ChildKiller for WrappedSshChildKiller { type BoxedReader = Box<(dyn Read + Send + 'static)>; type BoxedWriter = Box<(dyn Write + Send + 'static)>; -struct WrappedSshPty { +pub(crate) struct WrappedSshPty { inner: RefCell, } +impl WrappedSshPty { + pub fn is_connecting(&mut self) -> bool { + self.inner.borrow_mut().is_connecting() + } +} + enum WrappedSshPtyInner { Connecting { reader: Option, @@ -975,6 +981,14 @@ impl WrappedSshPtyInner { _ => Ok(()), } } + + fn is_connecting(&mut self) -> bool { + self.check_connected().ok(); + match self { + Self::Connecting { .. } => true, + Self::Connected { .. } => false, + } + } } impl portable_pty::MasterPty for WrappedSshPty { diff --git a/pty/src/cmdbuilder.rs b/pty/src/cmdbuilder.rs index ec207099ecc..2c8c4fc514b 100644 --- a/pty/src/cmdbuilder.rs +++ b/pty/src/cmdbuilder.rs @@ -204,7 +204,7 @@ pub struct CommandBuilder { } impl CommandBuilder { - /// Create a new builder instance with argv[0] set to the specified + /// Create a new builder instance with argv\[0\] set to the specified /// program. pub fn new>(program: S) -> Self { Self { diff --git a/pty/src/lib.rs b/pty/src/lib.rs index 61934c6f403..a67bd36a4d2 100644 --- a/pty/src/lib.rs +++ b/pty/src/lib.rs @@ -85,7 +85,7 @@ impl Default for PtySize { } /// Represents the master/control end of the pty -pub trait MasterPty { +pub trait MasterPty: Downcast + Send { /// Inform the kernel and thus the child process that the window resized. /// It will update the winsize information maintained by the kernel, /// and generate a signal for the child to notice and update its state. @@ -123,10 +123,11 @@ pub trait MasterPty { None } } +impl_downcast!(MasterPty); /// Represents a child process spawned into the pty. /// This handle can be used to wait for or terminate that child process. -pub trait Child: std::fmt::Debug + ChildKiller { +pub trait Child: std::fmt::Debug + ChildKiller + Downcast + Send { /// Poll the child to see if it has completed. /// Does not block. /// Returns None if the child has not yet terminated, @@ -143,9 +144,10 @@ pub trait Child: std::fmt::Debug + ChildKiller { #[cfg(windows)] fn as_raw_handle(&self) -> Option; } +impl_downcast!(Child); /// Represents the ability to signal a Child to terminate -pub trait ChildKiller: std::fmt::Debug { +pub trait ChildKiller: std::fmt::Debug + Downcast + Send { /// Terminate the child process fn kill(&mut self) -> IoResult<()>; @@ -154,6 +156,7 @@ pub trait ChildKiller: std::fmt::Debug { /// blocked in `.wait`. fn clone_killer(&self) -> Box; } +impl_downcast!(ChildKiller); /// Represents the slave side of a pty. /// Can be used to spawn processes into the pty.