From 6f0f245dd0f0a0f27756fa931f8cd5618104c3cc Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 9 Apr 2020 18:27:56 -0500 Subject: [PATCH] Don't wait for a connection to finish when a control is closed (#5303) ## Summary of the Pull Request When a pane is closed by a connection, we want to wait until the connection is actually `Closed` before we fire the actual `Closed` event. If the connection didn't close gracefully, there are scenarios where we want to print a message to the screen. However, when a pane is closed by the UI, we don't really care to wait for the connection to be completely closed. We can just do it whenever. So I've moved that call to be on a background thread. ## PR Checklist * [x] Closes #1996 * [x] I work here * [ ] Tests added/passed * [n/a] Requires documentation to be updated ## Detailed Description of the Pull Request / Additional comments Previously we'd wait for the connection to close synchronously when closing tabs or panes. For misbehaving applications like `ssh.exe`, that could result in the `Close` needing to `WaitForSingleObject` _on the UI thread_. If the user closed the tab / pane either with a keybinding or with some other UI element, they don't really care to see the error message anymore. They just want the pane closed. So there's no need to wait for the actual connection to close - the app can just continue on with whatever it was doing. ## Validation Steps Performed Messed around with closing tabs, panes, tabs with many panes, the entire window. Did this with keybindings, or by clicking on the 'x' on the tab, the 'x' on the window, or using middle-click. I'm always scared of things like this, so there's a 50% chance this makes things horribly worse. --- src/cascadia/TerminalControl/TermControl.cpp | 31 ++++++++++++++++---- src/cascadia/TerminalControl/TermControl.h | 2 ++ 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 98617e41b70..9bfac7f1463 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -1970,6 +1970,26 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation _clipboardPasteHandlers(*this, *pasteArgs); } + // Method Description: + // - Asynchronously close our connection. The Connection will likely wait + // until the attached process terminates before Close returns. If that's + // the case, we don't want to block the UI thread waiting on that process + // handle. + // Arguments: + // - + // Return Value: + // - + winrt::fire_and_forget TermControl::_AsyncCloseConnection() + { + if (auto localConnection{ std::exchange(_connection, nullptr) }) + { + // Close the connection on the background thread. + co_await winrt::resume_background(); + localConnection.Close(); + // connection is destroyed. + } + } + void TermControl::Close() { if (!_closing.exchange(true)) @@ -1981,11 +2001,12 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation TSFInputControl().Close(); // Disconnect the TSF input control so it doesn't receive EditContext events. _autoScrollTimer.Stop(); - if (auto localConnection{ std::exchange(_connection, nullptr) }) - { - localConnection.Close(); - // connection is destroyed. - } + // GH#1996 - Close the connection asynchronously on a background + // thread. + // Since TermControl::Close is only ever triggered by the UI, we + // don't really care to wait for the connection to be completely + // closed. We can just do it whenever. + _AsyncCloseConnection(); if (auto localRenderEngine{ std::exchange(_renderEngine, nullptr) }) { diff --git a/src/cascadia/TerminalControl/TermControl.h b/src/cascadia/TerminalControl/TermControl.h index d3082965557..ee8aa6d7ddd 100644 --- a/src/cascadia/TerminalControl/TermControl.h +++ b/src/cascadia/TerminalControl/TermControl.h @@ -232,6 +232,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation void _CurrentCursorPositionHandler(const IInspectable& sender, const CursorPositionEventArgs& eventArgs); void _FontInfoHandler(const IInspectable& sender, const FontInfoEventArgs& eventArgs); + winrt::fire_and_forget _AsyncCloseConnection(); + // this atomic is to be used as a guard against dispatching billions of coroutines for // routine state changes that might happen millions of times a second. // Unbounded main dispatcher use leads to massive memory leaks and intense slowdowns