Repository: microsoft/terminal (language: cpp)

Issue:
Low reliability of VT replies
### Windows Terminal version

Latest source

### Windows build number

10.0.19045.4780

### Other Software

No

### Steps to reproduce

I want to query some terminal state, in particular the latest and greatest palette (#17729).
To do so, I build a bunch of those `"\x1b]4;N;?\x1b\\"`, send them in one go and wait for the reply.
Since I want to support not only the latest nightlies and do not want the older versions to deadlock on unknown queries, I prepend and append another query, the one that even prehistoric versions understand: `"\x1b[c"`.
If there is something between those DA replies, it must be the one I need.
If there are only two DA replies, then it is probably not supported.

This approach works flawlessly in OpenConsole.
In WT, however, not so much: way, way too often random parts of the reply are simply missing.

1. Compile the code:
    
<details>
<summary>Code</summary>
  
```C++
#ifndef _CRT_SECURE_NO_WARNINGS
#define _CRT_SECURE_NO_WARNINGS
#endif

#ifndef _UNICODE
#define _UNICODE
#endif

#ifndef UNICODE
#define UNICODE
#endif

#include <algorithm>
#include <format>
#include <exception>
#include <iostream>
#include <optional>
#include <ranges>
#include <string>
#include <string_view>

#include <string.h>

#include <windows.h>

using namespace std::literals;

auto win32_error()
{
	return std::runtime_error(std::format("Error 0x{:08X}, look it up"sv, GetLastError()));
};

auto invalid_response()
{
	return std::runtime_error("Invalid response");
}

auto invalid_response(const size_t Index)
{
	return std::runtime_error(std::format("Invalid response at #{}"sv, Index));
}

auto printable(const std::wstring_view Str)
{
	std::wstring Printable;
	std::ranges::transform(Str, std::back_inserter(Printable), [](wchar_t Char){ return Char < L' ' ? Char + L'\u2400' : Char; });
	return Printable;
}

void write(const std::wstring_view Str)
{
	DWORD Written;
	if (!WriteConsole(GetStdHandle(STD_OUTPUT_HANDLE), Str.data(), static_cast<DWORD>(Str.size()), &Written, {}))
		throw win32_error();
}

void write(const std::string_view Str)
{
	write(std::wstring(Str.begin(), Str.end()));
}

std::wstring query(const std::wstring& Command)
{
	const auto Dummy = L"\x1b[c"s;

	write(Dummy + Command + Dummy);

	// Sleep(1);

	std::wstring Response;

	std::optional<size_t>
		FirstTokenPrefixPos,
		FirstTokenSuffixPos,
		SecondTokenPrefixPos,
		SecondTokenSuffixPos;

	const auto
		TokenPrefix = L"\x1b[?"sv,
		TokenSuffix = L"c"sv;

	while (!SecondTokenSuffixPos)
	{
		wchar_t ResponseBuffer[8192];
		DWORD ResponseSize;

		if (!ReadConsole(GetStdHandle(STD_INPUT_HANDLE), ResponseBuffer, ARRAYSIZE(ResponseBuffer),  &ResponseSize, {}))
			throw win32_error();

		Response.append(ResponseBuffer, ResponseSize);

		if (!FirstTokenPrefixPos)
			if (const auto Pos = Response.find(TokenPrefix); Pos != Response.npos)
				FirstTokenPrefixPos = Pos;

		if (FirstTokenPrefixPos && !FirstTokenSuffixPos)
			if (const auto Pos = Response.find(TokenSuffix, *FirstTokenPrefixPos + TokenPrefix.size()); Pos != Response.npos)
				FirstTokenSuffixPos = Pos;

		if (FirstTokenSuffixPos && !SecondTokenPrefixPos)
			if (const auto Pos = Response.find(TokenPrefix, *FirstTokenSuffixPos + TokenSuffix.size()); Pos != Response.npos)
				SecondTokenPrefixPos = Pos;

		if (SecondTokenPrefixPos && !SecondTokenSuffixPos)
			if (const auto Pos = Response.find(TokenSuffix, *SecondTokenPrefixPos + TokenPrefix.size()); Pos != Response.npos)
				SecondTokenSuffixPos = Pos;
	}

	Response.resize(*SecondTokenPrefixPos);
	Response.erase(0, *FirstTokenSuffixPos + TokenSuffix.size());

	if (Response.empty())
		throw std::runtime_error("Query is not supported"s);

	return Response;
}

void set_modes()
{
	const auto
		In = GetStdHandle(STD_INPUT_HANDLE),
		Out = GetStdHandle(STD_OUTPUT_HANDLE);

	DWORD InMode;
	if (!GetConsoleMode(In, &InMode))
		throw win32_error();

	if (!SetConsoleMode(In, (InMode & ~(ENABLE_LINE_INPUT | ENABLE_ECHO_INPUT | ENABLE_QUICK_EDIT_MODE)) | ENABLE_MOUSE_INPUT | ENABLE_EXTENDED_FLAGS | ENABLE_VIRTUAL_TERMINAL_INPUT))
		throw win32_error();

	DWORD OutMode;
	if (!GetConsoleMode(Out, &OutMode))
		throw win32_error();

	if (!SetConsoleMode(Out, OutMode | ENABLE_PROCESSED_OUTPUT | ENABLE_VIRTUAL_TERMINAL_PROCESSING))
		throw win32_error();
}

int main()
{
	try
	{
		set_modes();

		std::wstring Str;

		for (size_t i = 0; i != 256; ++i)
			std::format_to(std::back_inserter(Str), L"\x1b]4;{};?\x1b\\"sv, i);

		for (;;)
		{
			const auto ReplyData = query(Str);

			std::wstring_view Reply(ReplyData);

			if (!Reply.ends_with(L"\\"sv))
				throw invalid_response();

			Reply.remove_suffix(1);

			size_t Index = 0;

			for (const auto& Part: std::views::split(Reply, L"\\"sv))
			{
				std::wstring_view ColorStr(Part.begin(), Part.end());

				try
				{
					if (!ColorStr.starts_with(L"\x1b]4;"sv))
						throw invalid_response(Index);

					ColorStr.remove_prefix(L"\x1b]4;"sv.size());

					wchar_t* Ptr;
					const auto ReportedIndex = std::wcstol(ColorStr.data(), &Ptr, 10);

					if (Ptr == ColorStr.data())
						throw invalid_response(Index);

					if (ReportedIndex != Index)
						throw invalid_response(Index);

					ColorStr.remove_prefix(Ptr - ColorStr.data());

					const auto End = ColorStr.find(L"\x1b"sv);
					if (End == ColorStr.npos)
						throw invalid_response(Index);

					ColorStr.remove_suffix(ColorStr.size() - End);

					if (!ColorStr.starts_with(L";rgb:"sv))
						throw invalid_response(Index);

					ColorStr.remove_prefix(L";rgb:"sv.size());

					if (ColorStr.size() != L"ffff/ffff/ffff"sv.size())
						throw invalid_response(Index);
				}
				catch (const std::runtime_error& e)
				{
					Beep(500, 200);

					write(e.what());
					write(L": "sv);
					write(printable(ColorStr));
					write(L"\n"sv);
				}

				++Index;
			}

			std::wcout << L"Everything is OK"sv << std::endl;

		}
	}
	catch (const std::exception& e)
	{
		std::cerr << e.what() << std::endl;
	}
}
```
</details>

2. Run it in WT.
 


### Expected Behavior

The program should print "Everything is OK" in a loop indefinitely and nothing else:
```
Everything is OK
Everything is OK
Everything is OK
Everything is OK
...and so on.
```

### Actual Behavior

The program prints "Everything is OK" a few times, then it detects errors due to missing bits in the reply, e.g.
```
Everything is OK
Everything is OK
Everything is OK
Everything is OK
Invalid response at #234: c/1c1c␛
Everything is OK
```

Eventually it deadlocks in `ReadConsole`.

Adding a delay between sending the query and reading the reply improves the situation dramatically, but still not to 100% and it does not feel like the right thing to do: fixing issues with `Sleep(N)` never works in the long term.

Splitting it to smaller queries also helps, but again, it is guesswork.
Overall it is about 2700 characters to send and about 7000 to receive. It does not look like something humongous by modern standards.

Moving the mouse or pressing keyboard keys noticeably increases the error rate. Again, only in WT.



Discussion / hints:
I believe this is caused by #16224. Since the terminal lock isn't held anymore, any thread can write into the input pipe concurrently. In our case that's the VT parser thread sending VT responses and the UI thread sending cursor messages.

Fix patch:
diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp
index d8d975f667c..357b43c1aa1 100644
--- a/src/cascadia/TerminalControl/ControlCore.cpp
+++ b/src/cascadia/TerminalControl/ControlCore.cpp
@@ -98,7 +98,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         Connection(connection);
 
         _terminal->SetWriteInputCallback([this](std::wstring_view wstr) {
-            _sendInputToConnection(wstr);
+            _pendingResponses.append(wstr);
         });
 
         // GH#8969: pre-seed working directory to prevent potential races
@@ -419,6 +419,17 @@ namespace winrt::Microsoft::Terminal::Control::implementation
     // Return Value:
     // - <none>
     void ControlCore::_sendInputToConnection(std::wstring_view wstr)
+    {
+        _connection.WriteInput(winrt_wstring_to_array_view(wstr));
+    }
+
+    // Method Description:
+    // - Writes the given sequence as input to the active terminal connection,
+    // Arguments:
+    // - wstr: the string of characters to write to the terminal connection.
+    // Return Value:
+    // - <none>
+    void ControlCore::SendInput(const std::wstring_view wstr)
     {
         if (wstr.empty())
         {
@@ -435,21 +446,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         }
         else
         {
-            _connection.WriteInput(winrt_wstring_to_array_view(wstr));
+            _sendInputToConnection(wstr);
         }
     }
 
-    // Method Description:
-    // - Writes the given sequence as input to the active terminal connection,
-    // Arguments:
-    // - wstr: the string of characters to write to the terminal connection.
-    // Return Value:
-    // - <none>
-    void ControlCore::SendInput(const std::wstring_view wstr)
-    {
-        _sendInputToConnection(wstr);
-    }
-
     bool ControlCore::SendCharEvent(const wchar_t ch,
                                     const WORD scanCode,
                                     const ::Microsoft::Terminal::Core::ControlKeyStates modifiers)
@@ -485,7 +485,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         }
         if (out)
         {
-            _sendInputToConnection(*out);
+            SendInput(*out);
             return true;
         }
         return false;
@@ -643,7 +643,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         }
         if (out)
         {
-            _sendInputToConnection(*out);
+            SendInput(*out);
             return true;
         }
         return false;
@@ -662,7 +662,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         }
         if (out)
         {
-            _sendInputToConnection(*out);
+            SendInput(*out);
             return true;
         }
         return false;
@@ -1412,7 +1412,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         }
 
         // It's important to not hold the terminal lock while calling this function as sending the data may take a long time.
-        _sendInputToConnection(filtered);
+        SendInput(filtered);
 
         const auto lock = _terminal->LockForWriting();
         _terminal->ClearSelection();
@@ -2107,7 +2107,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
                     // Sending input requires that we're unlocked, because
                     // writing the input pipe may block indefinitely.
                     const auto suspension = _terminal->SuspendLock();
-                    _sendInputToConnection(buffer);
+                    SendInput(buffer);
                 }
             }
         }
@@ -2164,6 +2164,12 @@ namespace winrt::Microsoft::Terminal::Control::implementation
                 _terminal->Write(hstr);
             }
 
+            if (!_pendingResponses.empty())
+            {
+                _sendInputToConnection(_pendingResponses);
+                _pendingResponses.clear();
+            }
+
             // Start the throttled update of where our hyperlinks are.
             const auto shared = _shared.lock_shared();
             if (shared->outputIdle)
@@ -2480,9 +2486,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         }
         if (out && !out->empty())
         {
-            // _sendInputToConnection() asserts that we aren't in focus mode,
-            // but window focus events are always fine to send.
-            _connection.WriteInput(winrt_wstring_to_array_view(*out));
+            _sendInputToConnection(*out);
         }
     }
 
diff --git a/src/cascadia/TerminalControl/ControlCore.h b/src/cascadia/TerminalControl/ControlCore.h
index 938d048243b..57484c0b7b7 100644
--- a/src/cascadia/TerminalControl/ControlCore.h
+++ b/src/cascadia/TerminalControl/ControlCore.h
@@ -319,6 +319,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         winrt::com_ptr<ControlSettings> _settings{ nullptr };
 
         std::shared_ptr<::Microsoft::Terminal::Core::Terminal> _terminal{ nullptr };
+        std::wstring _pendingResponses;
 
         // NOTE: _renderEngine must be ordered before _renderer.
         //
diff --git a/src/cascadia/TerminalCore/TerminalApi.cpp b/src/cascadia/TerminalCore/TerminalApi.cpp
index 26daf24e5d2..82e06b03d4a 100644
--- a/src/cascadia/TerminalCore/TerminalApi.cpp
+++ b/src/cascadia/TerminalCore/TerminalApi.cpp
@@ -24,7 +24,6 @@ void Terminal::ReturnResponse(const std::wstring_view response)
 {
     if (_pfnWriteInput && !response.empty())
     {
-        const auto suspension = _readWriteLock.suspend();
         _pfnWriteInput(response);
     }
 }


Test patch: