From e60afb6f4e3c207b0f245b2bbf10f2347f9f3ce4 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Fri, 23 Aug 2024 19:54:27 +0200 Subject: [PATCH] Improve reliability of VT responses (#17786) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Repurposes `_sendInputToConnection` to send output to the connection no matter whether the terminal is read-only or not. Now `SendInput` is the function responsible for the UI handling. * Buffers responses in a VT string into a single string before sending it as a response all at once. This reduces the chances for the UI thread to insert cursor positions and similar into the input pipe, because we're not constantly unlocking the terminal lock anymore for every response. The only way now that unrelated inputs are inserted into the input pipe is because the VT requests (e.g. DA1, DSR, etc.) are broken up across >1 reads. This also fixes VT responses in read-only panes. Closes #17775 * Repeatedly run `echo ^[[c` in cmd. DA1 responses don't stack & always stay the same ✅ * Run nvim in WSL. Doesn't deadlock when pasting 1MB. ✅ * Run the repro from #17775, which requests a ton of OSC 4 (color palette) responses. Jiggle the cursor on top of the window. Responses never get split up. ✅ (cherry picked from commit b07589e7a87e1d7a2f6144744a59c866c672a78e) Service-Card-Id: PVTI_lADOAF3p4s4AmhmszgSK_Dw Service-Version: 1.21 --- src/cascadia/TerminalControl/ControlCore.cpp | 44 +++++++++++--------- src/cascadia/TerminalControl/ControlCore.h | 1 + 2 files changed, 26 insertions(+), 19 deletions(-) diff --git a/src/cascadia/TerminalControl/ControlCore.cpp b/src/cascadia/TerminalControl/ControlCore.cpp index c4ad4b0799d..3ace9a25f7c 100644 --- a/src/cascadia/TerminalControl/ControlCore.cpp +++ b/src/cascadia/TerminalControl/ControlCore.cpp @@ -80,7 +80,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 @@ -398,6 +398,17 @@ namespace winrt::Microsoft::Terminal::Control::implementation // Return Value: // - void ControlCore::_sendInputToConnection(std::wstring_view wstr) + { + _connection.WriteInput(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: + // - + void ControlCore::SendInput(const std::wstring_view wstr) { if (wstr.empty()) { @@ -414,21 +425,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation } else { - _connection.WriteInput(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: - // - - 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) @@ -464,7 +464,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation } if (out) { - _sendInputToConnection(*out); + SendInput(*out); return true; } return false; @@ -622,7 +622,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation } if (out) { - _sendInputToConnection(*out); + SendInput(*out); return true; } return false; @@ -641,7 +641,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation } if (out) { - _sendInputToConnection(*out); + SendInput(*out); return true; } return false; @@ -1392,7 +1392,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(); @@ -2083,7 +2083,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); } } } @@ -2140,6 +2140,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) @@ -2443,7 +2449,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation { // _sendInputToConnection() asserts that we aren't in focus mode, // but window focus events are always fine to send. - _connection.WriteInput(*out); + _sendInputToConnection(*out); } } diff --git a/src/cascadia/TerminalControl/ControlCore.h b/src/cascadia/TerminalControl/ControlCore.h index 1eb4b532c49..1f0232877af 100644 --- a/src/cascadia/TerminalControl/ControlCore.h +++ b/src/cascadia/TerminalControl/ControlCore.h @@ -307,6 +307,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation winrt::com_ptr _settings{ nullptr }; std::shared_ptr<::Microsoft::Terminal::Core::Terminal> _terminal{ nullptr }; + std::wstring _pendingResponses; // NOTE: _renderEngine must be ordered before _renderer. //