From 6deb274b8283b1d5afe0666d21d6093c89a0835d Mon Sep 17 00:00:00 2001
From: David Hewitt <1939362+davidhewitt@users.noreply.github.com>
Date: Thu, 12 Dec 2019 15:01:23 +0000
Subject: [PATCH] Fix deadlock when closing on Windows using Conpty

Fixes #3042.
---
 CHANGELOG.md                                 |  1 +
 alacritty/src/main.rs                        | 13 +++++++++++++
 alacritty_terminal/src/tty/windows/conpty.rs |  4 ++++
 alacritty_terminal/src/tty/windows/mod.rs    |  2 ++
 4 files changed, 20 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 3bd9f84d..10b341fc 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -27,6 +27,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Minimize on windows causing layout issues
 - Performance bottleneck when clearing colored rows
 - Vague startup crash messages on Windows with WinPTY backend
+- Deadlock on Windows when closing Alacritty using the title bar "X" button (ConPTY backend)
 
 ## 0.4.0
 
diff --git a/alacritty/src/main.rs b/alacritty/src/main.rs
index 871c816e..9b5b8eb8 100644
--- a/alacritty/src/main.rs
+++ b/alacritty/src/main.rs
@@ -222,6 +222,19 @@ fn run(window_event_loop: GlutinEventLoop<Event>, config: Config) -> Result<(),
     // Start event loop and block until shutdown
     processor.run(terminal, window_event_loop);
 
+    // This explicit drop is needed for Windows, ConPTY backend. Otherwise a deadlock can occur.
+    // The cause:
+    //   - Drop for Conpty will deadlock if the conout pipe has already been dropped.
+    //   - The conout pipe is dropped when the io_thread is joined below (io_thread owns pty).
+    //   - Conpty is dropped when the last of processor and io_thread are dropped, because both of
+    //     them own an Arc<Conpty>.
+    //
+    // The fix is to ensure that processor is dropped first. That way, when io_thread (i.e. pty)
+    // is dropped, it can ensure Conpty is dropped before the conout pipe in the pty drop order.
+    //
+    // FIXME: Change PTY API to enforce the correct drop order with the typesystem.
+    drop(processor);
+
     // Shutdown PTY parser event loop
     loop_tx.send(Msg::Shutdown).expect("Error sending shutdown to pty event loop");
     io_thread.join().expect("join io thread");
diff --git a/alacritty_terminal/src/tty/windows/conpty.rs b/alacritty_terminal/src/tty/windows/conpty.rs
index 31f4c252..a1c7edfd 100644
--- a/alacritty_terminal/src/tty/windows/conpty.rs
+++ b/alacritty_terminal/src/tty/windows/conpty.rs
@@ -90,6 +90,10 @@ pub type ConptyHandle = Arc<Conpty>;
 
 impl Drop for Conpty {
     fn drop(&mut self) {
+        // XXX: This will block until the conout pipe is drained. Will cause a deadlock if the
+        // conout pipe has already been dropped by this point.
+        //
+        // See PR #3084 and https://docs.microsoft.com/en-us/windows/console/closepseudoconsole
         unsafe { (self.api.ClosePseudoConsole)(self.handle) }
     }
 }
diff --git a/alacritty_terminal/src/tty/windows/mod.rs b/alacritty_terminal/src/tty/windows/mod.rs
index c9c0be73..e112d305 100644
--- a/alacritty_terminal/src/tty/windows/mod.rs
+++ b/alacritty_terminal/src/tty/windows/mod.rs
@@ -45,6 +45,8 @@ pub enum PtyHandle {
 }
 
 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,
     // TODO: It's on the roadmap for the Conpty API to support Overlapped I/O.
     // See https://github.com/Microsoft/console/issues/262