Repository: microsoft/terminal (language: cpp)

Issue:
Windows terminal hangs under stress after resizing vertical pane to the minimum width
# Environment

```none
Windows build number: 10.0.20197.0
Windows Terminal version: 1.2.2381.0
```
# Steps to reproduce
1. Open Windows Terminal
2. Press alt+shift+= to open a second vertical pane
3. Build and run this [TerminalStress](https://github.com/Treit/TerminalStress) application in the left-hand pane.
4. While the stress test is running, press `alt+shift+left arrow key` to reduce the size of the pane. Repeat until the pane is reduced to the smallest possible width.

# Expected behavior
The terminal does not hang.

# Actual behavior
The terminal becomes unresponsive and hangs.




Discussion / hints:
I can reproduce this very easily. At the time of writing, this is caused by deadlocks due to numerous race conditions in [ticket_lock.h](https://github.com/microsoft/terminal/blob/06526cac0c8a7e1a63fd187ce495e3b88522adff/src/inc/til/ticket_lock.h).

The deadlock visualized:
![The deadlock visualized](https://user-images.githubusercontent.com/25628581/230237920-ca22cf02-397b-457b-abce-240e6965f9fd.png)

This isn't a deadlock (edit: ...between threads) though since none of the threads are waiting on each other? From what I can tell a deadlock occurs, because inserting a 2-column wide glyph into a 1 column wide text buffer triggers a newline wrap. But since there's no row in the text buffer that fits a 2-column wide glyph, it'll loop forever, trying to fit the same glyph into the next line and the next and the next, etc. The fix is to recognize this situation and drop the wide glyph entirely.
What I observed is all the threads are waiting for each other. This is not only occurring in described sitation AFAIK; we may have two separate issues here. You can reproduce the deadlock without resizing terminals (let them be equally wide) but instead very quickly resizing the entire host window horizontally (I acknowledge this will resize terminals, though) by grabbing and dragging the right border of the host window). All the threads share the same instance of  recursive_ticket_lock and get deadlocked since checks like

https://github.com/microsoft/terminal/blob/06526cac0c8a7e1a63fd187ce495e3b88522adff/src/inc/til/ticket_lock.h#L102

are not thread safe and prone to race conditions, (only **_owner.load(std::memory_order_relaxed)** part of the **if** is atomic, the **!=** operator is not, which leads to race conditions. Two threads will read value of **_owner.load(std::memory_order_relaxed)**, they both can get 0 and both will deem **!= id** to be **true**, both will try to acquire a lock which through that race condition later leads to a deadlock.
> You can reproduce the deadlock without resizing terminals (let them be equally wide) but instead very quickly resizing the entire host window horizontally (I acknowledge this will resize terminals, though) by grabbing and dragging the right border of the host window).

Can you reproduce the issue without resizing the window to a width of 1 column? If you do, I'd be super interested in this, because I cannot reproduce such an issue myself. If you could get a minidump of Windows Terminal when this happens, this would be extremely helpful! The screenshot above however seems to show the deadlock that occurs when you resize the window down to 1 column and is not caused by any race conditions. It occurs because of the issue described above (i.e. an infinite loop in `AdaptDispatch::_WriteBuffer`).

> All the threads share the same instance of  recursive_ticket_lock and get deadlocked since checks like [...] are not thread safe and prone to race conditions, (only **_owner.load(std::memory_order_relaxed)** part of the **if** is atomic, the **!=** operator is not, which leads to race conditions.

You didn't mean the actual comparison there right? Since a `!=` on a `std::atomic<T>::load` return value is thread safe in on itself (`load` returns a copy of the integer after all). You meant that the underlying value may change while the current thread performs the comparison, right? In this case, yes, that's actually fine.

That code is a generic approach to turn any non-recursive lock into a recursive one. The assumptions are that `GetCurrentThreadId()` never returns 0, because that's the initial value for `_owner` (this is true for `GetCurrentThreadId()` as it contractually never returns 0). Furthermore, it's return value is unique for every thread and a single thread can only run a single strand of instructions at a time, which means that within a thread there's no need for synchronization.

With the above in mind, the only possible way that this if condition is ever skipped:
https://github.com/microsoft/terminal/blob/06526cac0c8a7e1a63fd187ce495e3b88522adff/src/inc/til/ticket_lock.h#L102

is when the current thread has already acquired the lock. If the current thread has not acquired the lock, the `_owner` value must be some undetermined other value. It's not really important what that value is, but it's definitely not the same as `id`, because only the current thread can have that value.

But back to the race between threads you brought up: As mentioned above, it is possible for two or more threads to enter this if condition simultaneously. Let's say 2 threads enter the if condition simultaneously. Thread A loads and compares with `_owner` and then acquires the non-recursive actual lock (`_lock.lock();`). It'll then write their own ID into `_owner`. Thread B has already loaded `_owner` in the meantime, but hasn't compared it (the race condition you brought up). The underlying value of `_owner` has changed and thread B is looking at stale data, however the result of the comparison hasn't changed anyways: The value of `_owner` is still not identical to thread B's `id`. Only the current thread can set a value that would make the `!=` comparison fail.
@lhecker I deleted my latest response not to confuse anybody. I see how I completely misunderstood the intent here and that the **if** block is designed for potentially many threads to enter it and block except the one with an active lock already, in which case it will not enter the **if** again and simply increase the recursive counter. The race condition I brought up is irrelevant due to the blocking nature of **_lock.lock()** if another lock is already active. I apologize for the confusion, thank you for the detailed response, and I will follow up with Minidump when I can repro non-one width terminal hang.
I think the bug for freezes in general is #11903. Our leading theory is that it happens because we acquire a mutex on the main, while the rendering thread is holding it. If the rendering thread however waits on the main thread already (which can happen depending on the graphics driver implementation) you get a deadlock. #14959 would improve on this situation because it doesn't call any graphics APIs while holding the mutex anymore. Solving the issue fundamentally will have to wait a little longer though unfortunately (it requires being able to snapshot the text buffer).

Fix patch:
diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp
index 9e969c6ed24..b3c2e590e37 100644
--- a/src/buffer/out/textBuffer.cpp
+++ b/src/buffer/out/textBuffer.cpp
@@ -272,10 +272,6 @@ bool TextBuffer::_AssertValidDoubleByteSequence(const DbcsAttribute dbcsAttribut
 // - false otherwise (out of memory)
 bool TextBuffer::_PrepareForDoubleByteSequence(const DbcsAttribute dbcsAttribute)
 {
-    // This function corrects most errors. If this is false, we had an uncorrectable one which
-    // older versions of conhost simply let pass by unflinching.
-    LOG_HR_IF(E_NOT_VALID_STATE, !(_AssertValidDoubleByteSequence(dbcsAttribute))); // Shouldn't be uncorrectable sequences unless something is very wrong.
-
     auto fSuccess = true;
     // Now compensate if we don't have enough space for the upcoming double byte sequence
     // We only need to compensate for leading bytes
diff --git a/src/terminal/adapter/adaptDispatch.cpp b/src/terminal/adapter/adaptDispatch.cpp
index 32c06d67bff..37e7125627d 100644
--- a/src/terminal/adapter/adaptDispatch.cpp
+++ b/src/terminal/adapter/adaptDispatch.cpp
@@ -139,17 +139,25 @@ void AdaptDispatch::_WriteToBuffer(const std::wstring_view string)
 
         if (isWrapping)
         {
-            if (wrapAtEOL)
+            // We want to wrap, but we failed to write even a single character into the row.
+            // ROW::Write() returns the lineWidth and leaves stringIterator untouched. To prevent a
+            // deadlock, because stringIterator never advances, we need to throw that glyph away.
+            //
+            // This can happen under two circumstances:
+            // * The glyph is wider than the buffer and can never be inserted in
+            //   the first place. There's no good way to detect this, so we check
+            //   whether the begin column is the left margin, which is the column
+            //   at which any legit insertion should work at a minimum.
+            // * The DECAWM Autowrap mode is disabled ("\x1b[?7l", !wrapAtEOL) and
+            //   we tried writing a wide glyph into the last column which can't work.
+            if (textPositionBefore == textPositionAfter && (state.columnBegin == 0 || !wrapAtEOL))
             {
-                cursor.DelayEOLWrap();
+                textBuffer.ConsumeGrapheme(state.text);
             }
-            else if (textPositionBefore == textPositionAfter)
+
+            if (wrapAtEOL)
             {
-                // We want to wrap, but we're not allowed to and we failed to write even a single character into the row.
-                // This can only mean one thing! The DECAWM Autowrap mode is disabled ("\x1b[?7l") and we tried writing a
-                // wide glyph into the last column. ROW::Write() returns the lineWidth and leaves stringIterator untouched.
-                // To prevent a deadlock, because stringIterator never advances, we need to throw that glyph away.
-                textBuffer.ConsumeGrapheme(state.text);
+                cursor.DelayEOLWrap();
             }
         }
     }


Test patch: