Skip to content

Commit

Permalink
Improve reliability of VT responses (#17786)
Browse files Browse the repository at this point in the history
* 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 b07589e)
Service-Card-Id: PVTI_lADOAF3p4s4AmhmszgSK_Dw
Service-Version: 1.21
  • Loading branch information
lhecker authored and DHowett committed Aug 23, 2024
1 parent 1519b6f commit e60afb6
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 19 deletions.
44 changes: 25 additions & 19 deletions src/cascadia/TerminalControl/ControlCore.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -398,6 +398,17 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// Return Value:
// - <none>
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:
// - <none>
void ControlCore::SendInput(const std::wstring_view wstr)
{
if (wstr.empty())
{
Expand All @@ -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:
// - <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)
Expand Down Expand Up @@ -464,7 +464,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
}
if (out)
{
_sendInputToConnection(*out);
SendInput(*out);
return true;
}
return false;
Expand Down Expand Up @@ -622,7 +622,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
}
if (out)
{
_sendInputToConnection(*out);
SendInput(*out);
return true;
}
return false;
Expand All @@ -641,7 +641,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
}
if (out)
{
_sendInputToConnection(*out);
SendInput(*out);
return true;
}
return false;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);
}
}
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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);
}
}

Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalControl/ControlCore.h
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,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.
//
Expand Down

0 comments on commit e60afb6

Please sign in to comment.