Wait for clients to exit on ConPTY shutdown (#14282)

where (on my system) 9 out of 10 times everything works correctly,
but sometimes OpenConsole exits, while pwsh and bash keep running.

My leading theory is that the new code is exiting OpenConsole faster than the
old code. This prevents clients from calling console APIs, etc. causing them
to get stuck. The old code (and new code) calls `ExitProcess` when the ConPTY
pipes break and I think this is wrong: In conhost when you close the window we
only call `CloseConsoleProcessState` via the `WM_CLOSE` event and that's it.
Solution: Remove the call to `RundownAndExit` for ConPTY.

During testing I found that continuously printing text inside msys2 will cause
child processes to only exit slowly one by one every 5 seconds.
This happens because `CloseConsoleProcessState` calls `HandleCtrlEvent` without
holding the console lock. This creates a race condition where most of the time
the console IO thread is the one picking up the `CTRL_CLOSE_EVENT`. But that's
problematic because the `CTRL_CLOSE_EVENT` leads to a `ConsoleControl` call of
type `ConsoleEndTask` which calls back into conhost's IO thread and
so you got the IO thread waiting on itself to respond.
Solution: Don't race conditions.

* `Enter-VsDevShell` and close the tab
  Everything exits after 5s ✅
* Run msys2 bash from within pwsh and close the tab
  Everything exits instantly ✅
* Run `cat bigfile.txt` and close the tab
  Everything exits instantly ✅
* Patch `conhost.exe` with `sfpcopy`, as well as `KernelBase.dll`
  with the recent changes to `winconpty`, then launch and exit
  shells and applications via VS Code's terminal ✅
* On the main branch without this modification remove the call to
  `TriggerTeardown` in `RundownAndExit` (this speeds up the shutdown).
  Run (msys2's) `bash.exe --login` and hold enter and then press Ctrl+Shift+W
  simultaneously. The tab should close and randomly OpenConsole should exit
  early while pwsh/bash keep running. Then retry this with this branch and
  observe how the child processes don't stick around forever anymore. ✅

(cherry picked from commit b6b1ff8)

src/host/PtySignalInputThread.cpp

@@ -96,17 +96,30 @@ void PtySignalInputThread::CreatePseudoWindow()
 // Return Value:
 // - S_OK if the thread runs to completion.
 // - Otherwise it may cause an application termination and never return.
-[[nodiscard]] HRESULT PtySignalInputThread::_InputThread()
+[[nodiscard]] HRESULT PtySignalInputThread::_InputThread() noexcept
+try
 {
-    PtySignal signalId;
-    while (_GetData(&signalId, sizeof(signalId)))
+    const auto shutdown = wil::scope_exit([this]() {
+        _Shutdown();
+    });
+
+    for (;;)
     {
+        PtySignal signalId;
+        if (!_GetData(&signalId, sizeof(signalId)))
+        {
+            return S_OK;
+        }
+
         switch (signalId)
         {
         case PtySignal::ShowHideWindow:
         {
             ShowHideData msg = { 0 };
-            _GetData(&msg, sizeof(msg));
+            if (!_GetData(&msg, sizeof(msg)))
+            {
+                return S_OK;
+            }
 
             LockConsole();
             auto Unlock = wil::scope_exit([&] { UnlockConsole(); });
@@ -148,7 +161,10 @@ void PtySignalInputThread::CreatePseudoWindow()
         case PtySignal::ResizeWindow:
         {
             ResizeWindowData resizeMsg = { 0 };
-            _GetData(&resizeMsg, sizeof(resizeMsg));
+            if (!_GetData(&resizeMsg, sizeof(resizeMsg)))
+            {
+                return S_OK;
+            }
 
             LockConsole();
             auto Unlock = wil::scope_exit([&] { UnlockConsole(); });
@@ -171,7 +187,10 @@ void PtySignalInputThread::CreatePseudoWindow()
         case PtySignal::SetParent:
         {
             SetParentData reparentMessage = { 0 };
-            _GetData(&reparentMessage, sizeof(reparentMessage));
+            if (!_GetData(&reparentMessage, sizeof(reparentMessage)))
+            {
+                return S_OK;
+            }
 
             LockConsole();
             auto Unlock = wil::scope_exit([&] { UnlockConsole(); });
@@ -191,13 +210,11 @@ void PtySignalInputThread::CreatePseudoWindow()
             break;
         }
         default:
-        {
             THROW_HR(E_UNEXPECTED);
         }
-        }
     }
-    return S_OK;
 }
+CATCH_LOG_RETURN_HR(S_OK)
 
 // Method Description:
 // - Dispatches a resize window message to the rest of the console code
@@ -269,29 +286,27 @@ void PtySignalInputThread::_DoSetWindowParent(const SetParentData& data)
 // - cbBuffer - Count of bytes in the given buffer.
 // Return Value:
 // - True if data was retrieved successfully. False otherwise.
-bool PtySignalInputThread::_GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer,
-                                    const DWORD cbBuffer)
+[[nodiscard]] bool PtySignalInputThread::_GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer, const DWORD cbBuffer)
 {
+    if (!_hFile)
+    {
+        return false;
+    }
+
     DWORD dwRead = 0;
-    // If we failed to read because the terminal broke our pipe (usually due
-    //      to dying itself), close gracefully with ERROR_BROKEN_PIPE.
-    // Otherwise throw an exception. ERROR_BROKEN_PIPE is the only case that
-    //       we want to gracefully close in.
     if (FALSE == ReadFile(_hFile.get(), pBuffer, cbBuffer, &dwRead, nullptr))
     {
-        auto lastError = GetLastError();
-        if (lastError == ERROR_BROKEN_PIPE)
+        if (const auto err = GetLastError(); err != ERROR_BROKEN_PIPE)
         {
-            _Shutdown();
-        }
-        else
-        {
-            THROW_WIN32(lastError);
+            LOG_WIN32(err);
         }
+        _hFile.reset();
+        return false;
     }
-    else if (dwRead != cbBuffer)
+
+    if (dwRead != cbBuffer)
     {
-        _Shutdown();
+        return false;
     }
 
     return true;
@@ -326,9 +341,6 @@ bool PtySignalInputThread::_GetData(_Out_writes_bytes_(cbBuffer) void* const pBu
 // - Perform a shutdown of the console. This happens when the signal pipe is
 //      broken, which means either the parent terminal process has died, or they
 //      called ClosePseudoConsole.
-//  CloseConsoleProcessState is not enough by itself - it will disconnect
-//      clients as if the X was pressed, but then we need to actually still die,
-//      so then call RundownAndExit.
 // Arguments:
 // - <none>
 // Return Value:
@@ -337,16 +349,4 @@ void PtySignalInputThread::_Shutdown()
 {
     // Trigger process shutdown.
     CloseConsoleProcessState();
-
-    // If we haven't terminated by now, that's because there's a client that's still attached.
-    // Force the handling of the control events by the attached clients.
-    // As of MSFT:19419231, CloseConsoleProcessState will make sure this
-    //      happens if this method is called outside of lock, but if we're
-    //      currently locked, we want to make sure ctrl events are handled
-    //      _before_ we RundownAndExit.
-    LockConsole();
-    ProcessCtrlEvents();
-
-    // Make sure we terminate.
-    ServiceLocator::RundownAndExit(ERROR_BROKEN_PIPE);
 }

src/host/PtySignalInputThread.hpp

@@ -60,8 +60,8 @@ namespace Microsoft::Console
             uint64_t handle;
         };
 
-        [[nodiscard]] HRESULT _InputThread();
-        bool _GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer, const DWORD cbBuffer);
+        [[nodiscard]] HRESULT _InputThread() noexcept;
+        [[nodiscard]] bool _GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer, const DWORD cbBuffer);
         void _DoResizeWindow(const ResizeWindowData& data);
         void _DoSetWindowParent(const SetParentData& data);
         void _DoClearBuffer();

src/host/VtInputThread.cpp

@@ -94,6 +94,7 @@ DWORD WINAPI VtInputThread::StaticVtInputThreadProc(_In_ LPVOID lpParameter)
 {
     const auto pInstance = reinterpret_cast<VtInputThread*>(lpParameter);
     pInstance->_InputThread();
+    return S_OK;
 }
 
 // Method Description:
@@ -110,10 +111,6 @@ void VtInputThread::DoReadInput(const bool throwOnFail)
     DWORD dwRead = 0;
     auto fSuccess = !!ReadFile(_hFile.get(), buffer, ARRAYSIZE(buffer), &dwRead, nullptr);
 
-    // If we failed to read because the terminal broke our pipe (usually due
-    //      to dying itself), close gracefully with ERROR_BROKEN_PIPE.
-    // Otherwise throw an exception. ERROR_BROKEN_PIPE is the only case that
-    //       we want to gracefully close in.
     if (!fSuccess)
     {
         _exitRequested = true;

src/host/VtInputThread.hpp

@@ -30,7 +30,7 @@ namespace Microsoft::Console
 
     private:
         [[nodiscard]] HRESULT _HandleRunInput(const std::string_view u8Str);
-        [[noreturn]] void _InputThread();
+        void _InputThread();
 
         wil::unique_hfile _hFile;
         wil::unique_handle _hThread;

src/host/VtIo.cpp

@@ -463,18 +463,6 @@ void VtIo::_shutdownNow()
     // If we have any remaining attached processes, this will prepare us to send a ctrl+close to them
     // if we don't, this will cause us to rundown and exit.
     CloseConsoleProcessState();
-
-    // If we haven't terminated by now, that's because there's a client that's still attached.
-    // Force the handling of the control events by the attached clients.
-    // As of MSFT:19419231, CloseConsoleProcessState will make sure this
-    //      happens if this method is called outside of lock, but if we're
-    //      currently locked, we want to make sure ctrl events are handled
-    //      _before_ we RundownAndExit.
-    LockConsole();
-    ProcessCtrlEvents();
-
-    // Make sure we terminate.
-    ServiceLocator::RundownAndExit(ERROR_BROKEN_PIPE);
 }
 
 // Method Description:

src/host/VtIo.hpp

@@ -38,7 +38,7 @@ namespace Microsoft::Console::VirtualTerminal
 
         [[nodiscard]] HRESULT SwitchScreenBuffer(const bool useAltBuffer);
 
-        [[noreturn]] void CloseInput();
+        void CloseInput();
         void CloseOutput();
 
         void BeginResize();
@@ -78,7 +78,7 @@ namespace Microsoft::Console::VirtualTerminal
 
         [[nodiscard]] HRESULT _Initialize(const HANDLE InHandle, const HANDLE OutHandle, const std::wstring& VtMode, _In_opt_ const HANDLE SignalHandle);
 
-        [[noreturn]] void _shutdownNow();
+        void _shutdownNow();
 
 #ifdef UNIT_TESTING
         friend class VtIoTests;

src/host/consoleInformation.cpp

@@ -399,11 +399,7 @@ void CONSOLE_INFORMATION::ShutdownMidiAudio()
 {
     if (_midiAudio)
     {
-        // We lock the console here to make sure the shutdown promise is
-        // set before the audio is unlocked in the thread that is playing.
-        LockConsole();
         _midiAudio->Shutdown();
-        UnlockConsole();
     }
 }
 

src/host/output.cpp

@@ -499,7 +499,11 @@ void SetActiveScreenBuffer(SCREEN_INFORMATION& screenInfo)
 // TODO: MSFT 9450717 This should join the ProcessList class when CtrlEvents become moved into the server. https://osgvsowi/9450717
 void CloseConsoleProcessState()
 {
+    LockConsole();
+    const auto unlock = wil::scope_exit([] { UnlockConsole(); });
+
     auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
+
     // If there are no connected processes, sending control events is pointless as there's no one do send them to. In
     // this case we'll just exit conhost.
 
@@ -513,14 +517,4 @@ void CloseConsoleProcessState()
     HandleCtrlEvent(CTRL_CLOSE_EVENT);
 
     gci.ShutdownMidiAudio();
-
-    // Jiggle the handle: (see MSFT:19419231)
-    // When we call this function, we'll only actually close the console once
-    //      we're totally unlocked. If our caller has the console locked, great,
-    //      we'll dispatch the ctrl event once they unlock. However, if they're
-    //      not running under lock (eg PtySignalInputThread::_GetData), then the
-    //      ctrl event will never actually get dispatched.
-    // So, lock and unlock here, to make sure the ctrl event gets handled.
-    LockConsole();
-    UnlockConsole();
 }

src/interactivity/base/ServiceLocator.cpp

@@ -41,13 +41,21 @@ void ServiceLocator::SetOneCoreTeardownFunction(void (*pfn)()) noexcept
 
 void ServiceLocator::RundownAndExit(const HRESULT hr)
 {
-    static std::atomic<bool> locked;
+    // The TriggerTeardown() call below depends on the render thread being able to acquire the
+    // console lock, so that it can safely progress with flushing the last frame. Since there's no
+    // coming back from this function (it's [[noreturn]]), it's safe to unlock the console here.
+    auto& gci = s_globals.getConsoleInformation();
+    while (gci.IsConsoleLocked())
+    {
+        gci.UnlockConsole();
+    }
 
     // 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.
+    static std::atomic<bool> locked;
     if (locked.exchange(true, std::memory_order_relaxed))
     {
         // If we reach this point, another thread is already in the process of exiting.

src/winconpty/ft_pty/ConPtyTests.cpp

@@ -10,14 +10,16 @@ using namespace WEX::TestExecution;
 
 class ConPtyTests
 {
-    TEST_CLASS(ConPtyTests);
+    BEGIN_TEST_CLASS(ConPtyTests)
+        TEST_CLASS_PROPERTY(L"TestTimeout", L"0:0:15") // 15s timeout
+    END_TEST_CLASS()
+
     const COORD defaultSize = { 80, 30 };
     TEST_METHOD(CreateConPtyNoPipes);
     TEST_METHOD(CreateConPtyBadSize);
     TEST_METHOD(GoodCreate);
     TEST_METHOD(GoodCreateMultiple);
     TEST_METHOD(SurvivesOnBreakOutput);
-    TEST_METHOD(DiesOnBreakBoth);
     TEST_METHOD(DiesOnClose);
 };
 
@@ -218,63 +220,6 @@ void ConPtyTests::SurvivesOnBreakOutput()
     VERIFY_ARE_EQUAL(dwExit, (DWORD)STILL_ACTIVE);
 }
 
-void ConPtyTests::DiesOnBreakBoth()
-{
-    PseudoConsole pty = { 0 };
-    wil::unique_handle outPipeOurSide;
-    wil::unique_handle inPipeOurSide;
-    wil::unique_handle outPipePseudoConsoleSide;
-    wil::unique_handle inPipePseudoConsoleSide;
-    SECURITY_ATTRIBUTES sa;
-    sa.nLength = sizeof(sa);
-    sa.bInheritHandle = TRUE;
-    sa.lpSecurityDescriptor = nullptr;
-    VERIFY_IS_TRUE(CreatePipe(inPipePseudoConsoleSide.addressof(), inPipeOurSide.addressof(), &sa, 0));
-    VERIFY_IS_TRUE(CreatePipe(outPipeOurSide.addressof(), outPipePseudoConsoleSide.addressof(), &sa, 0));
-    VERIFY_IS_TRUE(SetHandleInformation(inPipeOurSide.get(), HANDLE_FLAG_INHERIT, 0));
-    VERIFY_IS_TRUE(SetHandleInformation(outPipeOurSide.get(), HANDLE_FLAG_INHERIT, 0));
-
-    VERIFY_SUCCEEDED(
-        _CreatePseudoConsole(defaultSize,
-                             inPipePseudoConsoleSide.get(),
-                             outPipePseudoConsoleSide.get(),
-                             0,
-                             &pty));
-    auto closePty1 = wil::scope_exit([&] {
-        _ClosePseudoConsoleMembers(&pty, TRUE);
-    });
-
-    DWORD dwExit;
-    VERIFY_IS_TRUE(GetExitCodeProcess(pty.hConPtyProcess, &dwExit));
-    VERIFY_ARE_EQUAL(dwExit, (DWORD)STILL_ACTIVE);
-
-    wil::unique_process_information piClient;
-    std::wstring realCommand = L"cmd.exe";
-    VERIFY_SUCCEEDED(AttachPseudoConsole(&pty, realCommand, piClient.addressof()));
-
-    VERIFY_IS_TRUE(GetExitCodeProcess(piClient.hProcess, &dwExit));
-    VERIFY_ARE_EQUAL(dwExit, (DWORD)STILL_ACTIVE);
-
-    // Close one of the pipes...
-    VERIFY_IS_TRUE(CloseHandle(outPipeOurSide.get()));
-
-    // ... Wait for a couple seconds, make sure the child is still alive.
-    VERIFY_ARE_EQUAL(WaitForSingleObject(pty.hConPtyProcess, 2000), (DWORD)WAIT_TIMEOUT);
-    VERIFY_IS_TRUE(GetExitCodeProcess(pty.hConPtyProcess, &dwExit));
-    VERIFY_ARE_EQUAL(dwExit, (DWORD)STILL_ACTIVE);
-
-    // Tricky - write some input to the pcon. We need to do this so conhost can
-    //      realize that the output pipe has broken.
-    VERIFY_SUCCEEDED(WriteFile(inPipeOurSide.get(), L"a", sizeof(wchar_t), nullptr, nullptr));
-
-    // Close the other pipe, and make sure conhost dies
-    VERIFY_IS_TRUE(CloseHandle(inPipeOurSide.get()));
-
-    VERIFY_ARE_EQUAL(WaitForSingleObject(pty.hConPtyProcess, 10000), (DWORD)WAIT_OBJECT_0);
-    VERIFY_IS_TRUE(GetExitCodeProcess(pty.hConPtyProcess, &dwExit));
-    VERIFY_ARE_NOT_EQUAL(dwExit, (DWORD)STILL_ACTIVE);
-}
-
 void ConPtyTests::DiesOnClose()
 {
     BEGIN_TEST_METHOD_PROPERTIES()