Add recursive_ticket_lock and use it for Terminal (#13746)

My hope with this commit is to make our code more robust against accidental
recursive locking, as well as making it easier to write code with confidence,
with only a slight performance trade-off.

## Validation Steps Performed
* Playing Pac Man music through MIDI ✅
* Windows Terminal runs happily ever after ✅

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

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

src/cascadia/TerminalControl/ControlCore.cpp

@@ -1412,14 +1412,12 @@ namespace winrt::Microsoft::Terminal::Control::implementation
     // - duration - How long the note should be sustained (in microseconds).
     void ControlCore::_terminalPlayMidiNote(const int noteNumber, const int velocity, const std::chrono::microseconds duration)
     {
-        // Unlock the terminal, so the UI doesn't hang while we're busy.
-        auto& terminalLock = _terminal->GetReadWriteLock();
-        terminalLock.unlock();
+        // The UI thread might try to acquire the console lock from time to time.
+        // --> Unlock it, 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(reinterpret_cast<HWND>(_owningHwnd), noteNumber, velocity, std::chrono::duration_cast<std::chrono::milliseconds>(duration));
-
-        terminalLock.lock();
     }
 
     bool ControlCore::HasSelection() const

src/cascadia/TerminalCore/Terminal.cpp

@@ -1012,15 +1012,9 @@ 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:
@@ -1028,24 +1022,18 @@ WORD Terminal::_TakeVirtualKeyFromLastKeyEvent(const WORD scanCode) noexcept
 // 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

@@ -93,9 +93,9 @@ public:
     // 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;
 
@@ -309,10 +309,7 @@ private:
     //
     // 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/cascadia/TerminalCore/terminalrenderdata.cpp

@@ -208,9 +208,6 @@ catch (...)
 void Terminal::LockConsole() noexcept
 {
     _readWriteLock.lock();
-#ifndef NDEBUG
-    _lastLocker = GetCurrentThreadId();
-#endif
 }
 
 // Method Description:

src/host/consoleInformation.cpp

@@ -15,8 +15,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;
 
@@ -51,50 +49,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 @@ public:
 
     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();
 
@@ -146,6 +148,8 @@ public:
     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,104 @@ 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, uint32_t owner, uint32_t recursion) noexcept :
+                _lock{ lock },
+                _owner{ owner },
+                _recursion{ recursion }
+            {
+            }
+
+            // When this class is destroyed it restores the recursive_ticket_lock state.
+            // This of course only works if the lock wasn't moved to another thread or something.
+            recursive_ticket_lock_suspension(const recursive_ticket_lock_suspension&) = delete;
+            recursive_ticket_lock_suspension& operator=(const recursive_ticket_lock_suspension&) = delete;
+            recursive_ticket_lock_suspension(recursive_ticket_lock_suspension&&) = delete;
+            recursive_ticket_lock_suspension& operator=(recursive_ticket_lock_suspension&&) = delete;
+
+            ~recursive_ticket_lock_suspension()
+            {
+                if (_owner)
+                {
+                    // If someone reacquired the lock on the current thread, we shouldn't lock it again.
+                    if (_lock._owner.load(std::memory_order_relaxed) != _owner)
+                    {
+                        _lock._lock.lock(); // lock-lock-lock lol
+                        _lock._owner.store(_owner, std::memory_order_relaxed);
+                    }
+                    // ...but we should restore the original recursion count.
+                    _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();
+            uint32_t owner = 0;
+            uint32_t recursion = 0;
+
+            if (_owner.load(std::memory_order_relaxed) == id)
+            {
+                owner = id;
+                recursion = _recursion;
+                _owner.store(0, std::memory_order_relaxed);
+                _recursion = 0;
+                _lock.unlock();
+            }
+
+            return { *this, owner, recursion };
+        }
+
+        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;
 }