Add recursive_ticket_lock and use it for Terminal

.github/actions/spelling/allow/allow.txt

@@ -54,6 +54,7 @@ Llast
 llvm
 Lmid
 locl
+lol
 lorem
 Lorigin
 maxed

src/cascadia/TerminalControl/ControlCore.cpp

@@ -1348,17 +1348,17 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         auto& midiAudio = _getMidiAudio();
         midiAudio.Lock();
 
-        // We then unlock the terminal, so the UI doesn't hang while we're busy.
-        auto& terminalLock = _terminal->GetReadWriteLock();
-        terminalLock.unlock();
+        {
+            // We then unlock the terminal, so the UI doesn't hang while we're busy.
+            const auto suspension = _terminal->SuspendLock();
 
-        // This call will block for the duration, unless shutdown early.
-        midiAudio.PlayNote(noteNumber, velocity, duration);
+            // This call will block for the duration, unless shutdown early.
+            midiAudio.PlayNote(noteNumber, velocity, duration);
+        }
 
         // Once complete, we reacquire the terminal lock and unlock the audio.
         // If the terminal has shutdown in the meantime, the Unlock call
         // will throw an exception, forcing the thread to exit ASAP.
-        terminalLock.lock();
         midiAudio.Unlock();
     }
 

src/cascadia/TerminalCore/Terminal.cpp

@@ -982,40 +982,28 @@ WORD Terminal::_TakeVirtualKeyFromLastKeyEvent(const WORD scanCode) noexcept
 // Return Value:
 // - a shared_lock which can be used to unlock the terminal. The shared_lock
 //      will release this lock when it's destructed.
-[[nodiscard]] std::unique_lock<til::ticket_lock> Terminal::LockForReading()
+[[nodiscard]] std::unique_lock<til::recursive_ticket_lock> Terminal::LockForReading()
 {
-#ifdef NDEBUG
     return std::unique_lock{ _readWriteLock };
-#else
-    auto lock = std::unique_lock{ _readWriteLock };
-    _lastLocker = GetCurrentThreadId();
-    return lock;
-#endif
 }
 
 // Method Description:
 // - Acquire a write lock on the terminal.
 // Return Value:
 // - a unique_lock which can be used to unlock the terminal. The unique_lock
 //      will release this lock when it's destructed.
-[[nodiscard]] std::unique_lock<til::ticket_lock> Terminal::LockForWriting()
+[[nodiscard]] std::unique_lock<til::recursive_ticket_lock> Terminal::LockForWriting()
 {
-#ifdef NDEBUG
     return std::unique_lock{ _readWriteLock };
-#else
-    auto lock = std::unique_lock{ _readWriteLock };
-    _lastLocker = GetCurrentThreadId();
-    return lock;
-#endif
 }
 
 // Method Description:
 // - Get a reference to the terminal's read/write lock.
 // Return Value:
 // - a ticket_lock which can be used to manually lock or unlock the terminal.
-til::ticket_lock& Terminal::GetReadWriteLock() noexcept
+til::recursive_ticket_lock_suspension Terminal::SuspendLock() noexcept
 {
-    return _readWriteLock;
+    return _readWriteLock.suspend();
 }
 
 Viewport Terminal::_GetMutableViewport() const noexcept

src/cascadia/TerminalCore/Terminal.hpp

@@ -90,9 +90,9 @@ class Microsoft::Terminal::Core::Terminal final :
     // WritePastedText comes from our input and goes back to the PTY's input channel
     void WritePastedText(std::wstring_view stringView);
 
-    [[nodiscard]] std::unique_lock<til::ticket_lock> LockForReading();
-    [[nodiscard]] std::unique_lock<til::ticket_lock> LockForWriting();
-    til::ticket_lock& GetReadWriteLock() noexcept;
+    [[nodiscard]] std::unique_lock<til::recursive_ticket_lock> LockForReading();
+    [[nodiscard]] std::unique_lock<til::recursive_ticket_lock> LockForWriting();
+    til::recursive_ticket_lock_suspension SuspendLock() noexcept;
 
     til::CoordType GetBufferHeight() const noexcept;
 
@@ -304,10 +304,7 @@ class Microsoft::Terminal::Core::Terminal final :
     //
     // But we can abuse the fact that the surrounding members rarely change and are huge
     // (std::function is like 64 bytes) to create some natural padding without wasting space.
-    til::ticket_lock _readWriteLock;
-#ifndef NDEBUG
-    DWORD _lastLocker;
-#endif
+    til::recursive_ticket_lock _readWriteLock;
 
     std::function<void(const int, const int, const int)> _pfnScrollPositionChanged;
     std::function<void()> _pfnCursorPositionChanged;

src/host/consoleInformation.cpp

@@ -11,8 +11,6 @@
 #include "../interactivity/inc/ServiceLocator.hpp"
 #include "../types/inc/convert.hpp"
 
-#include <til/ticket_lock.h>
-
 using Microsoft::Console::Interactivity::ServiceLocator;
 using Microsoft::Console::VirtualTerminal::VtIo;
 
@@ -47,50 +45,26 @@ CONSOLE_INFORMATION::CONSOLE_INFORMATION() :
     ZeroMemory((void*)&OutputCPInfo, sizeof(OutputCPInfo));
 }
 
-// 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()
+bool CONSOLE_INFORMATION::IsConsoleLocked() const noexcept
 {
-    return recursionCount != 0;
+    return _lock.is_locked();
 }
 
 #pragma prefast(suppress : 26135, "Adding lock annotation spills into entire project. Future work.")
-void CONSOLE_INFORMATION::LockConsole()
+void CONSOLE_INFORMATION::LockConsole() noexcept
 {
-    // See description of recursionCount a few lines above.
-    const auto rc = ++recursionCount;
-    FAIL_FAST_IF(rc == 0);
-    if (rc == 1)
-    {
-        lock.lock();
-    }
+    _lock.lock();
 }
 
 #pragma prefast(suppress : 26135, "Adding lock annotation spills into entire project. Future work.")
-void CONSOLE_INFORMATION::UnlockConsole()
+void CONSOLE_INFORMATION::UnlockConsole() noexcept
 {
-    // See description of recursionCount a few lines above.
-    const auto rc = --recursionCount;
-    FAIL_FAST_IF(rc == ULONG_MAX);
-    if (rc == 0)
-    {
-        lock.unlock();
-    }
+    _lock.unlock();
 }
 
-ULONG CONSOLE_INFORMATION::GetCSRecursionCount()
+ULONG CONSOLE_INFORMATION::GetCSRecursionCount() const noexcept
 {
-    return recursionCount;
+    return _lock.recursion_depth();
 }
 
 // Routine Description:

src/host/server.h

@@ -30,6 +30,8 @@ Revision History:
 #include "../host/RenderData.hpp"
 #include "../audio/midi/MidiAudio.hpp"
 
+#include <til/ticket_lock.h>
+
 // clang-format off
 // Flags flags
 #define CONSOLE_IS_ICONIC               0x00000001
@@ -103,10 +105,10 @@ class CONSOLE_INFORMATION :
 
     ConsoleImeInfo ConsoleIme;
 
-    static void LockConsole();
-    static void UnlockConsole();
-    static bool IsConsoleLocked();
-    static ULONG GetCSRecursionCount();
+    void LockConsole() noexcept;
+    void UnlockConsole() noexcept;
+    bool IsConsoleLocked() const noexcept;
+    ULONG GetCSRecursionCount() const noexcept;
 
     Microsoft::Console::VirtualTerminal::VtIo* GetVtIo();
 
@@ -147,6 +149,8 @@ class CONSOLE_INFORMATION :
     RenderData renderData;
 
 private:
+    til::recursive_ticket_lock _lock;
+
     std::wstring _Title;
     std::wstring _Prefix; // Eg Select, Mark - things that we manually prepend to the title.
     std::wstring _TitleAndPrefix;

src/inc/til/ticket_lock.h

@@ -53,4 +53,89 @@ namespace til
         std::atomic<uint32_t> _next_ticket{ 0 };
         std::atomic<uint32_t> _now_serving{ 0 };
     };
+
+    struct recursive_ticket_lock
+    {
+        struct recursive_ticket_lock_suspension
+        {
+            constexpr recursive_ticket_lock_suspension(recursive_ticket_lock& lock) noexcept :
+                _lock{ lock }
+            {
+            }
+
+            ~recursive_ticket_lock_suspension()
+            {
+                if (_owner)
+                {
+                    _lock._lock.lock(); // lock-lock-lock lol
+                    _lock._owner.store(_owner, std::memory_order_relaxed);
+                    _lock._recursion = _recursion;
+                }
+            }
+
+        private:
+            friend struct recursive_ticket_lock;
+
+            recursive_ticket_lock& _lock;
+            uint32_t _owner = 0;
+            uint32_t _recursion = 0;
+        };
+
+        void lock() noexcept
+        {
+            const auto id = GetCurrentThreadId();
+
+            if (_owner.load(std::memory_order_relaxed) != id)
+            {
+                _lock.lock();
+                _owner.store(id, std::memory_order_relaxed);
+            }
+
+            _recursion++;
+        }
+
+        void unlock() noexcept
+        {
+            if (--_recursion == 0)
+            {
+                _owner.store(0, std::memory_order_relaxed);
+                _lock.unlock();
+            }
+        }
+
+        [[nodiscard]] recursive_ticket_lock_suspension suspend() noexcept
+        {
+            const auto id = GetCurrentThreadId();
+            recursive_ticket_lock_suspension guard{ *this };
+
+            if (_owner.load(std::memory_order_relaxed) == id)
+            {
+                guard._owner = id;
+                guard._recursion = _recursion;
+                _owner.store(0, std::memory_order_relaxed);
+                _recursion = 0;
+                _lock.unlock();
+            }
+
+            return guard;
+        }
+
+        uint32_t is_locked() const noexcept
+        {
+            const auto id = GetCurrentThreadId();
+            return _owner.load(std::memory_order_relaxed) == id;
+        }
+
+        uint32_t recursion_depth() const noexcept
+        {
+            return is_locked() ? _recursion : 0;
+        }
+
+    private:
+        ticket_lock _lock;
+        std::atomic<uint32_t> _owner = 0;
+        uint32_t _recursion = 0;
+    };
+
+    using recursive_ticket_lock_suspension = recursive_ticket_lock::recursive_ticket_lock_suspension;
 }