diff --git a/src/host/VtInputThread.cpp b/src/host/VtInputThread.cpp index 8706ff6f87a..9b2c8b3bdfe 100644 --- a/src/host/VtInputThread.cpp +++ b/src/host/VtInputThread.cpp @@ -30,7 +30,6 @@ VtInputThread::VtInputThread(_In_ wil::unique_hfile hPipe, _u8State{}, _dwThreadId{ 0 }, _exitRequested{ false }, - _exitResult{ S_OK }, _pfnSetLookingForDSR{} { THROW_HR_IF(E_HANDLE, _hFile.get() == INVALID_HANDLE_VALUE); @@ -94,7 +93,7 @@ VtInputThread::VtInputThread(_In_ wil::unique_hfile hPipe, DWORD WINAPI VtInputThread::StaticVtInputThreadProc(_In_ LPVOID lpParameter) { const auto pInstance = reinterpret_cast(lpParameter); - return pInstance->_InputThread(); + pInstance->_InputThread(); } // Method Description: @@ -118,7 +117,6 @@ void VtInputThread::DoReadInput(const bool throwOnFail) if (!fSuccess) { _exitRequested = true; - _exitResult = HRESULT_FROM_WIN32(GetLastError()); return; } @@ -127,7 +125,6 @@ void VtInputThread::DoReadInput(const bool throwOnFail) { if (throwOnFail) { - _exitResult = hr; _exitRequested = true; } else @@ -149,18 +146,13 @@ void VtInputThread::SetLookingForDSR(const bool looking) noexcept // - The ThreadProc for the VT Input Thread. Reads input from the pipe, and // passes it to _HandleRunInput to be processed by the // InputStateMachineEngine. -// Return Value: -// - Any error from reading the pipe or writing to the input buffer that might -// have caused us to exit. -DWORD VtInputThread::_InputThread() +void VtInputThread::_InputThread() { while (!_exitRequested) { DoReadInput(true); } ServiceLocator::LocateGlobals().getConsoleInformation().GetVtIo()->CloseInput(); - - return _exitResult; } // Method Description: diff --git a/src/host/VtInputThread.hpp b/src/host/VtInputThread.hpp index 9d54d6bbf8d..98741ff7e77 100644 --- a/src/host/VtInputThread.hpp +++ b/src/host/VtInputThread.hpp @@ -30,14 +30,13 @@ namespace Microsoft::Console private: [[nodiscard]] HRESULT _HandleRunInput(const std::string_view u8Str); - DWORD _InputThread(); + [[noreturn]] void _InputThread(); wil::unique_hfile _hFile; wil::unique_handle _hThread; DWORD _dwThreadId; bool _exitRequested; - HRESULT _exitResult; std::function _pfnSetLookingForDSR; diff --git a/src/host/VtIo.cpp b/src/host/VtIo.cpp index 3e9c17854c9..fcb97f4aac3 100644 --- a/src/host/VtIo.cpp +++ b/src/host/VtIo.cpp @@ -143,6 +143,8 @@ VtIo::VtIo() : auto& globals = ServiceLocator::LocateGlobals(); const auto& gci = globals.getConsoleInformation(); + // SetWindowVisibility uses the console lock to protect access to _pVtRenderEngine. + assert(gci.IsConsoleLocked()); try { @@ -337,21 +339,20 @@ void VtIo::CreatePseudoWindow() void VtIo::SetWindowVisibility(bool showOrHide) noexcept { - // MSFT:40853556 Grab the shutdown lock here, so that another - // thread can't trigger a CloseOutput and release the - // _pVtRenderEngine out from underneath us. - std::lock_guard lk(_shutdownLock); + auto& gci = ::Microsoft::Console::Interactivity::ServiceLocator::LocateGlobals().getConsoleInformation(); + + gci.LockConsole(); + auto unlock = wil::scope_exit([&] { gci.UnlockConsole(); }); + // ConsoleInputThreadProcWin32 calls VtIo::CreatePseudoWindow, + // which calls CreateWindowExW, which causes a WM_SIZE message. + // In short, this function might be called before _pVtRenderEngine exists. + // See PtySignalInputThread::CreatePseudoWindow(). if (!_pVtRenderEngine) { return; } - auto& gci = ::Microsoft::Console::Interactivity::ServiceLocator::LocateGlobals().getConsoleInformation(); - - gci.LockConsole(); - auto unlock = wil::scope_exit([&] { gci.UnlockConsole(); }); - LOG_IF_FAILED(_pVtRenderEngine->SetWindowVisibility(showOrHide)); } @@ -444,56 +445,36 @@ void VtIo::SetWindowVisibility(bool showOrHide) noexcept void VtIo::CloseInput() { - // This will release the lock when it goes out of scope - std::lock_guard lk(_shutdownLock); _pVtInputThread = nullptr; - _ShutdownIfNeeded(); + _shutdownNow(); } void VtIo::CloseOutput() { - // This will release the lock when it goes out of scope - std::lock_guard lk(_shutdownLock); - auto& g = ServiceLocator::LocateGlobals(); - // DON'T RemoveRenderEngine, as that requires the engine list lock, and this - // is usually being triggered on a paint operation, when the lock is already - // owned by the paint. - // Instead we're releasing the Engine here. A pointer to it has already been - // given to the Renderer, so we don't want the unique_ptr to delete it. The - // Renderer will own its lifetime now. - _pVtRenderEngine.release(); - g.getConsoleInformation().GetActiveOutputBuffer().SetTerminalConnection(nullptr); - - _ShutdownIfNeeded(); } -void VtIo::_ShutdownIfNeeded() +void VtIo::_shutdownNow() { - // The callers should have both acquired the _shutdownLock at this point - - // we dont want a race on who is actually responsible for closing it. - if (_objectsCreated && _pVtInputThread == nullptr && _pVtRenderEngine == nullptr) - { - // At this point, we no longer have a renderer or inthread. So we've - // effectively been disconnected from the terminal. - - // 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); - } + // At this point, we no longer have a renderer or inthread. So we've + // effectively been disconnected from the terminal. + + // 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: diff --git a/src/host/VtIo.hpp b/src/host/VtIo.hpp index b6a710e3e5a..da1099704b1 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); - void CloseInput(); + [[noreturn]] void CloseInput(); void CloseOutput(); void BeginResize(); @@ -67,7 +67,6 @@ namespace Microsoft::Console::VirtualTerminal bool _objectsCreated; bool _lookingForCursorPosition; - std::mutex _shutdownLock; bool _resizeQuirk{ false }; bool _win32InputMode{ false }; @@ -79,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); - void _ShutdownIfNeeded(); + [[noreturn]] void _shutdownNow(); #ifdef UNIT_TESTING friend class VtIoTests; diff --git a/src/interactivity/base/ServiceLocator.cpp b/src/interactivity/base/ServiceLocator.cpp index d21b1be0bc5..6218dbfc017 100644 --- a/src/interactivity/base/ServiceLocator.cpp +++ b/src/interactivity/base/ServiceLocator.cpp @@ -41,18 +41,8 @@ void ServiceLocator::SetOneCoreTeardownFunction(void (*pfn)()) noexcept 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(), diff --git a/src/interactivity/inc/ServiceLocator.hpp b/src/interactivity/inc/ServiceLocator.hpp index c9221031bbe..0bc0131bc21 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; - static void RundownAndExit(const HRESULT hr); + [[noreturn]] static void RundownAndExit(const HRESULT hr); // N.B.: Location methods without corresponding creation methods // automatically create the singleton object on demand. diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp index 169bcb83449..0e8639749b0 100644 --- a/src/renderer/vt/paint.cpp +++ b/src/renderer/vt/paint.cpp @@ -20,7 +20,7 @@ using namespace Microsoft::Console::Types; // HRESULT error code if painting didn't start successfully. [[nodiscard]] HRESULT VtEngine::StartPaint() noexcept { - if (_pipeBroken) + if (!_hFile) { return S_FALSE; } diff --git a/src/renderer/vt/state.cpp b/src/renderer/vt/state.cpp index e292e242104..0d10195b1e9 100644 --- a/src/renderer/vt/state.cpp +++ b/src/renderer/vt/state.cpp @@ -46,7 +46,6 @@ VtEngine::VtEngine(_In_ wil::unique_hfile pipe, _circled(false), _firstPaint(true), _skipCursor(false), - _pipeBroken(false), _exitResult{ S_OK }, _terminalOwner{ nullptr }, _newBottomLine{ false }, @@ -61,7 +60,7 @@ VtEngine::VtEngine(_In_ wil::unique_hfile pipe, { #ifndef UNIT_TESTING // When unit testing, we can instantiate a VtEngine without a pipe. - THROW_HR_IF(E_HANDLE, _hFile.get() == INVALID_HANDLE_VALUE); + THROW_HR_IF(E_HANDLE, !_hFile); #else // member is only defined when UNIT_TESTING is. _usingTestCallback = false; @@ -139,22 +138,14 @@ CATCH_RETURN(); [[nodiscard]] HRESULT VtEngine::_Flush() noexcept { -#ifdef UNIT_TESTING - if (_hFile.get() == INVALID_HANDLE_VALUE) - { - // Do not flush during Unit Testing because we won't have a valid file. - return S_OK; - } -#endif - - if (!_pipeBroken) + if (_hFile) { auto fSuccess = !!WriteFile(_hFile.get(), _buffer.data(), gsl::narrow_cast(_buffer.size()), nullptr, nullptr); _buffer.clear(); if (!fSuccess) { _exitResult = HRESULT_FROM_WIN32(GetLastError()); - _pipeBroken = true; + _hFile.reset(); if (_terminalOwner) { _terminalOwner->CloseOutput(); diff --git a/src/renderer/vt/vtrenderer.hpp b/src/renderer/vt/vtrenderer.hpp index 18ac4e4cc8f..a7cdb7326e7 100644 --- a/src/renderer/vt/vtrenderer.hpp +++ b/src/renderer/vt/vtrenderer.hpp @@ -127,7 +127,6 @@ namespace Microsoft::Console::Render bool _newBottomLine; til::point _deferredCursorPos; - bool _pipeBroken; HRESULT _exitResult; Microsoft::Console::VirtualTerminal::VtIo* _terminalOwner; diff --git a/src/winconpty/ft_pty/ConPtyTests.cpp b/src/winconpty/ft_pty/ConPtyTests.cpp index 92574aba51b..4d41e6135ee 100644 --- a/src/winconpty/ft_pty/ConPtyTests.cpp +++ b/src/winconpty/ft_pty/ConPtyTests.cpp @@ -16,7 +16,6 @@ class ConPtyTests TEST_METHOD(CreateConPtyBadSize); TEST_METHOD(GoodCreate); TEST_METHOD(GoodCreateMultiple); - TEST_METHOD(SurvivesOnBreakInput); TEST_METHOD(SurvivesOnBreakOutput); TEST_METHOD(DiesOnBreakBoth); TEST_METHOD(DiesOnClose); @@ -174,51 +173,6 @@ void ConPtyTests::GoodCreateMultiple() }); } -void ConPtyTests::SurvivesOnBreakInput() -{ - 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); - - VERIFY_IS_TRUE(CloseHandle(inPipeOurSide.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); -} - void ConPtyTests::SurvivesOnBreakOutput() { PseudoConsole pty = { 0 };