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.

Sign up for GitHub

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

Jump to bottom

Properly implement WaitForData for ReadConsoleInput #18228

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+58 −128
Open

Properly implement WaitForData for ReadConsoleInput #18228

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 8 additions & 4 deletions OpenConsole.sln
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2304,10 +2304,14 @@ Global
{6515F03F-E56D-4DB4-B23D-AC4FB80DB36F}.Release|x64.Build.0 = Release|x64
{6515F03F-E56D-4DB4-B23D-AC4FB80DB36F}.Release|x86.ActiveCfg = Release|Win32
{6515F03F-E56D-4DB4-B23D-AC4FB80DB36F}.Release|x86.Build.0 = Release|Win32
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|Any CPU.ActiveCfg = Debug|Win32
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|ARM64.ActiveCfg = Release|ARM64
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x64.ActiveCfg = Release|x64
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x86.ActiveCfg = Release|Win32
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|Any CPU.ActiveCfg = AuditMode|x64
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|Any CPU.Build.0 = AuditMode|x64
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|ARM64.ActiveCfg = AuditMode|ARM64
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|ARM64.Build.0 = AuditMode|ARM64
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x64.ActiveCfg = AuditMode|x64
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x64.Build.0 = AuditMode|x64
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x86.ActiveCfg = AuditMode|Win32
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x86.Build.0 = AuditMode|Win32
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.Debug|Any CPU.ActiveCfg = Debug|x64
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.Debug|ARM64.ActiveCfg = Debug|ARM64
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.Debug|ARM64.Build.0 = Debug|ARM64
Expand Down
1 change: 1 addition & 0 deletions src/host/ApiRoutines.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ class ApiRoutines : public IApiRoutines
INPUT_READ_HANDLE_DATA& readHandleState,
const bool IsUnicode,
const bool IsPeek,
const bool IsWaitAllowed,
std::unique_ptr<IWaitRoutine>& waiter) noexcept override;

[[nodiscard]] HRESULT ReadConsoleImpl(IConsoleInputObject& context,
Expand Down
3 changes: 2 additions & 1 deletion src/host/directio.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ using Microsoft::Console::Interactivity::ServiceLocator;
INPUT_READ_HANDLE_DATA& readHandleState,
const bool IsUnicode,
const bool IsPeek,
const bool IsWaitAllowed,
std::unique_ptr<IWaitRoutine>& waiter) noexcept
{
try
Expand All @@ -73,7 +74,7 @@ using Microsoft::Console::Interactivity::ServiceLocator;
const auto Status = inputBuffer.Read(outEvents,
eventReadCount,
IsPeek,
true,
IsWaitAllowed,
IsUnicode,
false);

Expand Down
122 changes: 29 additions & 93 deletions src/host/inputBuffer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,8 +492,7 @@ size_t InputBuffer::Prepend(const std::span<const INPUT_RECORD>& inEvents)
return STATUS_SUCCESS;
}

_vtInputShouldSuppress = true;
auto resetVtInputSuppress = wil::scope_exit([&]() { _vtInputShouldSuppress = false; });
const auto wakeup = _wakeupReadersOnExit();
Copy link
Member Author

Choose a reason for hiding this comment

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

Much cleaner and safer. Also, it's more correct. We would previously call WakeUpReadersWaitingForData unconditionally but the storage could still be empty. This error has probably been in conhost since v1.


// read all of the records out of the buffer, then write the
// prepend ones, then write the original set. We need to do it
Expand All @@ -503,35 +502,15 @@ size_t InputBuffer::Prepend(const std::span<const INPUT_RECORD>& inEvents)
std::deque<INPUT_RECORD> existingStorage;
existingStorage.swap(_storage);

// We will need this variable to pass to _WriteBuffer so it can attempt to determine wait status.
// However, because we swapped the storage out from under it with an empty deque, it will always
// return true after the first one (as it is filling the newly emptied backing deque.)
// Then after the second one, because we've inserted some input, it will always say false.
auto unusedWaitStatus = false;

// write the prepend records
size_t prependEventsWritten;
_WriteBuffer(inEvents, prependEventsWritten, unusedWaitStatus);
FAIL_FAST_IF(!(unusedWaitStatus));
_WriteBuffer(inEvents, prependEventsWritten);

for (const auto& event : existingStorage)
{
_storage.push_back(event);
}

// We need to set the wait event if there were 0 events in the
// input queue when we started.
// Because we did interesting manipulation of the wait queue
// in order to prepend, we can't trust what _WriteBuffer said
// and instead need to set the event if the original backing
// buffer (the one we swapped out at the top) was empty
// when this whole thing started.
if (existingStorage.empty())
{
ServiceLocator::LocateGlobals().hInputEvent.SetEvent();
}
WakeUpReadersWaitingForData();

return prependEventsWritten;
}
catch (...)
Expand Down Expand Up @@ -575,21 +554,9 @@ size_t InputBuffer::Write(const std::span<const INPUT_RECORD>& inEvents)
return 0;
}

_vtInputShouldSuppress = true;
auto resetVtInputSuppress = wil::scope_exit([&]() { _vtInputShouldSuppress = false; });

// Write to buffer.
const auto wakeup = _wakeupReadersOnExit();
size_t EventsWritten;
bool SetWaitEvent;
_WriteBuffer(inEvents, EventsWritten, SetWaitEvent);

if (SetWaitEvent)
{
ServiceLocator::LocateGlobals().hInputEvent.SetEvent();
}

// Alert any writers waiting for space.
WakeUpReadersWaitingForData();
_WriteBuffer(inEvents, EventsWritten);
return EventsWritten;
}
catch (...)
Expand All @@ -607,16 +574,8 @@ try
return;
}

const auto initiallyEmptyQueue = _storage.empty();

const auto wakeup = _wakeupReadersOnExit();
_writeString(text);

if (initiallyEmptyQueue && !_storage.empty())
{
ServiceLocator::LocateGlobals().hInputEvent.SetEvent();
}

WakeUpReadersWaitingForData();
}
CATCH_LOG()

Expand All @@ -625,29 +584,26 @@ CATCH_LOG()
// input buffer and the next application will suddenly get a "\x1b[I" sequence in their input. See GH#13238.
void InputBuffer::WriteFocusEvent(bool focused) noexcept
{
const auto wakeup = _wakeupReadersOnExit();

if (IsInVirtualTerminalInputMode())
{
if (const auto out = _termInput.HandleFocus(focused))
{
_HandleTerminalInputCallback(*out);
_writeString(*out);
}
}
else
{
// This is a mini-version of Write().
const auto wasEmpty = _storage.empty();
_storage.push_back(SynthesizeFocusEvent(focused));
if (wasEmpty)
{
ServiceLocator::LocateGlobals().hInputEvent.SetEvent();
}
WakeUpReadersWaitingForData();
}
}

// Returns true when mouse input started. You should then capture the mouse and produce further events.
bool InputBuffer::WriteMouseEvent(til::point position, const unsigned int button, const short keyState, const short wheelDelta)
{
const auto wakeup = _wakeupReadersOnExit();

if (IsInVirtualTerminalInputMode())
{
// This magic flag is "documented" at https://msdn.microsoft.com/en-us/library/windows/desktop/ms646301(v=vs.85).aspx
Expand All @@ -669,7 +625,7 @@ bool InputBuffer::WriteMouseEvent(til::point position, const unsigned int button

if (const auto out = _termInput.HandleMouse(position, button, keyState, wheelDelta, state))
{
_HandleTerminalInputCallback(*out);
_writeString(*out);
return true;
}
}
Expand All @@ -690,6 +646,22 @@ static bool IsPauseKey(const KEY_EVENT_RECORD& event)
return ctrlButNotAlt && event.wVirtualKeyCode == L'S';
}

void InputBuffer::_wakeupReadersImpl(bool initiallyEmpty)
{
if (!_storage.empty())
{
// It would be fine to call SetEvent() unconditionally,
// but technically we only need to ResetEvent() if the buffer is empty,
// and SetEvent() once it stopped being so, which is what this code does.
if (initiallyEmpty)
{
ServiceLocator::LocateGlobals().hInputEvent.SetEvent();
}

WakeUpReadersWaitingForData();
}
}

// Routine Description:
// - Coalesces input events and transfers them to storage queue.
// Arguments:
Expand All @@ -702,13 +674,11 @@ static bool IsPauseKey(const KEY_EVENT_RECORD& event)
// Note:
// - The console lock must be held when calling this routine.
// - will throw on failure
void InputBuffer::_WriteBuffer(const std::span<const INPUT_RECORD>& inEvents, _Out_ size_t& eventsWritten, _Out_ bool& setWaitEvent)
void InputBuffer::_WriteBuffer(const std::span<const INPUT_RECORD>& inEvents, _Out_ size_t& eventsWritten)
{
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();

eventsWritten = 0;
setWaitEvent = false;
const auto initiallyEmptyQueue = _storage.empty();
const auto initialInEventsSize = inEvents.size();
const auto vtInputMode = IsInVirtualTerminalInputMode();

Expand Down Expand Up @@ -739,7 +709,7 @@ void InputBuffer::_WriteBuffer(const std::span<const INPUT_RECORD>& inEvents, _O
// GH#11682: TerminalInput::HandleKey can handle both KeyEvents and Focus events seamlessly
if (const auto out = _termInput.HandleKey(inEvent))
{
_HandleTerminalInputCallback(*out);
_writeString(*out);
eventsWritten++;
continue;
}
Expand All @@ -759,10 +729,6 @@ void InputBuffer::_WriteBuffer(const std::span<const INPUT_RECORD>& inEvents, _O
_storage.push_back(inEvent);
++eventsWritten;
}
if (initiallyEmptyQueue && !_storage.empty())
{
setWaitEvent = true;
}
}

// Routine Description::
Expand Down Expand Up @@ -826,36 +792,6 @@ bool InputBuffer::IsInVirtualTerminalInputMode() const
return WI_IsFlagSet(InputMode, ENABLE_VIRTUAL_TERMINAL_INPUT);
}

// Routine Description:
// - Handler for inserting key sequences into the buffer when the terminal emulation layer
// has determined a key can be converted appropriately into a sequence of inputs
// Arguments:
// - inEvents - Series of input records to insert into the buffer
// Return Value:
// - <none>
void InputBuffer::_HandleTerminalInputCallback(const TerminalInput::StringType& text)
Copy link
Member Author

Choose a reason for hiding this comment

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

The second fix. It's also redundant with _writeString, especially with _vtInputShouldSuppress kicked out.

{
try
{
if (text.empty())
{
return;
}

_writeString(text);

if (!_vtInputShouldSuppress)
{
ServiceLocator::LocateGlobals().hInputEvent.SetEvent();
WakeUpReadersWaitingForData();
}
}
catch (...)
{
LOG_HR(wil::ResultFromCaughtException());
}
}

void InputBuffer::_writeString(const std::wstring_view& text)
{
for (const auto& wch : text)
Expand Down
17 changes: 10 additions & 7 deletions src/host/inputBuffer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,17 +84,20 @@ class InputBuffer final : public ConsoleObjectHeader
bool _writePartialByteSequenceAvailable = false;
Microsoft::Console::VirtualTerminal::TerminalInput _termInput;

// This flag is used in _HandleTerminalInputCallback
// If the InputBuffer leads to a _HandleTerminalInputCallback call,
// we should suppress the wakeup functions.
// Otherwise, we should be calling them.
bool _vtInputShouldSuppress{ false };
// Wakes up readers waiting for data to be in the input buffer.
auto _wakeupReadersOnExit()
{
const auto initiallyEmpty = _storage.empty();
return wil::scope_exit([this, initiallyEmpty]() {
_wakeupReadersImpl(initiallyEmpty);
});
}

void _wakeupReadersImpl(bool initiallyEmpty);
void _switchReadingMode(ReadingMode mode);
void _switchReadingModeSlowPath(ReadingMode mode);
void _WriteBuffer(const std::span<const INPUT_RECORD>& inRecords, _Out_ size_t& eventsWritten, _Out_ bool& setWaitEvent);
void _WriteBuffer(const std::span<const INPUT_RECORD>& inRecords, _Out_ size_t& eventsWritten);
bool _CoalesceEvent(const INPUT_RECORD& inEvent) noexcept;
void _HandleTerminalInputCallback(const Microsoft::Console::VirtualTerminal::TerminalInput::StringType& text);
void _writeString(const std::wstring_view& text);

#ifdef UNIT_TESTING
Expand Down
2 changes: 1 addition & 1 deletion src/host/readDataDirect.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ try
*pReplyStatus = _pInputBuffer->Read(_outEvents,
amountToRead,
false,
false,
true,
Copy link
Member Author

Choose a reason for hiding this comment

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

The first fix.

fIsUnicode,
false);

Expand Down
28 changes: 6 additions & 22 deletions src/server/ApiDispatchers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ constexpr T saturate(auto val)
*pInputReadHandleData,
a->Unicode,
fIsPeek,
fIsWaitAllowed,
waiter);

// We must return the number of records in the message payload (to alert the client)
Expand All @@ -191,30 +192,13 @@ constexpr T saturate(auto val)
size_t cbWritten;
LOG_IF_FAILED(SizeTMult(outEvents.size(), sizeof(INPUT_RECORD), &cbWritten));

if (nullptr != waiter.get())
if (waiter)
{
// In some circumstances, the read may have told us to wait because it didn't have data,
// but the client explicitly asked us to return immediate. In that case, we'll convert the
// wait request into a "0 bytes found, OK".

if (fIsWaitAllowed)
{
hr = ConsoleWaitQueue::s_CreateWait(m, waiter.release());
if (SUCCEEDED(hr))
{
*pbReplyPending = TRUE;
hr = CONSOLE_STATUS_WAIT;
}
}
Comment on lines -200 to -208
Copy link
Member Author

Choose a reason for hiding this comment

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

Makes no sense IMO to check this here if we can just pass down fIsWaitAllowed and avoid a waiter from being created in the first place.

else
hr = ConsoleWaitQueue::s_CreateWait(m, waiter.release());
if (SUCCEEDED(hr))
{
// If wait isn't allowed and the routine generated a
// waiter, say there was nothing to be
// retrieved right now.
// The waiter will be auto-freed in the smart pointer.

cbWritten = 0;
hr = S_OK;
*pbReplyPending = TRUE;
hr = CONSOLE_STATUS_WAIT;
}
}
else
Expand Down
1 change: 1 addition & 0 deletions src/server/IApiRoutines.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ class __declspec(novtable) IApiRoutines
INPUT_READ_HANDLE_DATA& readHandleState,
const bool IsUnicode,
const bool IsPeek,
const bool IsWaitAllowed,
std::unique_ptr<IWaitRoutine>& waiter) noexcept = 0;

[[nodiscard]] virtual HRESULT ReadConsoleImpl(IConsoleInputObject& context,
Expand Down
Loading