Skip to content

Commit

Permalink
Reduce likelihood of races between stdout and cooked stdin reads (#18326
Browse files Browse the repository at this point in the history 
)

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. ✅

(cherry picked from commit 1040035)
Service-Card-Id: PVTI_lADOAF3p4s4AmhmQzgWppag
Service-Version: 1.22
  • Loading branch information
@lhecker @DHowett
lhecker authored and DHowett committed Jan 28, 2025
1 parent e435c16 commit 26f8260
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 18 deletions.
53 changes: 38 additions & 15 deletions src/host/readDataCooked.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -285,22 +283,29 @@ 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.
// Then, after reflow, we'll be able to ask the buffer where it went (the new origin).
// 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);
}
Expand All @@ -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.
Expand Down Expand Up @@ -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();
Expand All @@ -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
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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;
Expand All @@ -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 };
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -1092,7 +1115,7 @@ void COOKED_READ_DATA::_redisplay()
if (_clearPending)
{
_clearPending = false;
_appendCUP(output, _originInViewport);
_appendCUP(output, originInViewport);
output.append(L"\x1b[J");
}

Expand All @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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");

Expand Down
5 changes: 3 additions & 2 deletions src/host/readDataCooked.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class COOKED_READ_DATA final : public ReadData
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;
Expand Down Expand Up @@ -129,6 +129,7 @@ class COOKED_READ_DATA final : public ReadData
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;
Expand Down Expand Up @@ -166,7 +167,7 @@ class COOKED_READ_DATA final : public ReadData
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.
Expand Down
2 changes: 1 addition & 1 deletion src/host/selectionInput.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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())
{
Expand Down

0 comments on commit 26f8260

Please sign in to comment.