From a712c3a21e07d522e7514c9980e610e79e216c5e Mon Sep 17 00:00:00 2001 From: Dustin Howett Date: Tue, 8 Sep 2020 16:44:12 -0700 Subject: [PATCH] Destruct ConptyConnection on a background thread This commit leverages C++/WinRT's final_release [extension point] to pull the final destruction of ConptyConnection off onto a background thread. We've been seeing some deadlocks during teardown where the output thread (holding the last owning reference to the connection) was trying to destruct the threadpool wait while the threadpool wait was simultaneously running its callback and waiting for the output thread to terminate. It turns out that trying to release a threadpool wait while it's running a callback that's blocked on you will absolutely result in a deadlock. Fixes #7392. [extension point]: https://devblogs.microsoft.com/oldnewthing/20191018-00 --- .../TerminalConnection/ConptyConnection.cpp | 19 +++++++++++++++++++ .../TerminalConnection/ConptyConnection.h | 1 + 2 files changed, 20 insertions(+) diff --git a/src/cascadia/TerminalConnection/ConptyConnection.cpp b/src/cascadia/TerminalConnection/ConptyConnection.cpp index ab1c9ab5c55..676d1f905fc 100644 --- a/src/cascadia/TerminalConnection/ConptyConnection.cpp +++ b/src/cascadia/TerminalConnection/ConptyConnection.cpp @@ -455,4 +455,23 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation return 0; } + // Function Description: + // - This function will be called (by C++/WinRT) after the final outstanding reference to + // any given connection instance is released. + // When a client application exits, its termination will wait for the output thread to + // run down. However, because our teardown is somewhat complex, our last reference may + // be owned by the very output thread that the client wait threadpool is blocked on. + // During destruction, we'll try to release any outstanding handles--including the one + // we have to the threadpool wait. As you might imagine, this takes us right to deadlock + // city. + // Deferring the final destruction of the connection to a background thread that can't + // be awaiting our destruction breaks the deadlock. + // Arguments: + // - connection: the final living reference to an outgoing connection + winrt::fire_and_forget ConptyConnection::final_release(std::unique_ptr connection) + { + co_await winrt::resume_background(); // move to background + connection.reset(); // explicitly destruct + } + } diff --git a/src/cascadia/TerminalConnection/ConptyConnection.h b/src/cascadia/TerminalConnection/ConptyConnection.h index b3ca0827aa8..557fb4969e0 100644 --- a/src/cascadia/TerminalConnection/ConptyConnection.h +++ b/src/cascadia/TerminalConnection/ConptyConnection.h @@ -27,6 +27,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation const uint32_t rows, const uint32_t cols, const guid& guid); + static winrt::fire_and_forget final_release(std::unique_ptr connection); void Start(); void WriteInput(hstring const& data);