From 8bccc33f9aac00caf5ef66b06bcbe66019da23c1 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Sun, 23 Oct 2022 01:37:18 +0200 Subject: [PATCH 01/11] Wait for clients to exit on ConPTY shutdown --- src/host/PtySignalInputThread.cpp | 3 --- src/host/VtIo.cpp | 3 --- 2 files changed, 6 deletions(-) diff --git a/src/host/PtySignalInputThread.cpp b/src/host/PtySignalInputThread.cpp index 935354ebf6e..49dd546d9be 100644 --- a/src/host/PtySignalInputThread.cpp +++ b/src/host/PtySignalInputThread.cpp @@ -346,7 +346,4 @@ void PtySignalInputThread::_Shutdown() // _before_ we RundownAndExit. LockConsole(); ProcessCtrlEvents(); - - // Make sure we terminate. - ServiceLocator::RundownAndExit(ERROR_BROKEN_PIPE); } diff --git a/src/host/VtIo.cpp b/src/host/VtIo.cpp index fcb97f4aac3..8cc25806ab3 100644 --- a/src/host/VtIo.cpp +++ b/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: From 6f177cf78b193cbe88595c8dd7165ebbc9f60a4e Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Thu, 27 Oct 2022 17:35:50 +0200 Subject: [PATCH 02/11] Fix code that used to rely on [[noreturn]], Fix hPtyReference --- src/host/PtySignalInputThread.cpp | 60 ++++++++++++++++----------- src/host/PtySignalInputThread.hpp | 2 +- src/host/VtInputThread.cpp | 5 +-- src/host/VtInputThread.hpp | 2 +- src/host/VtIo.hpp | 4 +- src/winconpty/ft_pty/ConPtyTests.cpp | 62 ++-------------------------- src/winconpty/winconpty.cpp | 16 +++---- 7 files changed, 52 insertions(+), 99 deletions(-) diff --git a/src/host/PtySignalInputThread.cpp b/src/host/PtySignalInputThread.cpp index 49dd546d9be..073de80d437 100644 --- a/src/host/PtySignalInputThread.cpp +++ b/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; diff --git a/src/host/PtySignalInputThread.hpp b/src/host/PtySignalInputThread.hpp index f9f4268bb9c..e0ebaeed4b7 100644 --- a/src/host/PtySignalInputThread.hpp +++ b/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); diff --git a/src/host/VtInputThread.cpp b/src/host/VtInputThread.cpp index 9b2c8b3bdfe..495a6e51d8b 100644 --- a/src/host/VtInputThread.cpp +++ b/src/host/VtInputThread.cpp @@ -94,6 +94,7 @@ DWORD WINAPI VtInputThread::StaticVtInputThreadProc(_In_ LPVOID lpParameter) { const auto pInstance = reinterpret_cast(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; diff --git a/src/host/VtInputThread.hpp b/src/host/VtInputThread.hpp index 98741ff7e77..7c075b70025 100644 --- a/src/host/VtInputThread.hpp +++ b/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; diff --git a/src/host/VtIo.hpp b/src/host/VtIo.hpp index da1099704b1..7265464fbe6 100644 --- a/src/host/VtIo.hpp +++ b/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; diff --git a/src/winconpty/ft_pty/ConPtyTests.cpp b/src/winconpty/ft_pty/ConPtyTests.cpp index 4d41e6135ee..cb85450081e 100644 --- a/src/winconpty/ft_pty/ConPtyTests.cpp +++ b/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() diff --git a/src/winconpty/winconpty.cpp b/src/winconpty/winconpty.cpp index 7a4f9b56395..54685014321 100644 --- a/src/winconpty/winconpty.cpp +++ b/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; - } } } From a7db7a9309194679f95e869147489b7977e0ee4c Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Thu, 27 Oct 2022 17:45:28 +0200 Subject: [PATCH 03/11] Fix build --- src/winconpty/ft_pty/ConPtyTests.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/winconpty/ft_pty/ConPtyTests.cpp b/src/winconpty/ft_pty/ConPtyTests.cpp index cb85450081e..8289023f879 100644 --- a/src/winconpty/ft_pty/ConPtyTests.cpp +++ b/src/winconpty/ft_pty/ConPtyTests.cpp @@ -20,7 +20,6 @@ class ConPtyTests TEST_METHOD(GoodCreate); TEST_METHOD(GoodCreateMultiple); TEST_METHOD(SurvivesOnBreakOutput); - TEST_METHOD(DiesOnBreakBoth); TEST_METHOD(DiesOnClose); }; From 688a58721f090053326256ad18e6e58302bd7d4b Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Fri, 28 Oct 2022 00:43:40 +0200 Subject: [PATCH 04/11] Fix typo --- src/host/PtySignalInputThread.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/host/PtySignalInputThread.cpp b/src/host/PtySignalInputThread.cpp index 073de80d437..f2e7ae8564a 100644 --- a/src/host/PtySignalInputThread.cpp +++ b/src/host/PtySignalInputThread.cpp @@ -186,7 +186,7 @@ void PtySignalInputThread::CreatePseudoWindow() case PtySignal::SetParent: { SetParentData reparentMessage = { 0 }; - if (_GetData(&reparentMessage, sizeof(reparentMessage))) + if (!_GetData(&reparentMessage, sizeof(reparentMessage))) { return S_OK; } From d693df4a6dea75842161409f2afc4fa76d60ce38 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Fri, 28 Oct 2022 01:03:58 +0200 Subject: [PATCH 05/11] Fix another deadlock on shutdown --- src/host/PtySignalInputThread.cpp | 9 --------- src/host/VtIo.cpp | 9 --------- src/host/output.cpp | 14 ++++---------- src/interactivity/base/ServiceLocator.cpp | 8 ++++++++ 4 files changed, 12 insertions(+), 28 deletions(-) diff --git a/src/host/PtySignalInputThread.cpp b/src/host/PtySignalInputThread.cpp index f2e7ae8564a..77942a2fcaa 100644 --- a/src/host/PtySignalInputThread.cpp +++ b/src/host/PtySignalInputThread.cpp @@ -347,13 +347,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(); } diff --git a/src/host/VtIo.cpp b/src/host/VtIo.cpp index 8cc25806ab3..a02959f444a 100644 --- a/src/host/VtIo.cpp +++ b/src/host/VtIo.cpp @@ -463,15 +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(); } // Method Description: diff --git a/src/host/output.cpp b/src/host/output.cpp index 0a9fceb03c1..d3cfd7be1c0 100644 --- a/src/host/output.cpp +++ b/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(); } diff --git a/src/interactivity/base/ServiceLocator.cpp b/src/interactivity/base/ServiceLocator.cpp index 6218dbfc017..ee73b50565c 100644 --- a/src/interactivity/base/ServiceLocator.cpp +++ b/src/interactivity/base/ServiceLocator.cpp @@ -50,6 +50,14 @@ void ServiceLocator::RundownAndExit(const HRESULT hr) // because doing so would prevent the render thread from progressing. if (locked.exchange(true, std::memory_order_relaxed)) { + // Don't sleep forever with the console lock being held or otherwise + // no one can acquire it while we shut down. + const auto& gci = s_globals.getConsoleInformation(); + while (gci.IsConsoleLocked()) + { + gci.UnlockConsole(); + } + // If we reach this point, another thread is already in the process of exiting. // There's a lot of ways to suspend ourselves until we exit, one of which is "sleep forever". Sleep(INFINITE); From 7fb6ffa6a938a522d74ef70d5e217b153c566df2 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Mon, 14 Nov 2022 23:20:53 +0100 Subject: [PATCH 06/11] Fix merge origin/main --- src/interactivity/base/ServiceLocator.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/interactivity/base/ServiceLocator.cpp b/src/interactivity/base/ServiceLocator.cpp index c16589d923c..dd59231d13c 100644 --- a/src/interactivity/base/ServiceLocator.cpp +++ b/src/interactivity/base/ServiceLocator.cpp @@ -56,7 +56,7 @@ void ServiceLocator::RundownAndExit(const HRESULT hr) { // Don't sleep forever with the console lock being held or otherwise // no one can acquire it while we shut down. - const auto& gci = s_globals.getConsoleInformation(); + auto& gci = s_globals.getConsoleInformation(); while (gci.IsConsoleLocked()) { gci.UnlockConsole(); From bfd900492b788c849964b89f497ff879df5d9749 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Mon, 14 Nov 2022 23:21:02 +0100 Subject: [PATCH 07/11] Address feedback --- src/host/PtySignalInputThread.cpp | 9 ++++++--- src/host/PtySignalInputThread.hpp | 2 +- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/host/PtySignalInputThread.cpp b/src/host/PtySignalInputThread.cpp index 77942a2fcaa..d353d6f0204 100644 --- a/src/host/PtySignalInputThread.cpp +++ b/src/host/PtySignalInputThread.cpp @@ -97,6 +97,7 @@ void PtySignalInputThread::CreatePseudoWindow() // - S_OK if the thread runs to completion. // - Otherwise it may cause an application termination and never return. [[nodiscard]] HRESULT PtySignalInputThread::_InputThread() noexcept +try { const auto shutdown = wil::scope_exit([this]() { _Shutdown(); @@ -209,10 +210,11 @@ void PtySignalInputThread::CreatePseudoWindow() break; } default: - return S_OK; + THROW_HR(E_UNEXPECTED); } } } +CATCH_LOG_RETURN_HR(S_OK) // Method Description: // - Dispatches a resize window message to the rest of the console code @@ -284,8 +286,7 @@ 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) { @@ -295,6 +296,8 @@ bool PtySignalInputThread::_GetData(_Out_writes_bytes_(cbBuffer) void* const pBu DWORD dwRead = 0; if (FALSE == ReadFile(_hFile.get(), pBuffer, cbBuffer, &dwRead, nullptr)) { + const auto hr = GetLastError(); + LOG_HR_IF(hr, hr != ERROR_BROKEN_PIPE); _hFile.reset(); return false; } diff --git a/src/host/PtySignalInputThread.hpp b/src/host/PtySignalInputThread.hpp index e0ebaeed4b7..12089d1a5e8 100644 --- a/src/host/PtySignalInputThread.hpp +++ b/src/host/PtySignalInputThread.hpp @@ -61,7 +61,7 @@ namespace Microsoft::Console }; [[nodiscard]] HRESULT _InputThread() noexcept; - bool _GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer, const DWORD cbBuffer); + [[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(); From 0a1da70708884d4b07fbaf66ac9df4515ff3b421 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Tue, 15 Nov 2022 01:34:54 +0100 Subject: [PATCH 08/11] Fix build failure --- src/host/PtySignalInputThread.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/host/PtySignalInputThread.cpp b/src/host/PtySignalInputThread.cpp index d353d6f0204..e2c99828115 100644 --- a/src/host/PtySignalInputThread.cpp +++ b/src/host/PtySignalInputThread.cpp @@ -296,8 +296,10 @@ void PtySignalInputThread::_DoSetWindowParent(const SetParentData& data) DWORD dwRead = 0; if (FALSE == ReadFile(_hFile.get(), pBuffer, cbBuffer, &dwRead, nullptr)) { - const auto hr = GetLastError(); - LOG_HR_IF(hr, hr != ERROR_BROKEN_PIPE); + if (const auto err = GetLastError(); err != ERROR_BROKEN_PIPE) + { + LOG_WIN32(err); + } _hFile.reset(); return false; } From fb149827d069c2e5cf2a050c451a1c3a72bc86b1 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Fri, 18 Nov 2022 02:46:10 +0100 Subject: [PATCH 09/11] Dustin found a bug! --- src/interactivity/base/ServiceLocator.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/interactivity/base/ServiceLocator.cpp b/src/interactivity/base/ServiceLocator.cpp index dd59231d13c..23adfdb3c9e 100644 --- a/src/interactivity/base/ServiceLocator.cpp +++ b/src/interactivity/base/ServiceLocator.cpp @@ -45,23 +45,23 @@ void ServiceLocator::SetOneCoreTeardownFunction(void (*pfn)()) noexcept void ServiceLocator::RundownAndExit(const HRESULT hr) { - static std::atomic 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 locked; if (locked.exchange(true, std::memory_order_relaxed)) { - // Don't sleep forever with the console lock being held or otherwise - // no one can acquire it while we shut down. - auto& gci = s_globals.getConsoleInformation(); - while (gci.IsConsoleLocked()) - { - gci.UnlockConsole(); - } - // If we reach this point, another thread is already in the process of exiting. // There's a lot of ways to suspend ourselves until we exit, one of which is "sleep forever". Sleep(INFINITE); From 264481637ee83f35cd865e1271ca5fef86a78ab7 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Fri, 18 Nov 2022 02:46:24 +0100 Subject: [PATCH 10/11] Improve documentation --- src/winconpty/winconpty.cpp | 2 ++ src/winconpty/winconpty.h | 1 + 2 files changed, 3 insertions(+) diff --git a/src/winconpty/winconpty.cpp b/src/winconpty/winconpty.cpp index 54685014321..ed740920b9e 100644 --- a/src/winconpty/winconpty.cpp +++ b/src/winconpty/winconpty.cpp @@ -230,6 +230,8 @@ HRESULT _CreatePseudoConsole(const HANDLE hToken, pPty->hConPtyProcess = pi.hProcess; pi.hProcess = nullptr; + // The hPtyReference we create here is used when the PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE attribute is processed. + // This ensures that conhost's client processes inherit the correct (= our) console handle. RETURN_IF_NTSTATUS_FAILED(CreateClientHandle(&pPty->hPtyReference, serverHandle.get(), L"\\Reference", diff --git a/src/winconpty/winconpty.h b/src/winconpty/winconpty.h index c52c60e02a9..82355ccb071 100644 --- a/src/winconpty/winconpty.h +++ b/src/winconpty/winconpty.h @@ -7,6 +7,7 @@ extern "C" { #endif +// This structure is part of an ABI shared with the rest of the operating system. typedef struct _PseudoConsole { HANDLE hSignal; From a0d0e68aa0ea16366fa6f6ceb02bf900d4e0bb44 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Sat, 19 Nov 2022 03:32:13 +0100 Subject: [PATCH 11/11] Improve documentation --- src/host/PtySignalInputThread.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/host/PtySignalInputThread.cpp b/src/host/PtySignalInputThread.cpp index e2c99828115..6a91620198b 100644 --- a/src/host/PtySignalInputThread.cpp +++ b/src/host/PtySignalInputThread.cpp @@ -341,9 +341,6 @@ void PtySignalInputThread::_DoSetWindowParent(const SetParentData& data) // - 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: // - // Return Value: