Wait for clients to exit on ConPTY shutdown

src/host/PtySignalInputThread.cpp

@@ -96,17 +96,29 @@ 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
 {
-    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 +160,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 +186,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,12 +209,9 @@ void PtySignalInputThread::CreatePseudoWindow()
             break;
         }
         default:
-        {
-            THROW_HR(E_UNEXPECTED);
-        }
+            return S_OK;
         }
     }
-    return S_OK;
 }
 
 // Method Description:
@@ -272,26 +287,21 @@ void PtySignalInputThread::_DoSetWindowParent(const SetParentData& data)
 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)
-        {
-            _Shutdown();
-        }
-        else
-        {
-            THROW_WIN32(lastError);
-        }
+        _hFile.reset();
+        return false;
     }
-    else if (dwRead != cbBuffer)
+
+    if (dwRead != cbBuffer)
     {
-        _Shutdown();
+        return false;
     }
 
     return true;
@@ -346,7 +356,4 @@ void PtySignalInputThread::_Shutdown()
     //      _before_ we RundownAndExit.
     LockConsole();
     ProcessCtrlEvents();
-
-    // Make sure we terminate.
-    ServiceLocator::RundownAndExit(ERROR_BROKEN_PIPE);
 }

src/host/PtySignalInputThread.hpp

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

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

@@ -472,9 +472,6 @@ void VtIo::_shutdownNow()
     //      _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/winconpty/ft_pty/ConPtyTests.cpp

@@ -10,7 +10,10 @@ 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);
@@ -218,63 +221,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()

src/winconpty/winconpty.cpp

@@ -361,6 +361,14 @@ void _ClosePseudoConsoleMembers(_In_ PseudoConsole* pPty, BOOL wait)
             CloseHandle(pPty->hSignal);
             pPty->hSignal = nullptr;
         }
+        // The reference handle ensures that conhost keeps running unless ClosePseudoConsole is called.
+        // We have to call it before calling WaitForSingleObject however in order to not deadlock,
+        // Due to conhost waiting for all clients to disconnect, while we wait for conhost to exit.
+        if (_HandleIsValid(pPty->hPtyReference))
+        {
+            CloseHandle(pPty->hPtyReference);
+            pPty->hPtyReference = nullptr;
+        }
         // Then, wait on the conhost process before killing it.
         // We do this to make sure the conhost finishes flushing any output it
         //      has yet to send before we hard kill it.
@@ -374,14 +382,6 @@ void _ClosePseudoConsoleMembers(_In_ PseudoConsole* pPty, BOOL wait)
             CloseHandle(pPty->hConPtyProcess);
             pPty->hConPtyProcess = nullptr;
         }
-        // Then take care of the reference handle.
-        // TODO GH#1810: Closing the reference handle late leaves conhost thinking
-        // that we have an outstanding connected client.
-        if (_HandleIsValid(pPty->hPtyReference))
-        {
-            CloseHandle(pPty->hPtyReference);
-            pPty->hPtyReference = nullptr;
-        }
     }
 }