From 48861e463311145a653350688dc4bad83a528d91 Mon Sep 17 00:00:00 2001 From: Maciej Makowski Date: Sat, 16 Nov 2019 21:11:56 +0000 Subject: [PATCH] Fix WinPTY freeze on termination Fixes #2889. --- alacritty/src/event.rs | 7 +- alacritty_terminal/src/event_loop.rs | 7 +- alacritty_terminal/src/tty/mod.rs | 4 +- alacritty_terminal/src/tty/unix.rs | 4 - alacritty_terminal/src/tty/windows/child.rs | 115 +++++++++++++++++++ alacritty_terminal/src/tty/windows/conpty.rs | 13 +-- alacritty_terminal/src/tty/windows/mod.rs | 64 ++++++----- alacritty_terminal/src/tty/windows/winpty.rs | 11 +- 8 files changed, 171 insertions(+), 54 deletions(-) create mode 100644 alacritty_terminal/src/tty/windows/child.rs diff --git a/alacritty/src/event.rs b/alacritty/src/event.rs index 1fc1ee42..2c171e23 100644 --- a/alacritty/src/event.rs +++ b/alacritty/src/event.rs @@ -30,6 +30,7 @@ use alacritty_terminal::selection::Selection; use alacritty_terminal::sync::FairMutex; use alacritty_terminal::term::cell::Cell; use alacritty_terminal::term::{SizeInfo, Term}; +#[cfg(not(windows))] use alacritty_terminal::tty; use alacritty_terminal::util::{limit, start_daemon}; @@ -351,14 +352,14 @@ impl Processor { info!("glutin event: {:?}", event); } - match (&event, tty::process_should_exit()) { + match &event { // Check for shutdown - (GlutinEvent::UserEvent(Event::Exit), _) | (_, true) => { + GlutinEvent::UserEvent(Event::Exit) => { *control_flow = ControlFlow::Exit; return; }, // Process events - (GlutinEvent::EventsCleared, _) => { + GlutinEvent::EventsCleared => { *control_flow = ControlFlow::Wait; if event_queue.is_empty() { diff --git a/alacritty_terminal/src/event_loop.rs b/alacritty_terminal/src/event_loop.rs index 3e762840..00f77c9f 100644 --- a/alacritty_terminal/src/event_loop.rs +++ b/alacritty_terminal/src/event_loop.rs @@ -250,8 +250,10 @@ where } } - // Queue terminal redraw - self.event_proxy.send_event(Event::Wakeup); + if processed > 0 { + // Queue terminal redraw + self.event_proxy.send_event(Event::Wakeup); + } Ok(()) } @@ -327,7 +329,6 @@ where } }, - #[cfg(unix)] token if token == self.pty.child_event_token() => { if let Some(tty::ChildEvent::Exited) = self.pty.next_child_event() { if !self.hold { diff --git a/alacritty_terminal/src/tty/mod.rs b/alacritty_terminal/src/tty/mod.rs index 7150a42d..40d019f5 100644 --- a/alacritty_terminal/src/tty/mod.rs +++ b/alacritty_terminal/src/tty/mod.rs @@ -54,7 +54,7 @@ pub trait EventedReadWrite { } /// Events concerning TTY child processes -#[derive(PartialEq)] +#[derive(Debug, PartialEq)] pub enum ChildEvent { /// Indicates the child has exited Exited, @@ -66,13 +66,11 @@ pub enum ChildEvent { /// notified if the PTY child process does something we care about (other than writing to the TTY). /// In particular, this allows for race-free child exit notification on UNIX (cf. `SIGCHLD`). pub trait EventedPty: EventedReadWrite { - #[cfg(unix)] fn child_event_token(&self) -> mio::Token; /// Tries to retrieve an event /// /// Returns `Some(event)` on success, or `None` if there are no events to retrieve. - #[cfg(unix)] fn next_child_event(&mut self) -> Option; } diff --git a/alacritty_terminal/src/tty/unix.rs b/alacritty_terminal/src/tty/unix.rs index b820d754..01ee1b59 100644 --- a/alacritty_terminal/src/tty/unix.rs +++ b/alacritty_terminal/src/tty/unix.rs @@ -333,10 +333,6 @@ impl EventedPty for Pty { } } -pub fn process_should_exit() -> bool { - false -} - /// Types that can produce a `libc::winsize` pub trait ToWinsize { /// Get a `libc::winsize` diff --git a/alacritty_terminal/src/tty/windows/child.rs b/alacritty_terminal/src/tty/windows/child.rs new file mode 100644 index 00000000..447b7fbf --- /dev/null +++ b/alacritty_terminal/src/tty/windows/child.rs @@ -0,0 +1,115 @@ +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use std::ffi::c_void; +use std::io::Error; +use std::sync::atomic::{AtomicPtr, Ordering}; + +use mio_extras::channel::{channel, Receiver, Sender}; + +use winapi::shared::ntdef::{BOOLEAN, HANDLE, PVOID}; +use winapi::um::winbase::{RegisterWaitForSingleObject, UnregisterWait, INFINITE}; +use winapi::um::winnt::{WT_EXECUTEINWAITTHREAD, WT_EXECUTEONLYONCE}; + +use crate::tty::ChildEvent; + +/// WinAPI callback to run when child process exits. +extern "system" fn child_exit_callback(ctx: PVOID, timed_out: BOOLEAN) { + if timed_out != 0 { + return; + } + + let event_tx: Box<_> = unsafe { Box::from_raw(ctx as *mut Sender) }; + let _ = event_tx.send(ChildEvent::Exited); +} + +pub struct ChildExitWatcher { + wait_handle: AtomicPtr, + event_rx: Receiver, +} + +impl ChildExitWatcher { + pub fn new(child_handle: HANDLE) -> Result { + let (event_tx, event_rx) = channel::(); + + let mut wait_handle: HANDLE = 0 as HANDLE; + let sender_ref = Box::new(event_tx); + + let success = unsafe { + RegisterWaitForSingleObject( + &mut wait_handle, + child_handle, + Some(child_exit_callback), + Box::into_raw(sender_ref) as PVOID, + INFINITE, + WT_EXECUTEINWAITTHREAD | WT_EXECUTEONLYONCE, + ) + }; + + if success == 0 { + Err(Error::last_os_error()) + } else { + Ok(ChildExitWatcher { wait_handle: AtomicPtr::from(wait_handle), event_rx }) + } + } + + pub fn event_rx(&self) -> &Receiver { + &self.event_rx + } +} + +impl Drop for ChildExitWatcher { + fn drop(&mut self) { + unsafe { + UnregisterWait(self.wait_handle.load(Ordering::Relaxed)); + } + } +} + +#[cfg(test)] +mod test { + use std::os::windows::io::AsRawHandle; + use std::process::Command; + use std::time::Duration; + + use mio::{Events, Poll, PollOpt, Ready, Token}; + + use super::*; + + #[test] + pub fn event_is_emitted_when_child_exits() { + const WAIT_TIMEOUT: Duration = Duration::from_millis(200); + + let mut child = Command::new("cmd.exe").spawn().unwrap(); + let child_exit_watcher = ChildExitWatcher::new(child.as_raw_handle()).unwrap(); + + let mut events = Events::with_capacity(1); + let poll = Poll::new().unwrap(); + let child_events_token = Token::from(0usize); + + poll.register( + child_exit_watcher.event_rx(), + child_events_token, + Ready::readable(), + PollOpt::oneshot(), + ) + .unwrap(); + + child.kill().unwrap(); + + // Poll for the event or fail with timeout if nothing has been sent + poll.poll(&mut events, Some(WAIT_TIMEOUT)).unwrap(); + assert_eq!(events.iter().next().unwrap().token(), child_events_token); + // Verify that at least one `ChildEvent::Exited` was received + assert_eq!(child_exit_watcher.event_rx().try_recv(), Ok(ChildEvent::Exited)); + } +} diff --git a/alacritty_terminal/src/tty/windows/conpty.rs b/alacritty_terminal/src/tty/windows/conpty.rs index 185acfc2..f34ad922 100644 --- a/alacritty_terminal/src/tty/windows/conpty.rs +++ b/alacritty_terminal/src/tty/windows/conpty.rs @@ -12,8 +12,6 @@ // See the License for the specific language governing permissions and // limitations under the License. -use super::{Pty, HANDLE}; - use std::i16; use std::io::Error; use std::mem; @@ -22,7 +20,6 @@ use std::ptr; use std::sync::Arc; use dunce::canonicalize; -use log::info; use mio_anonymous_pipes::{EventedAnonRead, EventedAnonWrite}; use miow; use widestring::U16CString; @@ -41,6 +38,8 @@ use winapi::um::wincontypes::{COORD, HPCON}; use crate::config::{Config, Shell}; use crate::event::OnResize; use crate::term::SizeInfo; +use crate::tty::windows::child::ChildExitWatcher; +use crate::tty::windows::Pty; /// Dynamically-loaded Pseudoconsole API from kernel32.dll /// @@ -241,14 +240,10 @@ pub fn new<'a, C>( } } - // Store handle to console - unsafe { - HANDLE = proc_info.hProcess; - } - let conin = EventedAnonWrite::new(conin); let conout = EventedAnonRead::new(conout); + let child_watcher = ChildExitWatcher::new(proc_info.hProcess).unwrap(); let agent = Conpty { handle: pty_handle, api }; Some(Pty { @@ -257,6 +252,8 @@ pub fn new<'a, C>( conin: super::EventedWritablePipe::Anonymous(conin), read_token: 0.into(), write_token: 0.into(), + child_event_token: 0.into(), + child_watcher, }) } diff --git a/alacritty_terminal/src/tty/windows/mod.rs b/alacritty_terminal/src/tty/windows/mod.rs index 922dde26..436b5edf 100644 --- a/alacritty_terminal/src/tty/windows/mod.rs +++ b/alacritty_terminal/src/tty/windows/mod.rs @@ -13,49 +13,27 @@ // limitations under the License. use std::io::{self, Read, Write}; -use std::os::raw::c_void; use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::mpsc::TryRecvError; use mio::{self, Evented, Poll, PollOpt, Ready, Token}; use mio_anonymous_pipes::{EventedAnonRead, EventedAnonWrite}; use mio_named_pipes::NamedPipe; use log::info; -use winapi::shared::winerror::WAIT_TIMEOUT; -use winapi::um::synchapi::WaitForSingleObject; -use winapi::um::winbase::WAIT_OBJECT_0; use crate::config::Config; use crate::event::OnResize; use crate::term::SizeInfo; -use crate::tty::{EventedPty, EventedReadWrite}; +use crate::tty::windows::child::ChildExitWatcher; +use crate::tty::{ChildEvent, EventedPty, EventedReadWrite}; +mod child; mod conpty; mod winpty; -/// Handle to the winpty agent or conpty process. Required so we know when it closes. -static mut HANDLE: *mut c_void = 0usize as *mut c_void; static IS_CONPTY: AtomicBool = AtomicBool::new(false); -pub fn process_should_exit() -> bool { - unsafe { - match WaitForSingleObject(HANDLE, 0) { - // Process has exited - WAIT_OBJECT_0 => { - info!("wait_object_0"); - true - }, - // Reached timeout of 0, process has not exited - WAIT_TIMEOUT => false, - // Error checking process, winpty gave us a bad agent handle? - _ => { - info!("Bad exit: {}", ::std::io::Error::last_os_error()); - true - }, - } - } -} - pub fn is_conpty() -> bool { IS_CONPTY.load(Ordering::Relaxed) } @@ -76,6 +54,8 @@ pub struct Pty<'a> { conin: EventedWritablePipe, read_token: mio::Token, write_token: mio::Token, + child_event_token: mio::Token, + child_watcher: ChildExitWatcher, } impl<'a> Pty<'a> { @@ -244,6 +224,15 @@ impl<'a> EventedReadWrite for Pty<'a> { } else { poll.register(&self.conin, self.write_token, mio::Ready::empty(), poll_opts)? } + + self.child_event_token = token.next().unwrap(); + poll.register( + self.child_watcher.event_rx(), + self.child_event_token, + mio::Ready::readable(), + poll_opts, + )?; + Ok(()) } @@ -264,6 +253,14 @@ impl<'a> EventedReadWrite for Pty<'a> { } else { poll.reregister(&self.conin, self.write_token, mio::Ready::empty(), poll_opts)?; } + + poll.reregister( + self.child_watcher.event_rx(), + self.child_event_token, + mio::Ready::readable(), + poll_opts, + )?; + Ok(()) } @@ -271,6 +268,7 @@ impl<'a> EventedReadWrite for Pty<'a> { fn deregister(&mut self, poll: &mio::Poll) -> io::Result<()> { poll.deregister(&self.conout)?; poll.deregister(&self.conin)?; + poll.deregister(self.child_watcher.event_rx())?; Ok(()) } @@ -295,4 +293,16 @@ impl<'a> EventedReadWrite for Pty<'a> { } } -impl<'a> EventedPty for Pty<'a> {} +impl<'a> EventedPty for Pty<'a> { + fn child_event_token(&self) -> mio::Token { + self.child_event_token + } + + fn next_child_event(&mut self) -> Option { + match self.child_watcher.event_rx().try_recv() { + Ok(ev) => Some(ev), + Err(TryRecvError::Empty) => None, + Err(TryRecvError::Disconnected) => Some(ChildEvent::Exited), + } + } +} diff --git a/alacritty_terminal/src/tty/windows/winpty.rs b/alacritty_terminal/src/tty/windows/winpty.rs index db397ad9..258b7b2d 100644 --- a/alacritty_terminal/src/tty/windows/winpty.rs +++ b/alacritty_terminal/src/tty/windows/winpty.rs @@ -12,8 +12,6 @@ // See the License for the specific language governing permissions and // limitations under the License. -use super::{Pty, HANDLE}; - use std::fs::OpenOptions; use std::io; use std::os::windows::fs::OpenOptionsExt; @@ -30,6 +28,8 @@ use winpty::{Config as WinptyConfig, ConfigFlags, MouseMode, SpawnConfig, SpawnF use crate::config::{Config, Shell}; use crate::event::OnResize; 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 @@ -136,10 +136,7 @@ pub fn new<'a, C>(config: &Config, size: &SizeInfo, _window_id: Option winpty.spawn(&spawnconfig).unwrap(); - unsafe { - HANDLE = winpty.raw_handle(); - } - + let child_watcher = ChildExitWatcher::new(winpty.raw_handle()).unwrap(); let agent = Agent::new(winpty); Pty { @@ -148,6 +145,8 @@ pub fn new<'a, C>(config: &Config, size: &SizeInfo, _window_id: Option conin: super::EventedWritablePipe::Named(conin_pipe), read_token: 0.into(), write_token: 0.into(), + child_event_token: 0.into(), + child_watcher, } }