Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve reliability of VT responses #17786

Merged
merged 2 commits into from
Aug 23, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 28 additions & 21 deletions src/cascadia/TerminalControl/ControlCore.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -419,6 +419,20 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// Return Value:
// - <none>
void ControlCore::_sendInputToConnection(std::wstring_view wstr)
{
if (!wstr.empty())
{
_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())
{
Expand All @@ -435,21 +449,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);
}
Comment on lines -448 to -451
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SendInput now does what _sendInputToConnection previously did.
_sendInputToConnection is now a write-directly-to-connection function.


bool ControlCore::SendCharEvent(const wchar_t ch,
const WORD scanCode,
const ::Microsoft::Terminal::Core::ControlKeyStates modifiers)
Expand Down Expand Up @@ -485,7 +488,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
}
if (out)
{
_sendInputToConnection(*out);
SendInput(*out);
return true;
}
return false;
Expand Down Expand Up @@ -643,7 +646,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
}
if (out)
{
_sendInputToConnection(*out);
SendInput(*out);
return true;
}
return false;
Expand All @@ -662,7 +665,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
}
if (out)
{
_sendInputToConnection(*out);
SendInput(*out);
return true;
}
return false;
Expand Down Expand Up @@ -1412,7 +1415,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 @@ -2107,7 +2110,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 @@ -2164,6 +2167,12 @@ namespace winrt::Microsoft::Terminal::Control::implementation
_terminal->Write(hstr);
}

if (!_pendingResponses.empty())
{
_sendInputToConnection(_pendingResponses);
_pendingResponses.clear();
}
Comment on lines +2167 to +2171
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the bug. It also fixes 2nd bug: We'd previously use the old _sendInputToConnection for responses but this would check whether we're read-only. We shouldn't do that for VT responses.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh this is clever. it replies with it immediately after the write that caused a reply?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah exactly. This is much better than before, because it's now processing an entire VT string atomically without unlocking in between. Not sure why I haven't done this from the get-go.


// Start the throttled update of where our hyperlinks are.
const auto shared = _shared.lock_shared();
if (shared->outputIdle)
Expand Down Expand Up @@ -2480,9 +2489,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);
}
}

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 @@ -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.
//
Expand Down
1 change: 0 additions & 1 deletion src/cascadia/TerminalCore/TerminalApi.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ void Terminal::ReturnResponse(const std::wstring_view response)
{
if (_pfnWriteInput && !response.empty())
{
const auto suspension = _readWriteLock.suspend();
_pfnWriteInput(response);
}
}
Expand Down
Loading