Fix potential lags/deadlocks during tab close

src/cascadia/TerminalConnection/ConptyConnection.cpp

@@ -554,6 +554,15 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
 
             _hPC.reset(); // tear down the pseudoconsole (this is like clicking X on a console window)
 
+            // CloseHandle() on pipes blocks until any current WriteFile()/ReadFile() has returned.
+            // CancelSynchronousIo prevents us from deadlocking ourselves.
+            // At this point in Close(), _inPipe won't be used anymore since the UI parts are torn down.
+            // _outPipe is probably still stuck in ReadFile() and might currently be written to.
+            if (_hOutputThread)
+            {
+                CancelSynchronousIo(_hOutputThread.get());
+            }
+
             _inPipe.reset(); // break the pipes
             _outPipe.reset();
 
@@ -615,10 +624,17 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
             DWORD read{};
 
             const auto readFail{ !ReadFile(_outPipe.get(), _buffer.data(), gsl::narrow_cast<DWORD>(_buffer.size()), &read, nullptr) };
+
+            // When we call CancelSynchronousIo() in Close() this is the branch that's taken and gets us out of here.
+            if (_isStateAtOrBeyond(ConnectionState::Closing))
+            {
+                return 0;
+            }
+
             if (readFail) // reading failed (we must check this first, because read will also be 0.)
             {
                 const auto lastError = GetLastError();
-                if (lastError != ERROR_BROKEN_PIPE && !_isStateAtOrBeyond(ConnectionState::Closing))
+                if (lastError != ERROR_BROKEN_PIPE)
                 {
                     // EXIT POINT
                     _indicateExitWithStatus(HRESULT_FROM_WIN32(lastError)); // print a message
@@ -631,12 +647,6 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
             const auto result{ til::u8u16(std::string_view{ _buffer.data(), read }, _u16Str, _u8State) };
             if (FAILED(result))
             {
-                if (_isStateAtOrBeyond(ConnectionState::Closing))
-                {
-                    // This termination was expected.
-                    return 0;
-                }
-
                 // EXIT POINT
                 _indicateExitWithStatus(result); // print a message
                 _transitionToState(ConnectionState::Failed);

src/cascadia/TerminalConnection/ConptyConnection.h

@@ -13,7 +13,7 @@
 namespace wil
 {
     // These belong in WIL upstream, so when we reingest the change that has them we'll get rid of ours.
-    using unique_static_pseudoconsole_handle = wil::unique_any<HPCON, decltype(&::ConptyClosePseudoConsole), ::ConptyClosePseudoConsole>;
+    using unique_static_pseudoconsole_handle = wil::unique_any<HPCON, decltype(&::ConptyClosePseudoConsole), ::ConptyClosePseudoConsoleNoWait>;
 }
 
 namespace winrt::Microsoft::Terminal::TerminalConnection::implementation

src/inc/conpty-static.h

@@ -41,4 +41,6 @@ CONPTY_EXPORT HRESULT WINAPI ConptyReparentPseudoConsole(HPCON hPC, HWND newPare
 
 CONPTY_EXPORT VOID WINAPI ConptyClosePseudoConsole(HPCON hPC);
 
+CONPTY_EXPORT VOID WINAPI ConptyClosePseudoConsoleNoWait(HPCON hPC);
+
 CONPTY_EXPORT HRESULT WINAPI ConptyPackPseudoConsole(HANDLE hServerProcess, HANDLE hRef, HANDLE hSignal, HPCON* phPC);

src/interactivity/base/ServiceLocator.cpp

@@ -39,14 +39,29 @@ void ServiceLocator::SetOneCoreTeardownFunction(void (*pfn)()) noexcept
     s_oneCoreTeardownFunction = pfn;
 }
 
-[[noreturn]] void ServiceLocator::RundownAndExit(const HRESULT hr)
+void ServiceLocator::RundownAndExit(const HRESULT hr)
 {
+    static thread_local bool preventRecursion = false;
+    static std::atomic<bool> locked;
+
+    // BODGY:
+    // pRender->TriggerTeardown() might cause another VtEngine pass, which then might fail to write to the IO pipe.
+    // If that happens it calls VtIo::CloseOutput(), which in turn calls ServiceLocator::RundownAndExit().
+    // This prevents the unintended recursion and resulting deadlock.
+    if (std::exchange(preventRecursion, true))
+    {
+        return;
+    }
+
     // MSFT:40146639
     //   The premise of this function is that 1 thread enters and 0 threads leave alive.
     //   We need to prevent anyone from calling us until we actually ExitProcess(),
     //   so that we don't TriggerTeardown() twice. LockConsole() can't be used here,
     //   because doing so would prevent the render thread from progressing.
-    AcquireSRWLockExclusive(&s_shutdownLock);
+    if (locked.exchange(true, std::memory_order_relaxed))
+    {
+        Sleep(INFINITE);
+    }
 
     // MSFT:15506250
     // In VT I/O Mode, a client application might die before we've rendered

src/interactivity/inc/ServiceLocator.hpp

@@ -32,7 +32,7 @@ namespace Microsoft::Console::Interactivity
     public:
         static void SetOneCoreTeardownFunction(void (*pfn)()) noexcept;
 
-        [[noreturn]] static void RundownAndExit(const HRESULT hr);
+        static void RundownAndExit(const HRESULT hr);
 
         // N.B.: Location methods without corresponding creation methods
         //       automatically create the singleton object on demand.
@@ -86,7 +86,6 @@ namespace Microsoft::Console::Interactivity
 
         static HWND LocatePseudoWindow(const HWND owner = nullptr /*HWND_DESKTOP = 0*/);
 
-    protected:
         ServiceLocator(const ServiceLocator&) = delete;
         ServiceLocator& operator=(const ServiceLocator&) = delete;
 
@@ -112,7 +111,5 @@ namespace Microsoft::Console::Interactivity
         static Globals s_globals;
         static bool s_pseudoWindowInitialized;
         static wil::unique_hwnd s_pseudoWindow;
-
-        static inline SRWLOCK s_shutdownLock = SRWLOCK_INIT;
     };
 }

src/winconpty/dll/winconpty.def

@@ -4,6 +4,7 @@ EXPORTS
     ConptyCreatePseudoConsoleAsUser
     ConptyResizePseudoConsole
     ConptyClosePseudoConsole
+    ConptyClosePseudoConsoleNoWait
     ConptyClearPseudoConsole
     ConptyShowHidePseudoConsole
     ConptyReparentPseudoConsole

src/winconpty/ft_pty/ConPtyTests.cpp

@@ -84,10 +84,10 @@ void ConPtyTests::CreateConPtyNoPipes()
     VERIFY_FAILED(_CreatePseudoConsole(defaultSize, nullptr, nullptr, 0, &pcon));
 
     VERIFY_SUCCEEDED(_CreatePseudoConsole(defaultSize, nullptr, goodOut, 0, &pcon));
-    _ClosePseudoConsoleMembers(&pcon);
+    _ClosePseudoConsoleMembers(&pcon, TRUE);
 
     VERIFY_SUCCEEDED(_CreatePseudoConsole(defaultSize, goodIn, nullptr, 0, &pcon));
-    _ClosePseudoConsoleMembers(&pcon);
+    _ClosePseudoConsoleMembers(&pcon, TRUE);
 }
 
 void ConPtyTests::CreateConPtyBadSize()
@@ -131,7 +131,7 @@ void ConPtyTests::GoodCreate()
                              &pcon));
 
     auto closePty = wil::scope_exit([&] {
-        _ClosePseudoConsoleMembers(&pcon);
+        _ClosePseudoConsoleMembers(&pcon, TRUE);
     });
 }
 
@@ -160,7 +160,7 @@ void ConPtyTests::GoodCreateMultiple()
                              0,
                              &pcon1));
     auto closePty1 = wil::scope_exit([&] {
-        _ClosePseudoConsoleMembers(&pcon1);
+        _ClosePseudoConsoleMembers(&pcon1, TRUE);
     });
 
     VERIFY_SUCCEEDED(
@@ -170,7 +170,7 @@ void ConPtyTests::GoodCreateMultiple()
                              0,
                              &pcon2));
     auto closePty2 = wil::scope_exit([&] {
-        _ClosePseudoConsoleMembers(&pcon2);
+        _ClosePseudoConsoleMembers(&pcon2, TRUE);
     });
 }
 
@@ -197,7 +197,7 @@ void ConPtyTests::SurvivesOnBreakInput()
                              0,
                              &pty));
     auto closePty1 = wil::scope_exit([&] {
-        _ClosePseudoConsoleMembers(&pty);
+        _ClosePseudoConsoleMembers(&pty, TRUE);
     });
 
     DWORD dwExit;
@@ -242,7 +242,7 @@ void ConPtyTests::SurvivesOnBreakOutput()
                              0,
                              &pty));
     auto closePty1 = wil::scope_exit([&] {
-        _ClosePseudoConsoleMembers(&pty);
+        _ClosePseudoConsoleMembers(&pty, TRUE);
     });
 
     DWORD dwExit;
@@ -287,7 +287,7 @@ void ConPtyTests::DiesOnBreakBoth()
                              0,
                              &pty));
     auto closePty1 = wil::scope_exit([&] {
-        _ClosePseudoConsoleMembers(&pty);
+        _ClosePseudoConsoleMembers(&pty, TRUE);
     });
 
     DWORD dwExit;
@@ -358,7 +358,7 @@ void ConPtyTests::DiesOnClose()
                              0,
                              &pty));
     auto closePty1 = wil::scope_exit([&] {
-        _ClosePseudoConsoleMembers(&pty);
+        _ClosePseudoConsoleMembers(&pty, TRUE);
     });
 
     DWORD dwExit;
@@ -382,7 +382,7 @@ void ConPtyTests::DiesOnClose()
     Log::Comment(NoThrowString().Format(L"Sleep a bit to let the process attach"));
     Sleep(100);
 
-    _ClosePseudoConsoleMembers(&pty);
+    _ClosePseudoConsoleMembers(&pty, TRUE);
 
     GetExitCodeProcess(hConPtyProcess.get(), &dwExit);
     VERIFY_ARE_NOT_EQUAL(dwExit, (DWORD)STILL_ACTIVE);

src/winconpty/winconpty.cpp

@@ -347,9 +347,10 @@ HRESULT _ReparentPseudoConsole(_In_ const PseudoConsole* const pPty, _In_ const
 //      HPCON via the API).
 // Arguments:
 // - pPty: A pointer to a PseudoConsole struct.
+// - wait: If true, waits for conhost/OpenConsole to exit first.
 // Return Value:
 // - <none>
-void _ClosePseudoConsoleMembers(_In_ PseudoConsole* pPty)
+void _ClosePseudoConsoleMembers(_In_ PseudoConsole* pPty, BOOL wait)
 {
     if (pPty != nullptr)
     {
@@ -365,19 +366,11 @@ void _ClosePseudoConsoleMembers(_In_ PseudoConsole* pPty)
         //      has yet to send before we hard kill it.
         if (_HandleIsValid(pPty->hConPtyProcess))
         {
-            // If the conhost is already dead, then that's fine. Presumably
-            //      it's finished flushing it's output already.
-            DWORD dwExit = 0;
-            // If GetExitCodeProcess failed, it's likely conhost is already dead
-            //      If so, skip waiting regardless of whatever error
-            //      GetExitCodeProcess returned.
-            //      We'll just go straight to killing conhost.
-            if (GetExitCodeProcess(pPty->hConPtyProcess, &dwExit) && dwExit == STILL_ACTIVE)
+            if (wait)
             {
                 WaitForSingleObject(pPty->hConPtyProcess, INFINITE);
             }
 
-            TerminateProcess(pPty->hConPtyProcess, 0);
             CloseHandle(pPty->hConPtyProcess);
             pPty->hConPtyProcess = nullptr;
         }
@@ -398,13 +391,14 @@ void _ClosePseudoConsoleMembers(_In_ PseudoConsole* pPty)
 //      PseudoConsoles that were created with CreatePseudoConsole.
 // Arguments:
 // - pPty: A pointer to a PseudoConsole struct.
+// - wait: If true, waits for conhost/OpenConsole to exit first.
 // Return Value:
 // - <none>
-VOID _ClosePseudoConsole(_In_ PseudoConsole* pPty)
+static void _ClosePseudoConsole(_In_ PseudoConsole* pPty, BOOL wait) noexcept
 {
     if (pPty != nullptr)
     {
-        _ClosePseudoConsoleMembers(pPty);
+        _ClosePseudoConsoleMembers(pPty, wait);
         HeapFree(GetProcessHeap(), 0, pPty);
     }
 }
@@ -465,7 +459,7 @@ extern "C" HRESULT ConptyCreatePseudoConsoleAsUser(_In_ HANDLE hToken,
     auto pPty = (PseudoConsole*)HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(PseudoConsole));
     RETURN_IF_NULL_ALLOC(pPty);
     auto cleanupPty = wil::scope_exit([&]() noexcept {
-        _ClosePseudoConsole(pPty);
+        _ClosePseudoConsole(pPty, TRUE);
     });
 
     wil::unique_handle duplicatedInput;
@@ -544,13 +538,22 @@ extern "C" HRESULT WINAPI ConptyReparentPseudoConsole(_In_ HPCON hPC, HWND newPa
 //      console window they were running in was closed.
 // This can fail if the conhost hosting the pseudoconsole failed to be
 //      terminated, or if the pseudoconsole was already terminated.
+// Waits for conhost/OpenConsole to exit first.
 extern "C" VOID WINAPI ConptyClosePseudoConsole(_In_ HPCON hPC)
 {
-    const auto pPty = (PseudoConsole*)hPC;
-    if (pPty != nullptr)
-    {
-        _ClosePseudoConsole(pPty);
-    }
+    _ClosePseudoConsole((PseudoConsole*)hPC, TRUE);
+}
+
+// Function Description:
+// Closes the conpty and all associated state.
+// Client applications attached to the conpty will also behave as though the
+//      console window they were running in was closed.
+// This can fail if the conhost hosting the pseudoconsole failed to be
+//      terminated, or if the pseudoconsole was already terminated.
+// Doesn't wait for conhost/OpenConsole to exit.
+extern "C" VOID WINAPI ConptyClosePseudoConsoleNoWait(_In_ HPCON hPC)
+{
+    _ClosePseudoConsole((PseudoConsole*)hPC, FALSE);
 }
 
 // NOTE: This one is not defined in the Windows headers but is

src/winconpty/winconpty.h

@@ -41,8 +41,7 @@ HRESULT _ResizePseudoConsole(_In_ const PseudoConsole* const pPty, _In_ const CO
 HRESULT _ClearPseudoConsole(_In_ const PseudoConsole* const pPty);
 HRESULT _ShowHidePseudoConsole(_In_ const PseudoConsole* const pPty, const bool show);
 HRESULT _ReparentPseudoConsole(_In_ const PseudoConsole* const pPty, _In_ const HWND newParent);
-void _ClosePseudoConsoleMembers(_In_ PseudoConsole* pPty);
-VOID _ClosePseudoConsole(_In_ PseudoConsole* pPty);
+void _ClosePseudoConsoleMembers(_In_ PseudoConsole* pPty, BOOL wait);
 
 HRESULT ConptyCreatePseudoConsoleAsUser(_In_ HANDLE hToken,
                                         _In_ COORD size,