diff --git a/src/host/CursorBlinker.cpp b/src/host/CursorBlinker.cpp index bab71cccbda..939efeaed58 100644 --- a/src/host/CursorBlinker.cpp +++ b/src/host/CursorBlinker.cpp @@ -10,22 +10,32 @@ using namespace Microsoft::Console; using namespace Microsoft::Console::Interactivity; using namespace Microsoft::Console::Render; +static void CALLBACK CursorTimerRoutineWrapper(_Inout_ PTP_CALLBACK_INSTANCE /*Instance*/, _Inout_opt_ PVOID /*Context*/, _Inout_ PTP_TIMER /*Timer*/) +{ + CONSOLE_INFORMATION& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); + + // There's a slight race condition here. + // CreateThreadpoolTimer callbacks may be scheduled even after they were canceled. + // But I'm not too concerned that this will lead to issues at the time of writing, + // as CursorBlinker is allocated as a static variable through the Globals class. + // It'd be nice to fix this, but realistically it'll likely not lead to issues. + gci.LockConsole(); + gci.GetCursorBlinker().TimerRoutine(gci.GetActiveOutputBuffer()); + gci.UnlockConsole(); +} + CursorBlinker::CursorBlinker() : - _hCaretBlinkTimer(INVALID_HANDLE_VALUE), - _hCaretBlinkTimerQueue(THROW_LAST_ERROR_IF_NULL(CreateTimerQueue())), + _timer(THROW_LAST_ERROR_IF_NULL(CreateThreadpoolTimer(&CursorTimerRoutineWrapper, nullptr, nullptr))), _uCaretBlinkTime(INFINITE) // default to no blink { } CursorBlinker::~CursorBlinker() { - if (_hCaretBlinkTimerQueue) - { - DeleteTimerQueueEx(_hCaretBlinkTimerQueue, INVALID_HANDLE_VALUE); - } + KillCaretTimer(); } -void CursorBlinker::UpdateSystemMetrics() +void CursorBlinker::UpdateSystemMetrics() noexcept { // This can be -1 in a TS session _uCaretBlinkTime = ServiceLocator::LocateSystemConfigurationProvider()->GetCaretBlinkTime(); @@ -37,7 +47,7 @@ void CursorBlinker::UpdateSystemMetrics() renderSettings.SetRenderMode(RenderSettings::Mode::BlinkAllowed, animationsEnabled && _uCaretBlinkTime != INFINITE); } -void CursorBlinker::SettingsChanged() +void CursorBlinker::SettingsChanged() noexcept { DWORD const dwCaretBlinkTime = ServiceLocator::LocateSystemConfigurationProvider()->GetCaretBlinkTime(); @@ -49,12 +59,12 @@ void CursorBlinker::SettingsChanged() } } -void CursorBlinker::FocusEnd() +void CursorBlinker::FocusEnd() const noexcept { KillCaretTimer(); } -void CursorBlinker::FocusStart() +void CursorBlinker::FocusStart() const noexcept { SetCaretTimer(); } @@ -66,7 +76,7 @@ void CursorBlinker::FocusStart() // - ScreenInfo - reference to screen info structure. // Return Value: // - -void CursorBlinker::TimerRoutine(SCREEN_INFORMATION& ScreenInfo) +void CursorBlinker::TimerRoutine(SCREEN_INFORMATION& ScreenInfo) const noexcept { auto& buffer = ScreenInfo.GetTextBuffer(); auto& cursor = buffer.GetCursor(); @@ -146,100 +156,23 @@ void CursorBlinker::TimerRoutine(SCREEN_INFORMATION& ScreenInfo) Scrolling::s_ScrollIfNecessary(ScreenInfo); } -void CALLBACK CursorTimerRoutineWrapper(_In_ PVOID /* lpParam */, _In_ BOOLEAN /* TimerOrWaitFired */) -{ - // Suppose the following sequence of events takes place: - // - // 1. The user resizes the console; - // 2. The console acquires the console lock; - // 3. The current SCREEN_INFORMATION instance is deleted; - // 4. This causes the current Cursor instance to be deleted, too; - // 5. The Cursor's destructor is called; - // => Somewhere between 1 and 5, the timer fires: - // Timer queue timer callbacks execute asynchronously with respect to - // the UI thread under which the numbered steps are taking place. - // Because the callback touches console state, it needs to acquire the - // console lock. But what if the timer callback fires at just the right - // time such that 2 has already acquired the lock? - // 6. The Cursor's destructor deletes the timer queue and thereby destroys - // the timer queue timer used for blinking. However, because this - // timer's callback modifies console state, it is prudent to not - // continue the destruction if the callback has already started but has - // not yet finished. Therefore, the destructor waits for the callback to - // finish executing. - // => Meanwhile, the callback just happens to be stuck waiting for the - // console lock acquired in step 2. Since the destructor is waiting on - // the callback to complete, and the callback is waiting on the lock, - // which will only be released way after the Cursor instance is deleted, - // the console has now deadlocked. - // - // As a solution, skip the blink if the console lock is already being held. - // Note that critical sections to not have a waitable synchronization - // object unless there readily is contention on it. As a result, if we - // wanted to wait until the lock became available under the condition of - // not being destroyed, things get too complicated. - CONSOLE_INFORMATION& gci = ServiceLocator::LocateGlobals().getConsoleInformation(); - - if (gci.TryLockConsole() != false) - { - // Cursor& cursor = gci.GetActiveOutputBuffer().GetTextBuffer().GetCursor(); - gci.GetCursorBlinker().TimerRoutine(gci.GetActiveOutputBuffer()); - - // This was originally just UnlockConsole, not CONSOLE_INFORMATION::UnlockConsole - // Is there a reason it would need to be the global version? - gci.UnlockConsole(); - } -} - // Routine Description: // - If guCaretBlinkTime is -1, we don't want to blink the caret. However, we // need to make sure it gets drawn, so we'll set a short timer. When that // goes off, we'll hit CursorTimerRoutine, and it'll do the right thing if // guCaretBlinkTime is -1. -void CursorBlinker::SetCaretTimer() +void CursorBlinker::SetCaretTimer() const noexcept { - static const DWORD dwDefTimeout = 0x212; - - KillCaretTimer(); + using filetime_duration = std::chrono::duration>; + static constexpr DWORD dwDefTimeout = 0x212; - if (_hCaretBlinkTimer == INVALID_HANDLE_VALUE) - { - bool bRet = true; - DWORD dwEffectivePeriod = _uCaretBlinkTime == -1 ? dwDefTimeout : _uCaretBlinkTime; - - bRet = CreateTimerQueueTimer(&_hCaretBlinkTimer, - _hCaretBlinkTimerQueue, - CursorTimerRoutineWrapper, - this, - dwEffectivePeriod, - dwEffectivePeriod, - 0); - - LOG_LAST_ERROR_IF(!bRet); - } + const auto periodInMS = _uCaretBlinkTime == -1 ? dwDefTimeout : _uCaretBlinkTime; + // The FILETIME struct measures time in 100ns steps. 10000 thus equals 1ms. + auto periodInFiletime = -static_cast(periodInMS) * 10000; + SetThreadpoolTimer(_timer.get(), reinterpret_cast(&periodInFiletime), periodInMS, 0); } -void CursorBlinker::KillCaretTimer() +void CursorBlinker::KillCaretTimer() const noexcept { - if (_hCaretBlinkTimer != INVALID_HANDLE_VALUE) - { - bool bRet = true; - - bRet = DeleteTimerQueueTimer(_hCaretBlinkTimerQueue, - _hCaretBlinkTimer, - nullptr); - - // According to https://msdn.microsoft.com/en-us/library/windows/desktop/ms682569(v=vs.85).aspx - // A failure to delete the timer with the LastError being ERROR_IO_PENDING means that the timer is - // currently in use and will get cleaned up when released. Delete should not be called again. - // We treat that case as a success. - if (!bRet && GetLastError() != ERROR_IO_PENDING) - { - LOG_LAST_ERROR(); - } - else - { - _hCaretBlinkTimer = INVALID_HANDLE_VALUE; - } - } + SetThreadpoolTimer(_timer.get(), nullptr, 0, 0); } diff --git a/src/host/CursorBlinker.hpp b/src/host/CursorBlinker.hpp index 6f8ca4a88a3..ec4443951e5 100644 --- a/src/host/CursorBlinker.hpp +++ b/src/host/CursorBlinker.hpp @@ -20,20 +20,18 @@ namespace Microsoft::Console CursorBlinker(); ~CursorBlinker(); - void FocusStart(); - void FocusEnd(); + void FocusStart() const noexcept; + void FocusEnd() const noexcept; - void UpdateSystemMetrics(); - void SettingsChanged(); - void TimerRoutine(SCREEN_INFORMATION& ScreenInfo); + void UpdateSystemMetrics() noexcept; + void SettingsChanged() noexcept; + void TimerRoutine(SCREEN_INFORMATION& ScreenInfo) const noexcept; private: - // These use Timer Queues: - // https://msdn.microsoft.com/en-us/library/windows/desktop/ms687003(v=vs.85).aspx - HANDLE _hCaretBlinkTimer; // timer used to periodically blink the cursor - HANDLE _hCaretBlinkTimerQueue; // timer queue where the blink timer lives + void SetCaretTimer() const noexcept; + void KillCaretTimer() const noexcept; + + wil::unique_threadpool_timer_nowait _timer; UINT _uCaretBlinkTime; - void SetCaretTimer(); - void KillCaretTimer(); }; } diff --git a/src/host/consoleInformation.cpp b/src/host/consoleInformation.cpp index 66124615eb8..9489ff59094 100644 --- a/src/host/consoleInformation.cpp +++ b/src/host/consoleInformation.cpp @@ -11,6 +11,8 @@ #include "../interactivity/inc/ServiceLocator.hpp" #include "../types/inc/convert.hpp" +#include + using Microsoft::Console::Interactivity::ServiceLocator; using Microsoft::Console::VirtualTerminal::VtIo; @@ -43,42 +45,52 @@ CONSOLE_INFORMATION::CONSOLE_INFORMATION() : { ZeroMemory((void*)&CPInfo, sizeof(CPInfo)); ZeroMemory((void*)&OutputCPInfo, sizeof(OutputCPInfo)); - InitializeCriticalSection(&_csConsoleLock); -} - -CONSOLE_INFORMATION::~CONSOLE_INFORMATION() -{ - DeleteCriticalSection(&_csConsoleLock); } -bool CONSOLE_INFORMATION::IsConsoleLocked() const +// Access to thread-local variables is thread-safe, because each thread +// gets its own copy of this variable with a default value of 0. +// +// Whenever we want to acquire the lock we increment recursionCount and on +// each release we decrement it again. We can then make the lock safe for +// reentrancy by only acquiring/releasing the lock if the recursionCount is 0. +// In a sense, recursionCount is counting the actual function +// call recursion depth of the caller. This works as long as +// the caller makes sure to properly call Unlock() once for each Lock(). +static thread_local ULONG recursionCount = 0; +static til::ticket_lock lock; + +bool CONSOLE_INFORMATION::IsConsoleLocked() { - // The critical section structure's OwningThread field contains the ThreadId despite having the HANDLE type. - // This requires us to hard cast the ID to compare. - return _csConsoleLock.OwningThread == (HANDLE)GetCurrentThreadId(); + return recursionCount != 0; } #pragma prefast(suppress : 26135, "Adding lock annotation spills into entire project. Future work.") void CONSOLE_INFORMATION::LockConsole() { - EnterCriticalSection(&_csConsoleLock); -} - -#pragma prefast(suppress : 26135, "Adding lock annotation spills into entire project. Future work.") -bool CONSOLE_INFORMATION::TryLockConsole() -{ - return !!TryEnterCriticalSection(&_csConsoleLock); + // See description of recursionCount a few lines above. + const auto rc = ++recursionCount; + FAIL_FAST_IF(rc == 0); + if (rc == 1) + { + lock.lock(); + } } #pragma prefast(suppress : 26135, "Adding lock annotation spills into entire project. Future work.") void CONSOLE_INFORMATION::UnlockConsole() { - LeaveCriticalSection(&_csConsoleLock); + // See description of recursionCount a few lines above. + const auto rc = --recursionCount; + FAIL_FAST_IF(rc == ULONG_MAX); + if (rc == 0) + { + lock.unlock(); + } } ULONG CONSOLE_INFORMATION::GetCSRecursionCount() { - return _csConsoleLock.RecursionCount; + return recursionCount; } // Routine Description: diff --git a/src/host/server.h b/src/host/server.h index c57619972a6..39d539158dd 100644 --- a/src/host/server.h +++ b/src/host/server.h @@ -77,7 +77,6 @@ class CONSOLE_INFORMATION : { public: CONSOLE_INFORMATION(); - ~CONSOLE_INFORMATION(); CONSOLE_INFORMATION(const CONSOLE_INFORMATION& c) = delete; CONSOLE_INFORMATION& operator=(const CONSOLE_INFORMATION& c) = delete; @@ -103,11 +102,10 @@ class CONSOLE_INFORMATION : ConsoleImeInfo ConsoleIme; - void LockConsole(); - bool TryLockConsole(); - void UnlockConsole(); - bool IsConsoleLocked() const; - ULONG GetCSRecursionCount(); + static void LockConsole(); + static void UnlockConsole(); + static bool IsConsoleLocked(); + static ULONG GetCSRecursionCount(); Microsoft::Console::VirtualTerminal::VtIo* GetVtIo(); @@ -144,7 +142,6 @@ class CONSOLE_INFORMATION : RenderData renderData; private: - CRITICAL_SECTION _csConsoleLock; // serialize input and output using this std::wstring _Title; std::wstring _Prefix; // Eg Select, Mark - things that we manually prepend to the title. std::wstring _TitleAndPrefix;