microsoft/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2025-12-10 17:53:06 -06:00
Code Issues Packages Projects Releases 138 Wiki Activity

Reduce likelihood of races between stdout and cooked stdin reads (#18326)

Browse Source 
As explained in the comment on `_getViewportCursorPosition`, printing
to stdout after initiating a cooked stdin reads is a race condition
between the application and the terminal. But we can significantly
reduce the likelihood of this being obvious with this change.

Related to #18265
Possibly related to #18081

## Validation Steps Performed

Execute the following Go code and start typing:
```go
package main

import (
	"fmt"
	"time"
)

func main() {
	go func() {
		time.Sleep(50 * time.Millisecond)
		fmt.Printf("Here is a prompt! >")
	}()

	var text string
	fmt.Scanln(&text)
}
```

Without this change the prompt will disappear,
and with this change in place, it'll work as expected. ✅
This commit is contained in:
Leonard Hecker 2025-01-27 14:37:04 -08:00 committed by GitHub
parent 08f9afe315
commit 1040035b55
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 42 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

53
src/host/readDataCooked.cpp
View File

@ -61,9 +61,6 @@ COOKED_READ_DATA::COOKED_READ_DATA(_In_ InputBuffer* const pInputBuffer,
THROW_IF_FAILED(_screenInfo.GetMainBuffer().AllocateIoHandle(ConsoleHandleData::HandleType::Output, GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, _tempHandle));
#endif
const auto cursorPos = _getViewportCursorPosition();
_originInViewport = cursorPos;
if (!initialData.empty())
{
// The console API around `nInitialChars` in `CONSOLE_READCONSOLE_CONTROL` is pretty weird.
@ -94,6 +91,7 @@ COOKED_READ_DATA::COOKED_READ_DATA(_In_ InputBuffer* const pInputBuffer,
// It replicates part of the _redisplay() logic to layout the text at various
// starting positions until it finds one that matches the current cursor position.
const auto cursorPos = _getViewportCursorPosition();
const auto size = _screenInfo.GetVtPageArea().Dimensions();
// Guess the initial cursor position based on the string length, assuming that 1 char = 1 column.
@ -285,14 +283,21 @@ bool COOKED_READ_DATA::Read(const bool isUnicode, size_t& numBytes, ULONG& contr
// Printing wide glyphs at the end of a row results in a forced line wrap and a padding whitespace to be inserted.
// When the text buffer resizes these padding spaces may vanish and the _distanceCursor and _distanceEnd measurements become inaccurate.
// To fix this, this function is called before a resize and will clear the input line. Afterwards, RedrawAfterResize() will restore it.
// To fix this, this function is called before a resize and will clear the input line. Afterward, RedrawAfterResize() will restore it.
void COOKED_READ_DATA::EraseBeforeResize()
{
// If we've already erased the buffer, we don't need to do it again.
if (_redrawPending)
{
return;
}
// If we don't have an origin, we've never had user input, and consequently there's nothing to erase.
if (!_originInViewport)
{
return;
}
_redrawPending = true;
// Position the cursor the start of the prompt before reflow.
@ -300,7 +305,7 @@ void COOKED_READ_DATA::EraseBeforeResize()
// This uses the buffer APIs directly, so that we don't emit unnecessary VT into ConPTY's output.
auto& textBuffer = _screenInfo.GetTextBuffer();
auto& cursor = textBuffer.GetCursor();
auto cursorPos = _originInViewport;
auto cursorPos = *_originInViewport;
_screenInfo.GetVtPageArea().ConvertFromOrigin(&cursorPos);
cursor.SetPosition(cursorPos);
}
@ -316,7 +321,10 @@ void COOKED_READ_DATA::RedrawAfterResize()
_redrawPending = false;
// Get the new cursor position after the reflow, since it may have changed.
_originInViewport = _getViewportCursorPosition();
if (_originInViewport)
{
_originInViewport = _getViewportCursorPosition();
}
// Ensure that we don't use any scroll sequences or try to clear previous pager contents.
// They have all been erased with the CSI J above.
@ -348,7 +356,7 @@ bool COOKED_READ_DATA::PresentingPopup() const noexcept
return !_popups.empty();
}
til::point_span COOKED_READ_DATA::GetBoundaries() const noexcept
til::point_span COOKED_READ_DATA::GetBoundaries() noexcept
{
const auto viewport = _screenInfo.GetViewport();
const auto virtualViewport = _screenInfo.GetVtPageArea();
@ -357,7 +365,7 @@ til::point_span COOKED_READ_DATA::GetBoundaries() const noexcept
const til::point max{ viewport.RightInclusive(), viewport.BottomInclusive() };
// Convert from VT-viewport-relative coordinate space back to the console one.
auto beg = _originInViewport;
auto beg = _getOriginInViewport();
virtualViewport.ConvertFromOrigin(&beg);
// Since the pager may be longer than the viewport is tall, we need to clamp the coordinates to still remain within
@ -831,6 +839,20 @@ til::point COOKED_READ_DATA::_getViewportCursorPosition() const noexcept
return cursorPos;
}
// Some applications initiate a read on stdin and _then_ print the prompt prefix to stdout.
// While that's not correct (because it's a race condition), we can make it significantly
// less bad by delaying the calculation of the origin until we actually need it.
// This turns it from a race between application and terminal into a race between
// application and user, which is much less likely to hit.
til::point COOKED_READ_DATA::_getOriginInViewport() noexcept
{
if (!_originInViewport)
{
_originInViewport.emplace(_getViewportCursorPosition());
}
return *_originInViewport;
}
void COOKED_READ_DATA::_replace(size_t offset, size_t remove, const wchar_t* input, size_t count)
{
const auto size = _buffer.size();
@ -885,7 +907,8 @@ void COOKED_READ_DATA::_redisplay()
}
const auto size = _screenInfo.GetVtPageArea().Dimensions();
auto originInViewportFinal = _originInViewport;
auto originInViewport = _getOriginInViewport();
auto originInViewportFinal = originInViewport;
til::point cursorPositionFinal;
til::point pagerPromptEnd;
std::vector<Line> lines;
@ -894,7 +917,7 @@ void COOKED_READ_DATA::_redisplay()
// and if MSVC says that then that must be true.
for (;;)
{
cursorPositionFinal = { _originInViewport.x, 0 };
cursorPositionFinal = { originInViewport.x, 0 };
// Construct the first line manually so that it starts at the correct horizontal position.
LayoutResult res{ .column = cursorPositionFinal.x };
@ -1057,8 +1080,8 @@ void COOKED_READ_DATA::_redisplay()
if (gsl::narrow_cast<til::CoordType>(lines.size()) > size.height && originInViewportFinal.x != 0)
{
lines.clear();
_originInViewport.x = 0;
_bufferDirtyBeg = 0;
originInViewport.x = 0;
originInViewportFinal = {};
continue;
}
@ -1092,7 +1115,7 @@ void COOKED_READ_DATA::_redisplay()
if (_clearPending)
{
_clearPending = false;
_appendCUP(output, _originInViewport);
_appendCUP(output, originInViewport);
output.append(L"\x1b[J");
}
@ -1111,7 +1134,7 @@ void COOKED_READ_DATA::_redisplay()
// The check for origin == {0,0} is important because it ensures that we "own" the entire viewport and
// that scrolling our contents doesn't scroll away the user's output that may still be in the viewport.
// (Anything below the origin is assumed to belong to us.)
if (const auto delta = pagerContentTop - _pagerContentTop; delta != 0 && _originInViewport == til::point{})
if (const auto delta = pagerContentTop - _pagerContentTop; delta != 0 && originInViewport == til::point{})
{
const auto deltaAbs = abs(delta);
til::CoordType beg = 0;
@ -1179,7 +1202,7 @@ void COOKED_READ_DATA::_redisplay()
for (til::CoordType i = 0; i < pagerHeight; i++)
{
const auto row = std::min(_originInViewport.y + i, size.height - 1);
const auto row = std::min(originInViewport.y + i, size.height - 1);
// If the last write left the cursor at the end of a line, the next write will start at the beginning of the next line.
// This avoids needless calls to _appendCUP. The reason it's here and not at the end of the loop is similar to how
@ -1223,7 +1246,7 @@ void COOKED_READ_DATA::_redisplay()
if (pagerHeight < pagerHeightPrevious)
{
const auto row = std::min(_originInViewport.y + pagerHeight, size.height - 1);
const auto row = std::min(originInViewport.y + pagerHeight, size.height - 1);
_appendCUP(output, { 0, row });
output.append(L"\x1b[K");

5
src/host/readDataCooked.hpp
View File

@ -36,7 +36,7 @@ public:
void SetInsertMode(bool insertMode) noexcept;
bool IsEmpty() const noexcept;
bool PresentingPopup() const noexcept;
til::point_span GetBoundaries() const noexcept;
til::point_span GetBoundaries() noexcept;
private:
static constexpr size_t CommandNumberMaxInputLength = 5;
@ -129,6 +129,7 @@ private:
void _handlePostCharInputLoop(bool isUnicode, size_t& numBytes, ULONG& controlKeyState);
void _transitionState(State state) noexcept;
til::point _getViewportCursorPosition() const noexcept;
til::point _getOriginInViewport() noexcept;
void _replace(size_t offset, size_t remove, const wchar_t* input, size_t count);
void _replace(const std::wstring_view& str);
std::wstring_view _slice(size_t from, size_t to) const noexcept;
@ -166,7 +167,7 @@ private:
bool _redrawPending = false;
bool _clearPending = false;
til::point _originInViewport;
std::optional<til::point> _originInViewport;
// This value is in the pager coordinate space. (0,0) is the first character of the
// first line, independent on where the prompt actually appears on the screen.
// The coordinate is "end exclusive", so the last character is 1 in front of it.

2
src/host/selectionInput.cpp
View File

@ -926,7 +926,7 @@ bool Selection::_HandleMarkModeSelectionNav(const INPUT_KEY_INFO* const pInputKe
// - If true, the boundaries returned are valid. If false, they should be discarded.
[[nodiscard]] bool Selection::s_GetInputLineBoundaries(_Out_opt_ til::point* const pcoordInputStart, _Out_opt_ til::point* const pcoordInputEnd)
{
const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
if (gci.HasPendingCookedRead())
{