From 08a122574880d299181c59bec186c0f9e8bef77c Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Sat, 14 Dec 2019 21:32:24 +0000 Subject: [PATCH] Send PTY resize messages through event loop This allows us to clean up the Arcs on windows, as well as tidy up the code on unix a little too. Fixes #3086. --- alacritty/src/event.rs | 7 +-- alacritty/src/main.rs | 23 +-------- alacritty_terminal/src/event_loop.rs | 21 +++++--- alacritty_terminal/src/tty/unix.rs | 10 ++-- alacritty_terminal/src/tty/windows/conpty.rs | 13 ++--- alacritty_terminal/src/tty/windows/mod.rs | 40 ++++++--------- alacritty_terminal/src/tty/windows/winpty.rs | 54 +++----------------- 7 files changed, 50 insertions(+), 118 deletions(-) diff --git a/alacritty/src/event.rs b/alacritty/src/event.rs index 4cfc215..4b1fe89 100644 --- a/alacritty/src/event.rs +++ b/alacritty/src/event.rs @@ -308,20 +308,18 @@ pub struct Processor { suppress_chars: bool, modifiers: ModifiersState, config: Config, - pty_resize_handle: Box, message_buffer: MessageBuffer, display: Display, font_size: Size, } -impl Processor { +impl Processor { /// Create a new event processor /// /// Takes a writer which is expected to be hooked up to the write end of a /// pty. pub fn new( notifier: N, - pty_resize_handle: Box, message_buffer: MessageBuffer, config: Config, display: Display, @@ -334,7 +332,6 @@ impl Processor { modifiers: Default::default(), font_size: config.font.size, config, - pty_resize_handle, message_buffer, display, } @@ -405,7 +402,7 @@ impl Processor { if !display_update_pending.is_empty() { self.display.handle_update( &mut terminal, - self.pty_resize_handle.as_mut(), + &mut self.notifier, &self.message_buffer, &self.config, display_update_pending, diff --git a/alacritty/src/main.rs b/alacritty/src/main.rs index 9b5b8eb..64e0b9c 100644 --- a/alacritty/src/main.rs +++ b/alacritty/src/main.rs @@ -27,8 +27,6 @@ use std::env; use std::error::Error; use std::fs; use std::io::{self, Write}; -#[cfg(not(windows))] -use std::os::unix::io::AsRawFd; use std::sync::Arc; #[cfg(target_os = "macos")] @@ -170,16 +168,6 @@ fn run(window_event_loop: GlutinEventLoop, config: Config) -> Result<(), #[cfg(any(target_os = "macos", windows))] let pty = tty::new(&config, &display.size_info, None); - // Create PTY resize handle - // - // This exists because rust doesn't know the interface is thread-safe - // and we need to be able to resize the PTY from the main thread while the IO - // thread owns the EventedRW object. - #[cfg(windows)] - let resize_handle = pty.resize_handle(); - #[cfg(not(windows))] - let resize_handle = pty.fd.as_raw_fd(); - // Create the pseudoterminal I/O loop // // pty I/O is ran on another thread as to not occupy cycles used by the @@ -204,15 +192,8 @@ fn run(window_event_loop: GlutinEventLoop, config: Config) -> Result<(), let message_buffer = MessageBuffer::new(); // Event processor - // - // Need the Rc> here since a ref is shared in the resize callback - let mut processor = Processor::new( - event_loop::Notifier(loop_tx.clone()), - Box::new(resize_handle), - message_buffer, - config, - display, - ); + let mut processor = + Processor::new(event_loop::Notifier(loop_tx.clone()), message_buffer, config, display); // Kick off the I/O thread let io_thread = event_loop.spawn(); diff --git a/alacritty_terminal/src/event_loop.rs b/alacritty_terminal/src/event_loop.rs index 00f77c9..2d55815 100644 --- a/alacritty_terminal/src/event_loop.rs +++ b/alacritty_terminal/src/event_loop.rs @@ -16,7 +16,7 @@ use crate::ansi; use crate::config::Config; use crate::event::{self, Event, EventListener}; use crate::sync::FairMutex; -use crate::term::Term; +use crate::term::{SizeInfo, Term}; use crate::tty; use crate::util::thread; @@ -31,6 +31,9 @@ pub enum Msg { /// Indicates that the `EventLoop` should shut down, as Alacritty is shutting down Shutdown, + + /// Instruction to resize the pty + Resize(SizeInfo), } /// The main event!.. loop. @@ -76,9 +79,14 @@ impl event::Notify for Notifier { if bytes.len() == 0 { return; } - if self.0.send(Msg::Input(bytes)).is_err() { - panic!("expected send event loop msg"); - } + + self.0.send(Msg::Input(bytes)).expect("send event loop msg"); + } +} + +impl event::OnResize for Notifier { + fn on_resize(&mut self, size: &SizeInfo) { + self.0.send(Msg::Resize(*size)).expect("expected send event loop msg"); } } @@ -141,7 +149,7 @@ impl Writing { impl EventLoop where - T: tty::EventedPty + Send + 'static, + T: tty::EventedPty + event::OnResize + Send + 'static, U: EventListener + Send + 'static, { /// Create a new event loop @@ -171,11 +179,12 @@ where // Drain the channel // // Returns `false` when a shutdown message was received. - fn drain_recv_channel(&self, state: &mut State) -> bool { + fn drain_recv_channel(&mut self, state: &mut State) -> bool { while let Ok(msg) = self.rx.try_recv() { match msg { Msg::Input(input) => state.write_list.push_back(input), Msg::Shutdown => return false, + Msg::Resize(size) => self.pty.on_resize(&size), } } diff --git a/alacritty_terminal/src/tty/unix.rs b/alacritty_terminal/src/tty/unix.rs index 01ee1b5..9419ead 100644 --- a/alacritty_terminal/src/tty/unix.rs +++ b/alacritty_terminal/src/tty/unix.rs @@ -135,7 +135,7 @@ fn get_pw_entry(buf: &mut [i8; 1024]) -> Passwd<'_> { pub struct Pty { child: Child, - pub fd: File, + fd: File, token: mio::Token, signals: Signals, signals_token: mio::Token, @@ -226,14 +226,14 @@ pub fn new(config: &Config, size: &SizeInfo, window_id: Option) -> set_nonblocking(master); } - let pty = Pty { + let mut pty = Pty { child, fd: unsafe { File::from_raw_fd(master) }, token: mio::Token::from(0), signals, signals_token: mio::Token::from(0), }; - pty.fd.as_raw_fd().on_resize(size); + pty.on_resize(size); pty }, Err(err) => die!("Failed to spawn command '{}': {}", shell.program, err), @@ -350,7 +350,7 @@ impl<'a> ToWinsize for &'a SizeInfo { } } -impl OnResize for i32 { +impl OnResize for Pty { /// Resize the pty /// /// Tells the kernel that the window size changed with the new pixel @@ -358,7 +358,7 @@ impl OnResize for i32 { fn on_resize(&mut self, size: &SizeInfo) { let win = size.to_winsize(); - let res = unsafe { libc::ioctl(*self, libc::TIOCSWINSZ, &win as *const _) }; + let res = unsafe { libc::ioctl(self.fd.as_raw_fd(), libc::TIOCSWINSZ, &win as *const _) }; if res < 0 { die!("ioctl TIOCSWINSZ failed: {}", io::Error::last_os_error()); diff --git a/alacritty_terminal/src/tty/windows/conpty.rs b/alacritty_terminal/src/tty/windows/conpty.rs index a1c7edf..d01f67b 100644 --- a/alacritty_terminal/src/tty/windows/conpty.rs +++ b/alacritty_terminal/src/tty/windows/conpty.rs @@ -17,7 +17,6 @@ use std::io::Error; use std::mem; use std::os::windows::io::IntoRawHandle; use std::ptr; -use std::sync::Arc; use dunce::canonicalize; use mio_anonymous_pipes::{EventedAnonRead, EventedAnonWrite}; @@ -85,9 +84,6 @@ pub struct Conpty { api: ConptyApi, } -/// Handle can be cloned freely and moved between threads. -pub type ConptyHandle = Arc; - impl Drop for Conpty { fn drop(&mut self) { // XXX: This will block until the conout pipe is drained. Will cause a deadlock if the @@ -98,9 +94,8 @@ impl Drop for Conpty { } } -// The Conpty API can be accessed from multiple threads. +// The Conpty handle can be sent between threads. unsafe impl Send for Conpty {} -unsafe impl Sync for Conpty {} pub fn new(config: &Config, size: &SizeInfo, _window_id: Option) -> Option { if !config.enable_experimental_conpty_backend { @@ -244,10 +239,10 @@ pub fn new(config: &Config, size: &SizeInfo, _window_id: Option) -> let conout = EventedAnonRead::new(conout); let child_watcher = ChildExitWatcher::new(proc_info.hProcess).unwrap(); - let agent = Conpty { handle: pty_handle, api }; + let conpty = Conpty { handle: pty_handle, api }; Some(Pty { - handle: super::PtyHandle::Conpty(ConptyHandle::new(agent)), + backend: super::PtyBackend::Conpty(conpty), conout: super::EventedReadablePipe::Anonymous(conout), conin: super::EventedWritablePipe::Anonymous(conin), read_token: 0.into(), @@ -262,7 +257,7 @@ fn panic_shell_spawn() { panic!("Unable to spawn shell: {}", Error::last_os_error()); } -impl OnResize for ConptyHandle { +impl OnResize for Conpty { fn on_resize(&mut self, sizeinfo: &SizeInfo) { if let Some(coord) = coord_from_sizeinfo(sizeinfo) { let result = unsafe { (self.api.ResizePseudoConsole)(self.handle, coord) }; diff --git a/alacritty_terminal/src/tty/windows/mod.rs b/alacritty_terminal/src/tty/windows/mod.rs index e112d30..9e2f722 100644 --- a/alacritty_terminal/src/tty/windows/mod.rs +++ b/alacritty_terminal/src/tty/windows/mod.rs @@ -38,16 +38,15 @@ pub fn is_conpty() -> bool { IS_CONPTY.load(Ordering::Relaxed) } -#[derive(Clone)] -pub enum PtyHandle { - Winpty(winpty::WinptyHandle), - Conpty(conpty::ConptyHandle), +enum PtyBackend { + Winpty(winpty::Agent), + Conpty(conpty::Conpty), } pub struct Pty { - // XXX: Handle is required to be the first field, to ensure correct drop order. Dropping - // `conout` before `handle` will cause a deadlock. - handle: PtyHandle, + // XXX: Backend is required to be the first field, to ensure correct drop order. Dropping + // `conout` before `backend` will cause a deadlock. + backend: PtyBackend, // TODO: It's on the roadmap for the Conpty API to support Overlapped I/O. // See https://github.com/Microsoft/console/issues/262 // When support for that lands then it should be possible to use @@ -60,12 +59,6 @@ pub struct Pty { child_watcher: ChildExitWatcher, } -impl Pty { - pub fn resize_handle(&self) -> impl OnResize { - self.handle.clone() - } -} - pub fn new(config: &Config, size: &SizeInfo, window_id: Option) -> Pty { if let Some(pty) = conpty::new(config, size, window_id) { info!("Using Conpty agent"); @@ -189,18 +182,6 @@ impl Write for EventedWritablePipe { } } -impl OnResize for PtyHandle { - fn on_resize(&mut self, sizeinfo: &SizeInfo) { - match self { - PtyHandle::Winpty(w) => w.resize(sizeinfo), - PtyHandle::Conpty(c) => { - let mut handle = c.clone(); - handle.on_resize(sizeinfo) - }, - } - } -} - impl EventedReadWrite for Pty { type Reader = EventedReadablePipe; type Writer = EventedWritablePipe; @@ -308,3 +289,12 @@ impl EventedPty for Pty { } } } + +impl OnResize for Pty { + fn on_resize(&mut self, size: &SizeInfo) { + match &mut self.backend { + PtyBackend::Winpty(w) => w.on_resize(size), + PtyBackend::Conpty(c) => c.on_resize(size), + } + } +} diff --git a/alacritty_terminal/src/tty/windows/winpty.rs b/alacritty_terminal/src/tty/windows/winpty.rs index 032c339..56ee34a 100644 --- a/alacritty_terminal/src/tty/windows/winpty.rs +++ b/alacritty_terminal/src/tty/windows/winpty.rs @@ -16,7 +16,6 @@ use std::fs::OpenOptions; use std::io; use std::os::windows::fs::OpenOptionsExt; use std::os::windows::io::{FromRawHandle, IntoRawHandle}; -use std::sync::Arc; use std::u16; use dunce::canonicalize; @@ -31,45 +30,7 @@ use crate::term::SizeInfo; use crate::tty::windows::child::ChildExitWatcher; use crate::tty::windows::Pty; -// We store a raw pointer because we need mutable access to call -// on_resize from a separate thread. Winpty internally uses a mutex -// so this is safe, despite outwards appearance. -pub struct Agent { - winpty: *mut Winpty, -} - -/// Handle can be cloned freely and moved between threads. -pub type WinptyHandle = Arc; - -// Because Winpty has a mutex, we can do this. -unsafe impl Send for Agent {} -unsafe impl Sync for Agent {} - -impl Agent { - pub fn new(winpty: Winpty) -> Self { - Self { winpty: Box::into_raw(Box::new(winpty)) } - } - - /// Get immutable access to Winpty. - pub fn winpty(&self) -> &Winpty { - unsafe { &*self.winpty } - } - - pub fn resize(&self, size: &SizeInfo) { - // This is safe since Winpty uses a mutex internally. - unsafe { - (&mut *self.winpty).on_resize(size); - } - } -} - -impl Drop for Agent { - fn drop(&mut self) { - unsafe { - Box::from_raw(self.winpty); - } - } -} +pub use winpty::Winpty as Agent; /// How long the winpty agent should wait for any RPC request /// This is a placeholder value until we see how often long responses happen @@ -84,8 +45,8 @@ pub fn new(config: &Config, size: &SizeInfo, _window_id: Option) -> wconfig.set_agent_timeout(AGENT_TIMEOUT); // Start agent - let mut winpty = Winpty::open(&wconfig).unwrap(); - let (conin, conout) = (winpty.conin_name(), winpty.conout_name()); + let mut agent = Winpty::open(&wconfig).unwrap(); + let (conin, conout) = (agent.conin_name(), agent.conout_name()); // Get process commandline let default_shell = &Shell::new("powershell"); @@ -134,13 +95,12 @@ pub fn new(config: &Config, size: &SizeInfo, _window_id: Option) -> } assert!(conin_pipe.take_error().unwrap().is_none()); - winpty.spawn(&spawnconfig).unwrap(); + agent.spawn(&spawnconfig).unwrap(); - let child_watcher = ChildExitWatcher::new(winpty.raw_handle()).unwrap(); - let agent = Agent::new(winpty); + let child_watcher = ChildExitWatcher::new(agent.raw_handle()).unwrap(); Pty { - handle: super::PtyHandle::Winpty(WinptyHandle::new(agent)), + backend: super::PtyBackend::Winpty(agent), conout: super::EventedReadablePipe::Named(conout_pipe), conin: super::EventedWritablePipe::Named(conin_pipe), read_token: 0.into(), @@ -150,7 +110,7 @@ pub fn new(config: &Config, size: &SizeInfo, _window_id: Option) -> } } -impl OnResize for Winpty { +impl OnResize for Agent { fn on_resize(&mut self, sizeinfo: &SizeInfo) { let (cols, lines) = (sizeinfo.cols().0, sizeinfo.lines().0); if cols > 0 && cols <= u16::MAX as usize && lines > 0 && lines <= u16::MAX as usize {