diff --git a/src/cascadia/TerminalConnection/ConptyConnection.cpp b/src/cascadia/TerminalConnection/ConptyConnection.cpp index 6d82a880a2e..69a99a71a1c 100644 --- a/src/cascadia/TerminalConnection/ConptyConnection.cpp +++ b/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(_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); diff --git a/src/cascadia/TerminalConnection/ConptyConnection.h b/src/cascadia/TerminalConnection/ConptyConnection.h index beaaac058f1..90466dfad81 100644 --- a/src/cascadia/TerminalConnection/ConptyConnection.h +++ b/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; + using unique_static_pseudoconsole_handle = wil::unique_any; } namespace winrt::Microsoft::Terminal::TerminalConnection::implementation diff --git a/src/inc/conpty-static.h b/src/inc/conpty-static.h index 82982864dd6..27fa1437ae6 100644 --- a/src/inc/conpty-static.h +++ b/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); diff --git a/src/interactivity/base/ServiceLocator.cpp b/src/interactivity/base/ServiceLocator.cpp index 9471d70b2b9..d21b1be0bc5 100644 --- a/src/interactivity/base/ServiceLocator.cpp +++ b/src/interactivity/base/ServiceLocator.cpp @@ -39,14 +39,31 @@ 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 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)) + { + // 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); + } // MSFT:15506250 // In VT I/O Mode, a client application might die before we've rendered diff --git a/src/interactivity/inc/ServiceLocator.hpp b/src/interactivity/inc/ServiceLocator.hpp index 4a5f81b0512..c9221031bbe 100644 --- a/src/interactivity/inc/ServiceLocator.hpp +++ b/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; }; } diff --git a/src/winconpty/dll/winconpty.def b/src/winconpty/dll/winconpty.def index 1a1952bf421..c5b808e3a96 100644 --- a/src/winconpty/dll/winconpty.def +++ b/src/winconpty/dll/winconpty.def @@ -4,6 +4,7 @@ EXPORTS ConptyCreatePseudoConsoleAsUser ConptyResizePseudoConsole ConptyClosePseudoConsole + ConptyClosePseudoConsoleNoWait ConptyClearPseudoConsole ConptyShowHidePseudoConsole ConptyReparentPseudoConsole diff --git a/src/winconpty/ft_pty/ConPtyTests.cpp b/src/winconpty/ft_pty/ConPtyTests.cpp index 78cfe774f72..92574aba51b 100644 --- a/src/winconpty/ft_pty/ConPtyTests.cpp +++ b/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); diff --git a/src/winconpty/winconpty.cpp b/src/winconpty/winconpty.cpp index 19b91b797b3..7a4f9b56395 100644 --- a/src/winconpty/winconpty.cpp +++ b/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: // - -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: // - -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 diff --git a/src/winconpty/winconpty.h b/src/winconpty/winconpty.h index 289c42e5f28..c52c60e02a9 100644 --- a/src/winconpty/winconpty.h +++ b/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,