From 4dd476ecbd2dca59f288304d2b7581c8ff4c6e38 Mon Sep 17 00:00:00 2001 From: Michael Niksa Date: Fri, 8 Nov 2019 13:44:52 -0800 Subject: [PATCH] Revert locking changes (#3488) This reverts commit 56c35945b97ff59621959f621fa9ab5d0ec7e158. Also patches up some build fixes made after it and corrects a VtRendererTest that was dependent on the locks. --- src/host/PtySignalInputThread.cpp | 76 ++++++--- src/host/PtySignalInputThread.hpp | 8 +- src/host/VtInputThread.cpp | 120 +++++--------- src/host/VtInputThread.hpp | 16 +- src/host/VtIo.cpp | 153 ++++++++---------- src/host/VtIo.hpp | 15 +- src/host/ft_host/Host.FeatureTests.vcxproj | 1 - .../ft_host/Host.FeatureTests.vcxproj.filters | 3 - src/host/ft_host/sources | 1 - src/host/globals.h | 3 +- src/host/output.cpp | 23 ++- src/host/srvinit.cpp | 16 +- src/host/ut_host/VtIoTests.cpp | 35 +--- src/host/ut_host/VtRendererTests.cpp | 34 ++-- src/inc/ITerminalOwner.hpp | 33 ++++ src/inc/LibraryIncludes.h | 1 - src/interactivity/onecore/ConIoSrvComm.cpp | 2 +- .../onecore/ConsoleInputThread.cpp | 2 +- src/interactivity/win32/windowio.cpp | 18 +-- src/interactivity/win32/windowproc.cpp | 58 ++----- src/renderer/vt/WinTelnetEngine.cpp | 3 +- src/renderer/vt/WinTelnetEngine.hpp | 1 - src/renderer/vt/Xterm256Engine.cpp | 3 +- src/renderer/vt/Xterm256Engine.hpp | 1 - src/renderer/vt/XtermEngine.cpp | 3 +- src/renderer/vt/XtermEngine.hpp | 1 - src/renderer/vt/paint.cpp | 2 +- src/renderer/vt/state.cpp | 61 ++----- src/renderer/vt/vtrenderer.hpp | 15 +- src/server/DeviceComm.cpp | 21 +-- src/server/DeviceComm.h | 3 - src/server/ProcessList.cpp | 33 ---- src/server/ProcessList.h | 5 - 33 files changed, 290 insertions(+), 480 deletions(-) create mode 100644 src/inc/ITerminalOwner.hpp diff --git a/src/host/PtySignalInputThread.cpp b/src/host/PtySignalInputThread.cpp index 861fb5205..c2218d0a7 100644 --- a/src/host/PtySignalInputThread.cpp +++ b/src/host/PtySignalInputThread.cpp @@ -26,18 +26,13 @@ using namespace Microsoft::Console::VirtualTerminal; // - Creates the PTY Signal Input Thread. // Arguments: // - hPipe - a handle to the file representing the read end of the VT pipe. -// - shutdownEvent - An event that will be signaled when the entire console is going down -// We can use this to know when to cancel what we're doing and cleanup. -PtySignalInputThread::PtySignalInputThread(wil::unique_hfile hPipe, - wil::shared_event shutdownEvent) : +PtySignalInputThread::PtySignalInputThread(_In_ wil::unique_hfile hPipe) : _hFile{ std::move(hPipe) }, - _shutdownEvent{ shutdownEvent }, _hThread{}, _pConApi{ std::make_unique(ServiceLocator::LocateGlobals().getConsoleInformation()) }, _dwThreadId{ 0 }, _consoleConnected{ false } { - THROW_HR_IF(E_HANDLE, !_shutdownEvent); THROW_HR_IF(E_HANDLE, _hFile.get() == INVALID_HANDLE_VALUE); THROW_IF_NULL_ALLOC(_pConApi.get()); } @@ -84,21 +79,15 @@ void PtySignalInputThread::ConnectConsole() noexcept // - Otherwise it may cause an application termination another route and never return. [[nodiscard]] HRESULT PtySignalInputThread::_InputThread() { - auto onExitTriggerShutdown = wil::scope_exit([&] { - _shutdownEvent.SetEvent(); - }); - - while (true) + unsigned short signalId; + while (_GetData(&signalId, sizeof(signalId))) { - unsigned short signalId; - RETURN_IF_FAILED(_GetData(&signalId, sizeof(signalId))); - switch (signalId) { case PTY_SIGNAL_RESIZE_WINDOW: { PTY_SIGNAL_RESIZE resizeMsg = { 0 }; - RETURN_IF_FAILED(_GetData(&resizeMsg, sizeof(resizeMsg))); + _GetData(&resizeMsg, sizeof(resizeMsg)); LockConsole(); auto Unlock = wil::scope_exit([&] { UnlockConsole(); }); @@ -128,7 +117,7 @@ void PtySignalInputThread::ConnectConsole() noexcept } default: { - RETURN_HR(E_UNEXPECTED); + THROW_HR(E_UNEXPECTED); } } } @@ -143,19 +132,34 @@ void PtySignalInputThread::ConnectConsole() noexcept // - cbBuffer - Count of bytes in the given buffer. // Return Value: // - True if data was retrieved successfully. False otherwise. -[[nodiscard]] HRESULT PtySignalInputThread::_GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer, - const DWORD cbBuffer) +bool PtySignalInputThread::_GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer, + const DWORD cbBuffer) { 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. - RETURN_IF_WIN32_BOOL_FALSE(ReadFile(_hFile.get(), pBuffer, cbBuffer, &dwRead, nullptr)); + if (FALSE == ReadFile(_hFile.get(), pBuffer, cbBuffer, &dwRead, nullptr)) + { + DWORD lastError = GetLastError(); + if (lastError == ERROR_BROKEN_PIPE) + { + _Shutdown(); + return false; + } + else + { + THROW_WIN32(lastError); + } + } + else if (dwRead != cbBuffer) + { + _Shutdown(); + return false; + } - RETURN_HR_IF(E_UNEXPECTED, dwRead != cbBuffer); - - return S_OK; + return true; } // Method Description: @@ -181,3 +185,31 @@ void PtySignalInputThread::ConnectConsole() noexcept return S_OK; } + +// Method Description: +// - 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: +// - +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. + ProcessCtrlEvents(); + + // Make sure we terminate. + ServiceLocator::RundownAndExit(ERROR_BROKEN_PIPE); +} diff --git a/src/host/PtySignalInputThread.hpp b/src/host/PtySignalInputThread.hpp index 64d5cbac5..ea9419a32 100644 --- a/src/host/PtySignalInputThread.hpp +++ b/src/host/PtySignalInputThread.hpp @@ -20,8 +20,7 @@ namespace Microsoft::Console class PtySignalInputThread final { public: - PtySignalInputThread(wil::unique_hfile hPipe, - wil::shared_event shutdownEvent); + PtySignalInputThread(_In_ wil::unique_hfile hPipe); ~PtySignalInputThread(); [[nodiscard]] HRESULT Start() noexcept; @@ -35,9 +34,8 @@ namespace Microsoft::Console private: [[nodiscard]] HRESULT _InputThread(); - [[nodiscard]] HRESULT _GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer, const DWORD cbBuffer); - - wil::shared_event _shutdownEvent; + bool _GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer, const DWORD cbBuffer); + void _Shutdown(); wil::unique_hfile _hFile; wil::unique_handle _hThread; diff --git a/src/host/VtInputThread.cpp b/src/host/VtInputThread.cpp index bc91f6a9c..e7e968204 100644 --- a/src/host/VtInputThread.cpp +++ b/src/host/VtInputThread.cpp @@ -24,16 +24,15 @@ using namespace Microsoft::Console::VirtualTerminal; // - hPipe - a handle to the file representing the read end of the VT pipe. // - inheritCursor - a bool indicating if the state machine should expect a // cursor positioning sequence. See MSFT:15681311. -VtInputThread::VtInputThread(wil::unique_hfile hPipe, - wil::shared_event shutdownEvent, +VtInputThread::VtInputThread(_In_ wil::unique_hfile hPipe, const bool inheritCursor) : _hFile{ std::move(hPipe) }, - _shutdownEvent{ shutdownEvent }, _hThread{}, _utf8Parser{ CP_UTF8 }, - _dwThreadId{ 0 } + _dwThreadId{ 0 }, + _exitRequested{ false }, + _exitResult{ S_OK } { - THROW_HR_IF(E_HANDLE, !_shutdownEvent); THROW_HR_IF(E_HANDLE, _hFile.get() == INVALID_HANDLE_VALUE); CONSOLE_INFORMATION& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); @@ -46,30 +45,6 @@ VtInputThread::VtInputThread(wil::unique_hfile hPipe, _pInputStateMachine = std::make_unique(engine.release()); THROW_IF_NULL_ALLOC(_pInputStateMachine.get()); - - _shutdownWatchdog = std::async(std::launch::async, [&] { - _shutdownEvent.wait(); - if (_dwThreadId != 0) - { - wil::unique_handle threadHandle(OpenThread(STANDARD_RIGHTS_ALL | THREAD_TERMINATE, FALSE, _dwThreadId)); - LOG_LAST_ERROR_IF_NULL(threadHandle.get()); - if (threadHandle) - { - LOG_IF_WIN32_BOOL_FALSE(CancelSynchronousIo(threadHandle.get())); - } - } - }); -} - -VtInputThread::~VtInputThread() -{ - if (_shutdownEvent) - { - _shutdownEvent.SetEvent(); - - // Wait for watchdog future to get the memo or we might try to destroy it before it gets to work. - _shutdownWatchdog.wait(); - } } // Method Description: @@ -121,54 +96,44 @@ DWORD WINAPI VtInputThread::StaticVtInputThreadProc(_In_ LPVOID lpParameter) return pInstance->_InputThread(); } -// Routine Description: -// - A public way of pumping a single input message through the VT input channel -// - Reading input can be a blocking operation. This function will capture -// the thread ID of whomever calls it so it can be unblocked on shutdown events -// by a watchdog thread. -// - This function cannot be called by two public methods simultaneously. -// If another is already waiting in a blocked read on the VT input thread, -// an invalid state error will be returned. -// - This function is only valid during startup. Once the real VtInputThread starts -// to process the input, it will fill the thread ID field permanently until shutdown. -// Arguments: -// - -// Return Value: -// - S_OK, a ReadFile error, an error processing input, or an invalid state error if another thread is already waiting. -[[nodiscard]] HRESULT VtInputThread::DoReadInput() -{ - // If there's already a thread pumping VT input, it's not valid to read this from the outside. - RETURN_HR_IF(HRESULT_FROM_WIN32(ERROR_INVALID_STATE), _dwThreadId != 0); - - // Store which thread is attempting to read VT input. It may get blocked indefinitely and need - // to get unstuck by a shutdown event. - _dwThreadId = GetCurrentThreadId(); - - // Set it back to 0 on the way out. - auto restoreThreadId = wil::scope_exit([&] { - _dwThreadId = 0; - }); - - // Perform the blocking read operation. - return _ReadInput(); -} - // Method Description: -// - Do a single ReadFile from our pipe, and try and handle it. +// - Do a single ReadFile from our pipe, and try and handle it. If handling +// failed, throw or log, depending on what the caller wants. // Arguments: -// - +// - throwOnFail: If true, throw an exception if there was an error processing +// the input recieved. Otherwise, log the error. // Return Value: -// - S_OK or relevant error -[[nodiscard]] HRESULT VtInputThread::_ReadInput() +// - +void VtInputThread::DoReadInput(const bool throwOnFail) { byte buffer[256]; DWORD dwRead = 0; + bool fSuccess = !!ReadFile(_hFile.get(), buffer, ARRAYSIZE(buffer), &dwRead, nullptr); - RETURN_IF_WIN32_BOOL_FALSE(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; + _exitResult = HRESULT_FROM_WIN32(GetLastError()); + return; + } - RETURN_IF_FAILED(_HandleRunInput(buffer, dwRead)); - - return S_OK; + HRESULT hr = _HandleRunInput(buffer, dwRead); + if (FAILED(hr)) + { + if (throwOnFail) + { + _exitResult = hr; + _exitRequested = true; + } + else + { + LOG_IF_FAILED(hr); + } + } } // Method Description: @@ -180,19 +145,13 @@ DWORD WINAPI VtInputThread::StaticVtInputThreadProc(_In_ LPVOID lpParameter) // have caused us to exit. DWORD VtInputThread::_InputThread() { - auto onExitTriggerShutdown = wil::scope_exit([&] { - _shutdownEvent.SetEvent(); - }); - - while (true) + while (!_exitRequested) { - // NOTE: From inside the thread itself, we don't need to stash the thread handle each call - // because it was done permanently for us when the thread was created. No one else is allowed - // in through the public method while the actual VtInputThread is running. Only during startup. - RETURN_IF_FAILED(_ReadInput()); + DoReadInput(true); } + ServiceLocator::LocateGlobals().getConsoleInformation().GetVtIo()->CloseInput(); - return S_OK; + return _exitResult; } // Method Description: @@ -214,9 +173,6 @@ DWORD VtInputThread::_InputThread() RETURN_LAST_ERROR_IF_NULL(hThread); _hThread.reset(hThread); - - // This will permanently shut the door on the public read method until shutdown. - // Once the thread is servicing messages, we don't want any other threads getting in here. _dwThreadId = dwThreadId; return S_OK; diff --git a/src/host/VtInputThread.hpp b/src/host/VtInputThread.hpp index f103793fe..9f596d709 100644 --- a/src/host/VtInputThread.hpp +++ b/src/host/VtInputThread.hpp @@ -22,29 +22,23 @@ namespace Microsoft::Console class VtInputThread { public: - VtInputThread(wil::unique_hfile hPipe, - wil::shared_event shutdownEvent, - const bool inheritCursor); - - ~VtInputThread(); + VtInputThread(_In_ wil::unique_hfile hPipe, const bool inheritCursor); [[nodiscard]] HRESULT Start(); static DWORD WINAPI StaticVtInputThreadProc(_In_ LPVOID lpParameter); - - [[nodiscard]] HRESULT DoReadInput(); + void DoReadInput(const bool throwOnFail); private: [[nodiscard]] HRESULT _HandleRunInput(_In_reads_(cch) const byte* const charBuffer, const int cch); DWORD _InputThread(); - [[nodiscard]] HRESULT _ReadInput(); - - wil::shared_event _shutdownEvent; - std::future _shutdownWatchdog; wil::unique_hfile _hFile; wil::unique_handle _hThread; DWORD _dwThreadId; + bool _exitRequested; + HRESULT _exitResult; + std::unique_ptr _pInputStateMachine; Utf8ToWideCharParser _utf8Parser; }; diff --git a/src/host/VtIo.cpp b/src/host/VtIo.cpp index 108411030..cf7ab1ecb 100644 --- a/src/host/VtIo.cpp +++ b/src/host/VtIo.cpp @@ -11,7 +11,7 @@ #include "../renderer/base/renderer.hpp" #include "../types/inc/utils.hpp" -#include "handle.h" // LockConsole and UnlockConsole +#include "input.h" // ProcessCtrlEvents #include "output.h" // CloseConsoleProcessState using namespace Microsoft::Console; @@ -25,24 +25,10 @@ VtIo::VtIo() : _initialized(false), _objectsCreated(false), _lookingForCursorPosition(false), - _IoMode(VtIoMode::INVALID), -#ifdef UNIT_TESTING - _doNotTerminate(false), -#endif - _shutdownEvent() + _IoMode(VtIoMode::INVALID) { } -VtIo::~VtIo() -{ - if (_shutdownEvent) - { - _shutdownEvent.SetEvent(); - // Wait for watchdog future to get the memo or we might try to destroy it before it gets to work. - _shutdownWatchdog.wait(); - } -} - // Routine Description: // Tries to get the VtIoMode from the given string. If it's not one of the // *_STRING constants in VtIoMode.hpp, then it returns E_INVALIDARG. @@ -131,35 +117,9 @@ VtIo::~VtIo() _hOutput.reset(OutHandle); _hSignal.reset(SignalHandle); - // Since we have a valid request for a PTY, set up the events and watchdog mechanisms - // required to tear everything apart correctly at the end of a PTY session. - _shutdownEvent.create(wil::EventOptions::ManualReset); - - _shutdownWatchdog = std::async(std::launch::async, [=] { - _shutdownEvent.wait(); - - LockConsole(); - auto Unlock = wil::scope_exit([&] { UnlockConsole(); }); - -#ifdef UNIT_TESTING - // Don't close process state if we're unit testing and this has been set by a friend because - // it will trigger the rundown and exit TerminateProcess killing the test host! - if (_doNotTerminate) - { - return; - } -#endif - - CloseConsoleProcessState(); - }); - - // We also need to register to know when the last process is exiting. - ServiceLocator::LocateGlobals().getConsoleInformation().ProcessHandleList.RegisterForNotifyOnLastFree(std::bind(&VtIo::_OnLastProcessExit, this)); - // The only way we're initialized is if the args said we're in conpty mode. // If the args say so, then at least one of in, out, or signal was specified _initialized = true; - return S_OK; } @@ -186,7 +146,7 @@ VtIo::~VtIo() { if (IsValidHandle(_hInput.get())) { - _pVtInputThread = std::make_unique(std::move(_hInput), _shutdownEvent, _lookingForCursorPosition); + _pVtInputThread = std::make_unique(std::move(_hInput), _lookingForCursorPosition); } if (IsValidHandle(_hOutput.get())) @@ -198,7 +158,6 @@ VtIo::~VtIo() { case VtIoMode::XTERM_256: _pVtRenderEngine = std::make_unique(std::move(_hOutput), - _shutdownEvent, gci, initialViewport, gci.GetColorTable(), @@ -206,7 +165,6 @@ VtIo::~VtIo() break; case VtIoMode::XTERM: _pVtRenderEngine = std::make_unique(std::move(_hOutput), - _shutdownEvent, gci, initialViewport, gci.GetColorTable(), @@ -215,7 +173,6 @@ VtIo::~VtIo() break; case VtIoMode::XTERM_ASCII: _pVtRenderEngine = std::make_unique(std::move(_hOutput), - _shutdownEvent, gci, initialViewport, gci.GetColorTable(), @@ -224,7 +181,6 @@ VtIo::~VtIo() break; case VtIoMode::WIN_TELNET: _pVtRenderEngine = std::make_unique(std::move(_hOutput), - _shutdownEvent, gci, initialViewport, gci.GetColorTable(), @@ -233,6 +189,10 @@ VtIo::~VtIo() default: return E_FAIL; } + if (_pVtRenderEngine) + { + _pVtRenderEngine->SetTerminalOwner(this); + } } } CATCH_RETURN(); @@ -285,29 +245,18 @@ bool VtIo::IsUsingVt() const // We need both handles for this initialization to work. If we don't have // both, we'll skip it. They either aren't going to be reading output // (so they can't get the DSR) or they can't write the response to us. - // GH Microsoft/Terminal#1810: - // - We can't just infinite loop and let them hang. It needs to recognize an error - // condition otherwise it can be getting notified on another thread of a proper shutdown - // and be unable to actually leave this loop and let go of the lock. if (_lookingForCursorPosition && _pVtRenderEngine && _pVtInputThread) { - RETURN_IF_FAILED(_pVtRenderEngine->RequestCursor()); - - // The consequences of the VT Input - // receiving its cursor position information cause additional entrances to the global - // lock that we're already holding on the IO thread as a part of initialization here. - // So we have to pump the read manually here until we get what we're looking for. - while (!_shutdownEvent.is_signaled() && _lookingForCursorPosition) + LOG_IF_FAILED(_pVtRenderEngine->RequestCursor()); + while (_lookingForCursorPosition) { - RETURN_IF_FAILED(_pVtInputThread->DoReadInput()); + _pVtInputThread->DoReadInput(false); } } - // We can't start the VT input thread until after we've pumped it manually - // (if necessary) above for the cursor response. if (_pVtInputThread) { - RETURN_IF_FAILED(_pVtInputThread->Start()); + LOG_IF_FAILED(_pVtInputThread->Start()); } if (_pPtySignalInputThread) @@ -344,7 +293,7 @@ bool VtIo::IsUsingVt() const { try { - _pPtySignalInputThread = std::make_unique(std::move(_hSignal), _shutdownEvent); + _pPtySignalInputThread = std::make_unique(std::move(_hSignal)); // Start it if it was successfully created. RETURN_IF_FAILED(_pPtySignalInputThread->Start()); @@ -396,6 +345,59 @@ bool VtIo::IsUsingVt() const return hr; } +void VtIo::CloseInput() +{ + // This will release the lock when it goes out of scope + std::lock_guard lk(_shutdownLock); + _pVtInputThread = nullptr; + _ShutdownIfNeeded(); +} + +void VtIo::CloseOutput() +{ + // This will release the lock when it goes out of scope + std::lock_guard lk(_shutdownLock); + + Globals& 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() +{ + // The callers should have both accquired 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. + ProcessCtrlEvents(); + + // Make sure we terminate. + ServiceLocator::RundownAndExit(ERROR_BROKEN_PIPE); + } +} + // Method Description: // - Tell the vt renderer to begin a resize operation. During a resize // operation, the vt renderer should _not_ request to be repainted during a @@ -429,28 +431,3 @@ void VtIo::EndResize() _pVtRenderEngine->EndResizeRequest(); } } - -// Method Description: -// - Called when the last client process exits. We'll use this to shut off the main IO thread. -// Arguments: -// - -// Return Value: -// - -void VtIo::_OnLastProcessExit() -{ - // If a shutdown is signaled because one of the PTY pipes has broken connection with the - // hosting Terminal on top and we've been notified that the last client application has just - // disconnected, then we want to stop all IO channel communication. - // - // Normally IO channel communication stops itself when all outstanding references to the console - // session are broken with the driver, but in this circumstance, the Terminal application may still - // be holding a server, reference, or other handle to the session. It is implied that breaking any - // one of the connections or calling ClosePseudoConsole is a specific request to close all those things off from the - // Terminal's end. That means those handles are not really valid means of keeping the session open and therefore - // once the client connections are gone, there's no longer a reason to stick around even if the Terminal on top - // didn't fully clean up all of its handles before reaching this state. - if (_shutdownEvent.is_signaled()) - { - ServiceLocator::LocateGlobals().pDeviceComm->Shutdown(); - } -} diff --git a/src/host/VtIo.hpp b/src/host/VtIo.hpp index 16c2e8035..0516d9ab0 100644 --- a/src/host/VtIo.hpp +++ b/src/host/VtIo.hpp @@ -4,6 +4,7 @@ #pragma once #include "..\inc\VtIoModes.hpp" +#include "..\inc\ITerminalOwner.hpp" #include "..\renderer\vt\vtrenderer.hpp" #include "VtInputThread.hpp" #include "PtySignalInputThread.hpp" @@ -12,11 +13,11 @@ class ConsoleArguments; namespace Microsoft::Console::VirtualTerminal { - class VtIo + class VtIo : public Microsoft::Console::ITerminalOwner { public: VtIo(); - ~VtIo(); + virtual ~VtIo() override = default; [[nodiscard]] HRESULT Initialize(const ConsoleArguments* const pArgs); @@ -32,13 +33,13 @@ namespace Microsoft::Console::VirtualTerminal [[nodiscard]] HRESULT SuppressResizeRepaint(); [[nodiscard]] HRESULT SetCursorPosition(const COORD coordCursor); + void CloseInput() override; + void CloseOutput() override; + void BeginResize(); void EndResize(); private: - wil::shared_event _shutdownEvent; - std::future _shutdownWatchdog; - // After CreateIoHandlers is called, these will be invalid. wil::unique_hfile _hInput; wil::unique_hfile _hOutput; @@ -50,6 +51,7 @@ namespace Microsoft::Console::VirtualTerminal bool _objectsCreated; bool _lookingForCursorPosition; + std::mutex _shutdownLock; std::unique_ptr _pVtRenderEngine; std::unique_ptr _pVtInputThread; @@ -57,10 +59,9 @@ namespace Microsoft::Console::VirtualTerminal [[nodiscard]] HRESULT _Initialize(const HANDLE InHandle, const HANDLE OutHandle, const std::wstring& VtMode, _In_opt_ const HANDLE SignalHandle); - void _OnLastProcessExit(); + void _ShutdownIfNeeded(); #ifdef UNIT_TESTING - bool _doNotTerminate; friend class VtIoTests; #endif }; diff --git a/src/host/ft_host/Host.FeatureTests.vcxproj b/src/host/ft_host/Host.FeatureTests.vcxproj index 05d8d544f..7299a0701 100644 --- a/src/host/ft_host/Host.FeatureTests.vcxproj +++ b/src/host/ft_host/Host.FeatureTests.vcxproj @@ -20,7 +20,6 @@ - diff --git a/src/host/ft_host/Host.FeatureTests.vcxproj.filters b/src/host/ft_host/Host.FeatureTests.vcxproj.filters index 696789c0b..514a6e751 100644 --- a/src/host/ft_host/Host.FeatureTests.vcxproj.filters +++ b/src/host/ft_host/Host.FeatureTests.vcxproj.filters @@ -84,9 +84,6 @@ Source Files\API - - Source Files\API - diff --git a/src/host/ft_host/sources b/src/host/ft_host/sources index 4725caf92..7a8837b2a 100644 --- a/src/host/ft_host/sources +++ b/src/host/ft_host/sources @@ -36,7 +36,6 @@ SOURCES = \ API_RgbColorTests.cpp \ API_TitleTests.cpp \ API_PolicyTests.cpp \ - API_PtyTests.cpp \ CJK_DbcsTests.cpp \ Message_KeyPressTests.cpp \ DefaultResource.rc # Autogenerated file name + version for Device Guard whitelisting effort diff --git a/src/host/globals.h b/src/host/globals.h index cdbee50ba..459bd3689 100644 --- a/src/host/globals.h +++ b/src/host/globals.h @@ -56,8 +56,7 @@ public: ULONG cursorPixelWidth = 1; NTSTATUS ntstatusConsoleInputInitStatus; - wil::unique_event_nothrow consoleInputSetupEvent; - wil::unique_event_nothrow consoleInputInitializedEvent; + wil::unique_event_nothrow hConsoleInputInitEvent; DWORD dwInputThreadId; std::vector WordDelimiters; diff --git a/src/host/output.cpp b/src/host/output.cpp index 198e121f6..74f4b44fe 100644 --- a/src/host/output.cpp +++ b/src/host/output.cpp @@ -474,21 +474,10 @@ void SetActiveScreenBuffer(SCREEN_INFORMATION& screenInfo) WriteToScreen(screenInfo, screenInfo.GetViewport()); } -// Routine Description: -// - Dispatches final close event to connected clients or will run down and exit (and never return) if all clients -// are already gone. -// - NOTE: MUST BE CALLED UNDER LOCK. We don't want clients joining or leaving while we're telling them to close and running down. -// Arguments: -// - -// Return Value: -// - // TODO: MSFT 9450717 This should join the ProcessList class when CtrlEvents become moved into the server. https://osgvsowi/9450717 void CloseConsoleProcessState() { - auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); - - FAIL_FAST_IF(!(gci.IsConsoleLocked())); - + const CONSOLE_INFORMATION& 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. @@ -500,4 +489,14 @@ void CloseConsoleProcessState() } HandleCtrlEvent(CTRL_CLOSE_EVENT); + + // 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 displatch 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(); + auto Unlock = wil::scope_exit([&] { UnlockConsole(); }); } diff --git a/src/host/srvinit.cpp b/src/host/srvinit.cpp index 9b3d181a2..03784839e 100644 --- a/src/host/srvinit.cpp +++ b/src/host/srvinit.cpp @@ -260,8 +260,7 @@ void ConsoleCheckDebug() { auto& g = ServiceLocator::LocateGlobals(); RETURN_IF_FAILED(ConsoleServerInitialization(Server, args)); - RETURN_IF_FAILED(g.consoleInputSetupEvent.create(wil::EventOptions::ManualReset)); - RETURN_IF_FAILED(g.consoleInputInitializedEvent.create(wil::EventOptions::ManualReset)); + RETURN_IF_FAILED(g.hConsoleInputInitEvent.create(wil::EventOptions::None)); // Set up and tell the driver about the input available event. RETURN_IF_FAILED(g.hInputEvent.create(wil::EventOptions::ManualReset)); @@ -576,12 +575,10 @@ PWSTR TranslateConsoleTitle(_In_ PCWSTR pwszConsoleTitle, const BOOL fUnexpand, { ServiceLocator::LocateGlobals().dwInputThreadId = pNewThread->GetThreadId(); - // The ConsoleInputThread needs to perform things under lock, - // but if we unlock it, we don't know who will get the lock. - // So we will signal that it is safe for the other thread to do its work as - // we hold the lock on its behalf and wait for it to tell us that it is done. - g.consoleInputSetupEvent.SetEvent(); - g.consoleInputInitializedEvent.wait(); + // The ConsoleInputThread needs to lock the console so we must first unlock it ourselves. + UnlockConsole(); + g.hConsoleInputInitEvent.wait(); + LockConsole(); // OK, we've been told that the input thread is done initializing under lock. // Cleanup the handles and events we used to maintain our virtual lock passing dance. @@ -680,8 +677,7 @@ DWORD WINAPI ConsoleIoThread(LPVOID /*lpParameter*/) HRESULT hr = ServiceLocator::LocateGlobals().pDeviceComm->ReadIo(ReplyMsg, &ReceiveMsg); if (FAILED(hr)) { - if (hr == HRESULT_FROM_WIN32(ERROR_PIPE_NOT_CONNECTED) || - hr == E_APPLICATION_EXITING) + if (hr == HRESULT_FROM_WIN32(ERROR_PIPE_NOT_CONNECTED)) { fShouldExit = true; diff --git a/src/host/ut_host/VtIoTests.cpp b/src/host/ut_host/VtIoTests.cpp index c0e0a30aa..860fb9209 100644 --- a/src/host/ut_host/VtIoTests.cpp +++ b/src/host/ut_host/VtIoTests.cpp @@ -23,20 +23,6 @@ class Microsoft::Console::VirtualTerminal::VtIoTests { TEST_CLASS(VtIoTests); - wil::shared_event _shutdownEvent; - - TEST_CLASS_SETUP(VtIoTestsSetup) - { - _shutdownEvent.create(wil::EventOptions::ManualReset); - return true; - } - - TEST_METHOD_SETUP(VtIoMethodSetup) - { - _shutdownEvent.ResetEvent(); - return true; - } - // General Tests: TEST_METHOD(NoOpStartTest); TEST_METHOD(ModeParsingTest); @@ -133,28 +119,28 @@ void VtIoTests::DtorTestJustEngine() wil::unique_hfile hOutputFile; hOutputFile.reset(INVALID_HANDLE_VALUE); - auto pRenderer256 = new Xterm256Engine(std::move(hOutputFile), _shutdownEvent, p, SetUpViewport(), colorTable, colorTableSize); + auto pRenderer256 = new Xterm256Engine(std::move(hOutputFile), p, SetUpViewport(), colorTable, colorTableSize); Log::Comment(NoThrowString().Format(L"Made Xterm256Engine")); delete pRenderer256; Log::Comment(NoThrowString().Format(L"Deleted.")); hOutputFile.reset(INVALID_HANDLE_VALUE); - auto pRenderEngineXterm = new XtermEngine(std::move(hOutputFile), _shutdownEvent, p, SetUpViewport(), colorTable, colorTableSize, false); + auto pRenderEngineXterm = new XtermEngine(std::move(hOutputFile), p, SetUpViewport(), colorTable, colorTableSize, false); Log::Comment(NoThrowString().Format(L"Made XtermEngine")); delete pRenderEngineXterm; Log::Comment(NoThrowString().Format(L"Deleted.")); hOutputFile.reset(INVALID_HANDLE_VALUE); - auto pRenderEngineXtermAscii = new XtermEngine(std::move(hOutputFile), _shutdownEvent, p, SetUpViewport(), colorTable, colorTableSize, true); + auto pRenderEngineXtermAscii = new XtermEngine(std::move(hOutputFile), p, SetUpViewport(), colorTable, colorTableSize, true); Log::Comment(NoThrowString().Format(L"Made XtermEngine")); delete pRenderEngineXtermAscii; Log::Comment(NoThrowString().Format(L"Deleted.")); hOutputFile.reset(INVALID_HANDLE_VALUE); - auto pRenderEngineWinTelnet = new WinTelnetEngine(std::move(hOutputFile), _shutdownEvent, p, SetUpViewport(), colorTable, colorTableSize); + auto pRenderEngineWinTelnet = new WinTelnetEngine(std::move(hOutputFile), p, SetUpViewport(), colorTable, colorTableSize); Log::Comment(NoThrowString().Format(L"Made WinTelnetEngine")); delete pRenderEngineWinTelnet; Log::Comment(NoThrowString().Format(L"Deleted.")); @@ -187,7 +173,6 @@ void VtIoTests::DtorTestDeleteVtio() VtIo* vtio = new VtIo(); Log::Comment(NoThrowString().Format(L"Made VtIo")); vtio->_pVtRenderEngine = std::make_unique(std::move(hOutputFile), - _shutdownEvent, p, SetUpViewport(), colorTable, @@ -200,7 +185,6 @@ void VtIoTests::DtorTestDeleteVtio() vtio = new VtIo(); Log::Comment(NoThrowString().Format(L"Made VtIo")); vtio->_pVtRenderEngine = std::make_unique(std::move(hOutputFile), - _shutdownEvent, p, SetUpViewport(), colorTable, @@ -214,7 +198,6 @@ void VtIoTests::DtorTestDeleteVtio() vtio = new VtIo(); Log::Comment(NoThrowString().Format(L"Made VtIo")); vtio->_pVtRenderEngine = std::make_unique(std::move(hOutputFile), - _shutdownEvent, p, SetUpViewport(), colorTable, @@ -228,7 +211,6 @@ void VtIoTests::DtorTestDeleteVtio() vtio = new VtIo(); Log::Comment(NoThrowString().Format(L"Made VtIo")); vtio->_pVtRenderEngine = std::make_unique(std::move(hOutputFile), - _shutdownEvent, p, SetUpViewport(), colorTable, @@ -264,7 +246,6 @@ void VtIoTests::DtorTestStackAlloc() { VtIo vtio; vtio._pVtRenderEngine = std::make_unique(std::move(hOutputFile), - _shutdownEvent, p, SetUpViewport(), colorTable, @@ -275,7 +256,6 @@ void VtIoTests::DtorTestStackAlloc() { VtIo vtio; vtio._pVtRenderEngine = std::make_unique(std::move(hOutputFile), - _shutdownEvent, p, SetUpViewport(), colorTable, @@ -287,7 +267,6 @@ void VtIoTests::DtorTestStackAlloc() { VtIo vtio; vtio._pVtRenderEngine = std::make_unique(std::move(hOutputFile), - _shutdownEvent, p, SetUpViewport(), colorTable, @@ -299,7 +278,6 @@ void VtIoTests::DtorTestStackAlloc() { VtIo vtio; vtio._pVtRenderEngine = std::make_unique(std::move(hOutputFile), - _shutdownEvent, p, SetUpViewport(), colorTable, @@ -332,7 +310,6 @@ void VtIoTests::DtorTestStackAllocMany() hOutputFile.reset(INVALID_HANDLE_VALUE); VtIo vtio1; vtio1._pVtRenderEngine = std::make_unique(std::move(hOutputFile), - _shutdownEvent, p, SetUpViewport(), colorTable, @@ -341,7 +318,6 @@ void VtIoTests::DtorTestStackAllocMany() hOutputFile.reset(INVALID_HANDLE_VALUE); VtIo vtio2; vtio2._pVtRenderEngine = std::make_unique(std::move(hOutputFile), - _shutdownEvent, p, SetUpViewport(), colorTable, @@ -351,7 +327,6 @@ void VtIoTests::DtorTestStackAllocMany() hOutputFile.reset(INVALID_HANDLE_VALUE); VtIo vtio3; vtio3._pVtRenderEngine = std::make_unique(std::move(hOutputFile), - _shutdownEvent, p, SetUpViewport(), colorTable, @@ -361,7 +336,6 @@ void VtIoTests::DtorTestStackAllocMany() hOutputFile.reset(INVALID_HANDLE_VALUE); VtIo vtio4; vtio4._pVtRenderEngine = std::make_unique(std::move(hOutputFile), - _shutdownEvent, p, SetUpViewport(), colorTable, @@ -441,7 +415,6 @@ void VtIoTests::BasicAnonymousPipeOpeningWithSignalChannelTest() Log::Comment(L"\tinitializing vtio"); VtIo vtio; - vtio._doNotTerminate = true; // suspend process termination on teardown so we don't lose the test host process VERIFY_IS_FALSE(vtio.IsUsingVt()); VERIFY_ARE_EQUAL(nullptr, vtio._pPtySignalInputThread); VERIFY_SUCCEEDED(vtio._Initialize(inPipeReadSide.release(), outPipeWriteSide.release(), L"", signalPipeReadSide.release())); diff --git a/src/host/ut_host/VtRendererTests.cpp b/src/host/ut_host/VtRendererTests.cpp index 44f5a1809..de4767efe 100644 --- a/src/host/ut_host/VtRendererTests.cpp +++ b/src/host/ut_host/VtRendererTests.cpp @@ -59,8 +59,6 @@ class Microsoft::Console::Render::VtRendererTest { TEST_CLASS(VtRendererTest); - wil::shared_event _shutdownEvent; - TEST_CLASS_SETUP(ClassSetup) { // clang-format off @@ -81,9 +79,6 @@ class Microsoft::Console::Render::VtRendererTest g_ColorTable[14] = RGB(249, 241, 165); // Bright Yellow g_ColorTable[15] = RGB(242, 242, 242); // White // clang-format on - - _shutdownEvent.create(wil::EventOptions::ManualReset); - return true; } @@ -95,7 +90,6 @@ class Microsoft::Console::Render::VtRendererTest TEST_METHOD_SETUP(MethodSetup) { qExpectedInput.clear(); - _shutdownEvent.ResetEvent(); return true; } @@ -193,7 +187,7 @@ void VtRendererTest::TestPaint(VtEngine& engine, std::function pfn) void VtRendererTest::VtSequenceHelperTests() { wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - std::unique_ptr engine = std::make_unique(std::move(hFile), _shutdownEvent, p, SetUpViewport(), g_ColorTable, static_cast(COLOR_TABLE_SIZE)); + std::unique_ptr engine = std::make_unique(std::move(hFile), p, SetUpViewport(), g_ColorTable, static_cast(COLOR_TABLE_SIZE)); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); @@ -250,7 +244,7 @@ void VtRendererTest::VtSequenceHelperTests() void VtRendererTest::Xterm256TestInvalidate() { wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - std::unique_ptr engine = std::make_unique(std::move(hFile), _shutdownEvent, p, SetUpViewport(), g_ColorTable, static_cast(COLOR_TABLE_SIZE)); + std::unique_ptr engine = std::make_unique(std::move(hFile), p, SetUpViewport(), g_ColorTable, static_cast(COLOR_TABLE_SIZE)); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); @@ -385,7 +379,7 @@ void VtRendererTest::Xterm256TestInvalidate() void VtRendererTest::Xterm256TestColors() { wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - std::unique_ptr engine = std::make_unique(std::move(hFile), _shutdownEvent, p, SetUpViewport(), g_ColorTable, static_cast(COLOR_TABLE_SIZE)); + std::unique_ptr engine = std::make_unique(std::move(hFile), p, SetUpViewport(), g_ColorTable, static_cast(COLOR_TABLE_SIZE)); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); @@ -522,7 +516,7 @@ void VtRendererTest::Xterm256TestColors() void VtRendererTest::Xterm256TestCursor() { wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - std::unique_ptr engine = std::make_unique(std::move(hFile), _shutdownEvent, p, SetUpViewport(), g_ColorTable, static_cast(COLOR_TABLE_SIZE)); + std::unique_ptr engine = std::make_unique(std::move(hFile), p, SetUpViewport(), g_ColorTable, static_cast(COLOR_TABLE_SIZE)); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); @@ -674,7 +668,7 @@ void VtRendererTest::Xterm256TestExtendedAttributes() } wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - std::unique_ptr engine = std::make_unique(std::move(hFile), _shutdownEvent, p, SetUpViewport(), g_ColorTable, static_cast(COLOR_TABLE_SIZE)); + std::unique_ptr engine = std::make_unique(std::move(hFile), p, SetUpViewport(), g_ColorTable, static_cast(COLOR_TABLE_SIZE)); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); @@ -718,7 +712,7 @@ void VtRendererTest::Xterm256TestExtendedAttributes() void VtRendererTest::XtermTestInvalidate() { wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - std::unique_ptr engine = std::make_unique(std::move(hFile), _shutdownEvent, p, SetUpViewport(), g_ColorTable, static_cast(COLOR_TABLE_SIZE), false); + std::unique_ptr engine = std::make_unique(std::move(hFile), p, SetUpViewport(), g_ColorTable, static_cast(COLOR_TABLE_SIZE), false); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); @@ -852,7 +846,7 @@ void VtRendererTest::XtermTestInvalidate() void VtRendererTest::XtermTestColors() { wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - std::unique_ptr engine = std::make_unique(std::move(hFile), _shutdownEvent, p, SetUpViewport(), g_ColorTable, static_cast(COLOR_TABLE_SIZE), false); + std::unique_ptr engine = std::make_unique(std::move(hFile), p, SetUpViewport(), g_ColorTable, static_cast(COLOR_TABLE_SIZE), false); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); @@ -938,7 +932,7 @@ void VtRendererTest::XtermTestColors() void VtRendererTest::XtermTestCursor() { wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - std::unique_ptr engine = std::make_unique(std::move(hFile), _shutdownEvent, p, SetUpViewport(), g_ColorTable, static_cast(COLOR_TABLE_SIZE), false); + std::unique_ptr engine = std::make_unique(std::move(hFile), p, SetUpViewport(), g_ColorTable, static_cast(COLOR_TABLE_SIZE), false); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); @@ -1047,7 +1041,7 @@ void VtRendererTest::XtermTestCursor() void VtRendererTest::WinTelnetTestInvalidate() { wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - std::unique_ptr engine = std::make_unique(std::move(hFile), _shutdownEvent, p, SetUpViewport(), g_ColorTable, static_cast(COLOR_TABLE_SIZE)); + std::unique_ptr engine = std::make_unique(std::move(hFile), p, SetUpViewport(), g_ColorTable, static_cast(COLOR_TABLE_SIZE)); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); @@ -1119,7 +1113,7 @@ void VtRendererTest::WinTelnetTestInvalidate() void VtRendererTest::WinTelnetTestColors() { wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - std::unique_ptr engine = std::make_unique(std::move(hFile), _shutdownEvent, p, SetUpViewport(), g_ColorTable, static_cast(COLOR_TABLE_SIZE)); + std::unique_ptr engine = std::make_unique(std::move(hFile), p, SetUpViewport(), g_ColorTable, static_cast(COLOR_TABLE_SIZE)); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); @@ -1197,7 +1191,7 @@ void VtRendererTest::WinTelnetTestColors() void VtRendererTest::WinTelnetTestCursor() { wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - std::unique_ptr engine = std::make_unique(std::move(hFile), _shutdownEvent, p, SetUpViewport(), g_ColorTable, static_cast(COLOR_TABLE_SIZE)); + std::unique_ptr engine = std::make_unique(std::move(hFile), p, SetUpViewport(), g_ColorTable, static_cast(COLOR_TABLE_SIZE)); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); @@ -1277,7 +1271,7 @@ void VtRendererTest::WinTelnetTestCursor() void VtRendererTest::TestWrapping() { wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - std::unique_ptr engine = std::make_unique(std::move(hFile), _shutdownEvent, p, SetUpViewport(), g_ColorTable, static_cast(COLOR_TABLE_SIZE)); + std::unique_ptr engine = std::make_unique(std::move(hFile), p, SetUpViewport(), g_ColorTable, static_cast(COLOR_TABLE_SIZE)); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); @@ -1330,7 +1324,7 @@ void VtRendererTest::TestResize() { Viewport view = SetUpViewport(); wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - auto engine = std::make_unique(std::move(hFile), _shutdownEvent, p, view, g_ColorTable, static_cast(COLOR_TABLE_SIZE)); + auto engine = std::make_unique(std::move(hFile), p, view, g_ColorTable, static_cast(COLOR_TABLE_SIZE)); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); @@ -1367,7 +1361,7 @@ void VtRendererTest::TestCursorVisibility() { Viewport view = SetUpViewport(); wil::unique_hfile hFile = wil::unique_hfile(INVALID_HANDLE_VALUE); - auto engine = std::make_unique(std::move(hFile), _shutdownEvent, p, view, g_ColorTable, static_cast(COLOR_TABLE_SIZE)); + auto engine = std::make_unique(std::move(hFile), p, view, g_ColorTable, static_cast(COLOR_TABLE_SIZE)); auto pfn = std::bind(&VtRendererTest::WriteCallback, this, std::placeholders::_1, std::placeholders::_2); engine->SetTestCallback(pfn); diff --git a/src/inc/ITerminalOwner.hpp b/src/inc/ITerminalOwner.hpp new file mode 100644 index 000000000..b2ba1e574 --- /dev/null +++ b/src/inc/ITerminalOwner.hpp @@ -0,0 +1,33 @@ +/*++ +Copyright (c) Microsoft Corporation. +Licensed under the MIT license. + +Module Name: +- ITerminalOwner.hpp + +Abstract: +- Provides an abstraction for Closing the Input/Output objects of a terminal + connection. This is implemented by VtIo in the host, and is used by the + renderer to be able to tell the VtIo object that the renderer has had it's + pipe broken. + +Author(s): +- Mike Griese (migrie) 28 March 2018 +--*/ + +#pragma once + +namespace Microsoft::Console +{ + class ITerminalOwner + { + public: + virtual ~ITerminalOwner() = 0; + + virtual void CloseInput() = 0; + virtual void CloseOutput() = 0; + }; + + // See docs/virtual-dtors.md for an explanation of why this is weird. + inline Microsoft::Console::ITerminalOwner::~ITerminalOwner() {} +} diff --git a/src/inc/LibraryIncludes.h b/src/inc/LibraryIncludes.h index 97e6bd66e..91308664a 100644 --- a/src/inc/LibraryIncludes.h +++ b/src/inc/LibraryIncludes.h @@ -21,7 +21,6 @@ #include #include #include -#include #include #include #include diff --git a/src/interactivity/onecore/ConIoSrvComm.cpp b/src/interactivity/onecore/ConIoSrvComm.cpp index e1a4d350d..abd9c47eb 100644 --- a/src/interactivity/onecore/ConIoSrvComm.cpp +++ b/src/interactivity/onecore/ConIoSrvComm.cpp @@ -454,7 +454,7 @@ VOID ConIoSrvComm::CleanupForHeadless(const NTSTATUS status) ServiceLocator::LocateGlobals().ntstatusConsoleInputInitStatus = status; // Signal that input is ready to go. - ServiceLocator::LocateGlobals().consoleInputInitializedEvent.SetEvent(); + ServiceLocator::LocateGlobals().hConsoleInputInitEvent.SetEvent(); _fIsInputInitialized = true; } diff --git a/src/interactivity/onecore/ConsoleInputThread.cpp b/src/interactivity/onecore/ConsoleInputThread.cpp index 2dd14e825..91579bf3a 100644 --- a/src/interactivity/onecore/ConsoleInputThread.cpp +++ b/src/interactivity/onecore/ConsoleInputThread.cpp @@ -57,7 +57,7 @@ DWORD WINAPI ConsoleInputThreadProcOneCore(LPVOID /*lpParam*/) } globals.ntstatusConsoleInputInitStatus = Status; - globals.consoleInputInitializedEvent.SetEvent(); + globals.hConsoleInputInitEvent.SetEvent(); if (NT_SUCCESS(Status)) { diff --git a/src/interactivity/win32/windowio.cpp b/src/interactivity/win32/windowio.cpp index b0b51a8cf..921f3f011 100644 --- a/src/interactivity/win32/windowio.cpp +++ b/src/interactivity/win32/windowio.cpp @@ -61,6 +61,7 @@ ULONG ConvertMouseButtonState(_In_ ULONG Flag, _In_ ULONG State) VOID SetConsoleWindowOwner(const HWND hwnd, _Inout_opt_ ConsoleProcessHandle* pProcessData) { const CONSOLE_INFORMATION& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); + FAIL_FAST_IF(!(gci.IsConsoleLocked())); DWORD dwProcessId; DWORD dwThreadId; @@ -993,10 +994,7 @@ DWORD WINAPI ConsoleInputThreadProcWin32(LPVOID /*lpParameter*/) { InitEnvironmentVariables(); - // When the setup event is triggered, the I/O thread has told us that it is - // officially holding the global lock on our behalf and we're free to setup. - ServiceLocator::LocateGlobals().consoleInputSetupEvent.wait(); - + LockConsole(); HHOOK hhook = nullptr; NTSTATUS Status = STATUS_SUCCESS; @@ -1015,18 +1013,16 @@ DWORD WINAPI ConsoleInputThreadProcWin32(LPVOID /*lpParameter*/) ServiceLocator::LocatePseudoWindow(); } - // Now we must pass back virtual ownership of the global lock by telling the I/O - // thread that we're done and what our status is. - ServiceLocator::LocateGlobals().ntstatusConsoleInputInitStatus = Status; - ServiceLocator::LocateGlobals().consoleInputInitializedEvent.SetEvent(); - - // If not successful, end the thread by returning the status. - // If successful, proceed down below to the message pump loop. + UnlockConsole(); if (!NT_SUCCESS(Status)) { + ServiceLocator::LocateGlobals().ntstatusConsoleInputInitStatus = Status; + ServiceLocator::LocateGlobals().hConsoleInputInitEvent.SetEvent(); return Status; } + ServiceLocator::LocateGlobals().hConsoleInputInitEvent.SetEvent(); + for (;;) { MSG msg; diff --git a/src/interactivity/win32/windowproc.cpp b/src/interactivity/win32/windowproc.cpp index 2fb35515b..9292bef98 100644 --- a/src/interactivity/win32/windowproc.cpp +++ b/src/interactivity/win32/windowproc.cpp @@ -68,32 +68,7 @@ using namespace Microsoft::Console::Types; LRESULT Status = 0; BOOL Unlock = TRUE; - // If we're during the initial allocation of the console, then the I/O thread is servicing a - // Console Connection Request under lock as the input thread is created on the first connection to a client. - // Normally we'd take the lock here at the top of the window proc to ensure consistency while servicing messages, - // but because the I/O thread needs to hold the lock for the duration of - // servicing the connection request, we have a wink-nudge agreement here with the I/O thread about the lock. - // As long as the input setup event exists and is signaled, we've been told that the I/O thread has the lock - // and is waiting for us to respond. - // As long as the input initialized event exists and is unsignaled, we haven't told the I/O thread that we're done - // yet and it can resume use of the lock. - // So under these conditions where we are assured the I/O thread is holding the lock on our behalf and waiting for us... - // skip the lock/unlock behavior down this entire procedure and perform the messages needed to get things set up - // knowing we have exclusive reign over the console variables. - if (g.consoleInputSetupEvent && g.consoleInputSetupEvent.is_signaled() && - g.consoleInputInitializedEvent && !g.consoleInputInitializedEvent.is_signaled()) - { - Unlock = FALSE; - } - - // Under normal conditions, Unlock is TRUE at the top and we will need to take the lock - // to process messages. - // Under special init conditions, someone else holds the lock for us and we can skip all lock/unlock behavior - // by checking the Unlock variable. - if (Unlock) - { - LockConsole(); - } + LockConsole(); SCREEN_INFORMATION& ScreenInfo = GetScreenInfo(); if (hWnd == nullptr) // TODO: this might not be possible anymore @@ -108,10 +83,7 @@ using namespace Microsoft::Console::Types; Status = DefWindowProcW(hWnd, Message, wParam, lParam); } - if (Unlock) - { - UnlockConsole(); - } + UnlockConsole(); return Status; } @@ -242,10 +214,7 @@ using namespace Microsoft::Console::Types; pSuggestionSize->cy = RECT_HEIGHT(&rectProposed); // Format our final suggestion for consumption. - if (Unlock) - { - UnlockConsole(); - } + UnlockConsole(); return TRUE; } @@ -516,11 +485,8 @@ using namespace Microsoft::Console::Types; { HMENU hHeirMenu = Menu::s_GetHeirMenuHandle(); - if (Unlock) - { - Unlock = FALSE; - UnlockConsole(); - } + Unlock = FALSE; + UnlockConsole(); TrackPopupMenuEx(hHeirMenu, TPM_RIGHTBUTTON | (GetSystemMetrics(SM_MENUDROPALIGNMENT) == 0 ? TPM_LEFTALIGN : TPM_RIGHTALIGN), @@ -542,11 +508,8 @@ using namespace Microsoft::Console::Types; switch (wParam & 0x00FF) { case HTCAPTION: - if (Unlock) - { - UnlockConsole(); - Unlock = FALSE; - } + UnlockConsole(); + Unlock = FALSE; SetActiveWindow(hWnd); SendMessageTimeoutW(hWnd, WM_SYSCOMMAND, SC_MOVE | wParam, lParam, SMTO_NORMAL, INFINITE, nullptr); break; @@ -712,11 +675,8 @@ using namespace Microsoft::Console::Types; case CM_BEEP: { - if (Unlock) - { - UnlockConsole(); - Unlock = FALSE; - } + UnlockConsole(); + Unlock = FALSE; // Don't fall back to Beep() on win32 systems -- if the user configures their system for no sound, we should // respect that. diff --git a/src/renderer/vt/WinTelnetEngine.cpp b/src/renderer/vt/WinTelnetEngine.cpp index c273232fd..a2a928380 100644 --- a/src/renderer/vt/WinTelnetEngine.cpp +++ b/src/renderer/vt/WinTelnetEngine.cpp @@ -10,12 +10,11 @@ using namespace Microsoft::Console::Render; using namespace Microsoft::Console::Types; WinTelnetEngine::WinTelnetEngine(_In_ wil::unique_hfile hPipe, - wil::shared_event shutdownEvent, const IDefaultColorProvider& colorProvider, const Viewport initialViewport, _In_reads_(cColorTable) const COLORREF* const ColorTable, const WORD cColorTable) : - VtEngine(std::move(hPipe), shutdownEvent, colorProvider, initialViewport), + VtEngine(std::move(hPipe), colorProvider, initialViewport), _ColorTable(ColorTable), _cColorTable(cColorTable) { diff --git a/src/renderer/vt/WinTelnetEngine.hpp b/src/renderer/vt/WinTelnetEngine.hpp index 8c27c2218..bbaeb6aea 100644 --- a/src/renderer/vt/WinTelnetEngine.hpp +++ b/src/renderer/vt/WinTelnetEngine.hpp @@ -24,7 +24,6 @@ namespace Microsoft::Console::Render { public: WinTelnetEngine(_In_ wil::unique_hfile hPipe, - wil::shared_event shutdownEvent, const Microsoft::Console::IDefaultColorProvider& colorProvider, const Microsoft::Console::Types::Viewport initialViewport, _In_reads_(cColorTable) const COLORREF* const ColorTable, diff --git a/src/renderer/vt/Xterm256Engine.cpp b/src/renderer/vt/Xterm256Engine.cpp index 14f98fdfc..bc1ea4a38 100644 --- a/src/renderer/vt/Xterm256Engine.cpp +++ b/src/renderer/vt/Xterm256Engine.cpp @@ -9,12 +9,11 @@ using namespace Microsoft::Console::Render; using namespace Microsoft::Console::Types; Xterm256Engine::Xterm256Engine(_In_ wil::unique_hfile hPipe, - wil::shared_event shutdownEvent, const IDefaultColorProvider& colorProvider, const Viewport initialViewport, _In_reads_(cColorTable) const COLORREF* const ColorTable, const WORD cColorTable) : - XtermEngine(std::move(hPipe), shutdownEvent, colorProvider, initialViewport, ColorTable, cColorTable, false), + XtermEngine(std::move(hPipe), colorProvider, initialViewport, ColorTable, cColorTable, false), _lastExtendedAttrsState{ ExtendedAttributes::Normal } { } diff --git a/src/renderer/vt/Xterm256Engine.hpp b/src/renderer/vt/Xterm256Engine.hpp index 729ed1243..03817fa77 100644 --- a/src/renderer/vt/Xterm256Engine.hpp +++ b/src/renderer/vt/Xterm256Engine.hpp @@ -24,7 +24,6 @@ namespace Microsoft::Console::Render { public: Xterm256Engine(_In_ wil::unique_hfile hPipe, - wil::shared_event shutdownEvent, const Microsoft::Console::IDefaultColorProvider& colorProvider, const Microsoft::Console::Types::Viewport initialViewport, _In_reads_(cColorTable) const COLORREF* const ColorTable, diff --git a/src/renderer/vt/XtermEngine.cpp b/src/renderer/vt/XtermEngine.cpp index b48dab303..88eda0389 100644 --- a/src/renderer/vt/XtermEngine.cpp +++ b/src/renderer/vt/XtermEngine.cpp @@ -10,13 +10,12 @@ using namespace Microsoft::Console::Render; using namespace Microsoft::Console::Types; XtermEngine::XtermEngine(_In_ wil::unique_hfile hPipe, - wil::shared_event shutdownEvent, const IDefaultColorProvider& colorProvider, const Viewport initialViewport, _In_reads_(cColorTable) const COLORREF* const ColorTable, const WORD cColorTable, const bool fUseAsciiOnly) : - VtEngine(std::move(hPipe), shutdownEvent, colorProvider, initialViewport), + VtEngine(std::move(hPipe), colorProvider, initialViewport), _ColorTable(ColorTable), _cColorTable(cColorTable), _fUseAsciiOnly(fUseAsciiOnly), diff --git a/src/renderer/vt/XtermEngine.hpp b/src/renderer/vt/XtermEngine.hpp index 2a482501d..6be72d6c7 100644 --- a/src/renderer/vt/XtermEngine.hpp +++ b/src/renderer/vt/XtermEngine.hpp @@ -28,7 +28,6 @@ namespace Microsoft::Console::Render { public: XtermEngine(_In_ wil::unique_hfile hPipe, - wil::shared_event shutdownEvent, const Microsoft::Console::IDefaultColorProvider& colorProvider, const Microsoft::Console::Types::Viewport initialViewport, _In_reads_(cColorTable) const COLORREF* const ColorTable, diff --git a/src/renderer/vt/paint.cpp b/src/renderer/vt/paint.cpp index 2523fefaa..920dc86ae 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 (_shutdownEvent.is_signaled()) + if (_pipeBroken) { return S_FALSE; } diff --git a/src/renderer/vt/state.cpp b/src/renderer/vt/state.cpp index f268b15ed..31cca05d4 100644 --- a/src/renderer/vt/state.cpp +++ b/src/renderer/vt/state.cpp @@ -25,12 +25,10 @@ const COORD VtEngine::INVALID_COORDS = { -1, -1 }; // - // Return Value: // - An instance of a Renderer. -VtEngine::VtEngine(wil::unique_hfile pipe, - wil::shared_event shutdownEvent, +VtEngine::VtEngine(_In_ wil::unique_hfile pipe, const IDefaultColorProvider& colorProvider, const Viewport initialViewport) : RenderEngineBase(), - _shutdownEvent(shutdownEvent), _hFile(std::move(pipe)), _colorProvider(colorProvider), _LastFG(INVALID_COLOR), @@ -51,6 +49,9 @@ VtEngine::VtEngine(wil::unique_hfile pipe, _circled(false), _firstPaint(true), _skipCursor(false), + _pipeBroken(false), + _exitResult{ S_OK }, + _terminalOwner{ nullptr }, _newBottomLine{ false }, _deferredCursorPos{ INVALID_COORDS }, _inResizeRequest{ false }, @@ -59,42 +60,10 @@ VtEngine::VtEngine(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, !_shutdownEvent); #else // member is only defined when UNIT_TESTING is. _usingTestCallback = false; #endif - - // Set up a background thread to wait until the shared shutdown event is called and then execute cleanup tasks. - _shutdownWatchdog = std::async(std::launch::async, [=] { - _shutdownEvent.wait(); - - // When someone calls the _Flush method, they will go into a potentially blocking WriteFile operation. - // Before they do that, they'll store their thread ID here so we can get them unstuck should we be shutting down. - if (const auto threadId = _blockedThreadId.load()) - { - // If we indeed had a valid thread ID meaning someone is blocked on a WriteFile operation, - // then let's open a handle to their thread. We need the standard read/write privileges to see - // what their thread is up to (it will not work without these) and also the Terminate privilege to - // unstick them. - wil::unique_handle threadHandle(OpenThread(STANDARD_RIGHTS_ALL | THREAD_TERMINATE, FALSE, threadId)); - LOG_LAST_ERROR_IF_NULL(threadHandle.get()); - if (threadHandle) - { - // Presuming we got all the way to acquiring the blocked thread's handle, call the OS function that - // will unstick any thread that is otherwise permanently blocked on a synchronous operation. - LOG_IF_WIN32_BOOL_FALSE(CancelSynchronousIo(threadHandle.get())); - } - } - }); -} - -VtEngine::~VtEngine() -{ - if (_shutdownEvent) - { - _shutdownEvent.SetEvent(); - } } // Method Description: @@ -135,20 +104,19 @@ VtEngine::~VtEngine() } #endif - if (!_shutdownEvent.is_signaled()) + if (!_pipeBroken) { - // Stash the current thread ID before we go into the potentially blocking synchronous write file operation. - // This will let the shutdown watchdog thread break us out of the stuck state should a shutdown event - // occur while we're still waiting for the WriteFile to complete. - _blockedThreadId.store(GetCurrentThreadId()); bool fSuccess = !!WriteFile(_hFile.get(), _buffer.data(), static_cast(_buffer.size()), nullptr, nullptr); - _blockedThreadId.store(0); // When done, clear the thread ID. - _buffer.clear(); if (!fSuccess) { - _shutdownEvent.SetEvent(); - RETURN_LAST_ERROR(); + _exitResult = HRESULT_FROM_WIN32(GetLastError()); + _pipeBroken = true; + if (_terminalOwner) + { + _terminalOwner->CloseOutput(); + } + return _exitResult; } } @@ -431,6 +399,11 @@ bool VtEngine::_AllIsInvalid() const return S_OK; } +void VtEngine::SetTerminalOwner(Microsoft::Console::ITerminalOwner* const terminalOwner) +{ + _terminalOwner = terminalOwner; +} + // Method Description: // - sends a sequence to request the end terminal to tell us the // cursor position. The terminal will reply back on the vt input handle. diff --git a/src/renderer/vt/vtrenderer.hpp b/src/renderer/vt/vtrenderer.hpp index d2c7804a9..c46295858 100644 --- a/src/renderer/vt/vtrenderer.hpp +++ b/src/renderer/vt/vtrenderer.hpp @@ -18,6 +18,7 @@ Author(s): #include "../inc/RenderEngineBase.hpp" #include "../../inc/IDefaultColorProvider.hpp" #include "../../inc/ITerminalOutputConnection.hpp" +#include "../../inc/ITerminalOwner.hpp" #include "../../types/inc/Viewport.hpp" #include "tracing.hpp" #include @@ -32,12 +33,11 @@ namespace Microsoft::Console::Render static const size_t ERASE_CHARACTER_STRING_LENGTH = 8; static const COORD INVALID_COORDS; - VtEngine(wil::unique_hfile hPipe, - wil::shared_event shutdownEvent, + VtEngine(_In_ wil::unique_hfile hPipe, const Microsoft::Console::IDefaultColorProvider& colorProvider, const Microsoft::Console::Types::Viewport initialViewport); - ~VtEngine() override; + virtual ~VtEngine() override = default; [[nodiscard]] HRESULT InvalidateSelection(const std::vector& rectangles) noexcept override; [[nodiscard]] virtual HRESULT InvalidateScroll(const COORD* const pcoordDelta) noexcept = 0; @@ -93,14 +93,11 @@ namespace Microsoft::Console::Render [[nodiscard]] virtual HRESULT WriteTerminalW(const std::wstring& str) noexcept = 0; + void SetTerminalOwner(Microsoft::Console::ITerminalOwner* const terminalOwner); void BeginResizeRequest(); void EndResizeRequest(); protected: - wil::shared_event _shutdownEvent; - std::future _shutdownWatchdog; - std::atomic _blockedThreadId; - wil::unique_hfile _hFile; std::string _buffer; @@ -132,6 +129,10 @@ namespace Microsoft::Console::Render bool _newBottomLine; COORD _deferredCursorPos; + bool _pipeBroken; + HRESULT _exitResult; + Microsoft::Console::ITerminalOwner* _terminalOwner; + Microsoft::Console::VirtualTerminal::RenderTracing _trace; bool _inResizeRequest{ false }; diff --git a/src/server/DeviceComm.cpp b/src/server/DeviceComm.cpp index 0acda57ba..10cee37f2 100644 --- a/src/server/DeviceComm.cpp +++ b/src/server/DeviceComm.cpp @@ -5,8 +5,7 @@ #include "DeviceComm.h" DeviceComm::DeviceComm(_In_ HANDLE Server) : - _Server(Server), - _shutdown(false) + _Server(Server) { THROW_HR_IF(E_HANDLE, Server == INVALID_HANDLE_VALUE); } @@ -15,13 +14,6 @@ DeviceComm::~DeviceComm() { } -// Routine Description: -// - Intentionally shut down our communications channel -void DeviceComm::Shutdown() -{ - _shutdown = true; -} - // Routine Description: // - Needs to be called once per server session and typically as the absolute first operation. // - This sets up the driver with the input event that it will need to coordinate with when client @@ -49,17 +41,6 @@ void DeviceComm::Shutdown() [[nodiscard]] HRESULT DeviceComm::ReadIo(_In_opt_ PCONSOLE_API_MSG const pReplyMsg, _Out_ CONSOLE_API_MSG* const pMessage) const { - // If we've been told to shutdown, we should allow a reply to go through to finish things off, - // but no more reads of new messages are allowed. - if (_shutdown) - { - if (pReplyMsg) - { - LOG_IF_FAILED(CompleteIo(&pReplyMsg->Complete)); - } - return E_APPLICATION_EXITING; - } - HRESULT hr = _CallIoctl(IOCTL_CONDRV_READ_IO, pReplyMsg == nullptr ? nullptr : &pReplyMsg->Complete, pReplyMsg == nullptr ? 0 : sizeof(pReplyMsg->Complete), diff --git a/src/server/DeviceComm.h b/src/server/DeviceComm.h index adaf186e8..ec78caca5 100644 --- a/src/server/DeviceComm.h +++ b/src/server/DeviceComm.h @@ -36,8 +36,6 @@ public: [[nodiscard]] HRESULT AllowUIAccess() const; - void Shutdown(); - private: [[nodiscard]] HRESULT _CallIoctl(_In_ DWORD dwIoControlCode, _In_reads_bytes_opt_(cbInBufferSize) PVOID pInBuffer, @@ -46,5 +44,4 @@ private: _In_ DWORD cbOutBufferSize) const; wil::unique_handle _Server; - bool _shutdown; }; diff --git a/src/server/ProcessList.cpp b/src/server/ProcessList.cpp index b46b9a707..f488b35e7 100644 --- a/src/server/ProcessList.cpp +++ b/src/server/ProcessList.cpp @@ -92,9 +92,6 @@ void ConsoleProcessList::FreeProcessData(_In_ ConsoleProcessHandle* const pProce _processes.remove(pProcessData); - // Attempt to dispatch events to registered listeners. - _NotifyOnLastFree(); - delete pProcessData; } @@ -332,19 +329,6 @@ bool ConsoleProcessList::IsEmpty() const return _processes.empty(); } -// Routine Description: -// - Gives us an event that we should notify when the last process is removed from this list -// - NOTE: This is a function callback and not an event so the notification of state can occur under the -// same global lock state that is required to add/remove client processes from the list. -// Arguments: -// - func - A function to call while we're removing the last process. -// Return Value: -// - -void ConsoleProcessList::RegisterForNotifyOnLastFree(std::function func) -{ - _notifyOnLastFree.emplace_back(func); -} - // Routine Description: // - Requests the OS allow the console to set one of its child processes as the foreground window // Arguments: @@ -356,20 +340,3 @@ void ConsoleProcessList::_ModifyProcessForegroundRights(const HANDLE hProcess, c { LOG_IF_NTSTATUS_FAILED(ServiceLocator::LocateConsoleControl()->SetForeground(hProcess, fForeground)); } - -// Routine Description: -// - Attempts to notify anyone listening for when the last client process is disconnecting. -// Arguments: -// - -// Return Value: -// - -void ConsoleProcessList::_NotifyOnLastFree() const -{ - if (_processes.empty()) - { - for (auto& func : _notifyOnLastFree) - { - func(); - } - } -} diff --git a/src/server/ProcessList.h b/src/server/ProcessList.h index d218cc9b7..2b3e2af69 100644 --- a/src/server/ProcessList.h +++ b/src/server/ProcessList.h @@ -58,13 +58,8 @@ public: bool IsEmpty() const; - void RegisterForNotifyOnLastFree(std::function func); - private: std::list _processes; - std::vector> _notifyOnLastFree; - - void _NotifyOnLastFree() const; void _ModifyProcessForegroundRights(const HANDLE hProcess, const bool fForeground) const; };