Properly implement WaitForData for ReadConsoleInput by lhecker · Pull Request #18228 · microsoft/terminal

lhecker · 2024-11-21T00:42:24Z

There were two bugs:

Ever since the conhost v1 -> v2 rewrite the readDataDirect.cpp
implementation incorrectly passed false as the wait flag.
The unintentional mistake is obvious in hindsight as the
check for CONSOLE_STATUS_WAIT makes no sense in this case.
The ConPTY integration into InputBuffer was done incorrectly,
as it would unconditionally wake up the readers/waiters without
checking if the buffer is now actually non-empty.

Validation Steps Performed

Test code:

#include <Windows.h>
#include <stdio.h>

int main() {
    HANDLE in = GetStdHandle(STD_INPUT_HANDLE);
    INPUT_RECORD buf[128];
    DWORD read;

    SetConsoleMode(
        in,
        ENABLE_PROCESSED_INPUT | ENABLE_VIRTUAL_TERMINAL_INPUT
    );

    for (int i = 0; ReadConsoleInputW(in, buf, 128, &read); ++i) {
        printf("%d read=%lu\n", i, read);
    }

    return 0;
}

Run it under Windows Terminal and type any input. >50% of all
inputs will result in read=0. This is fixed after this PR.

lhecker · 2024-11-21T00:44:50Z

src/host/inputBuffer.cpp


-        _vtInputShouldSuppress = true;
-        auto resetVtInputSuppress = wil::scope_exit([&]() { _vtInputShouldSuppress = false; });
+        const auto wakeup = _wakeupReadersOnExit();


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.

I can't remember 4 years ago why _vtInputShouldSuppress and input wake muffling was added...

It was due to the "circular" control flow, as _termInput was set up with the callback (boo!) _HandleTerminalInputCallback. Additionally, the Prepend was implemented with a series of 2 "Append"s. The first one would process the input and call into _termInput which would then invoke _HandleTerminalInputCallback. This meant that the event was flagged and the waiters woken up before the code had finished to append the 2nd half. To fix this, the flag was added to make the callback stop waking the waiters up.

This PR solves the "knot" by moving all of the waking logic out of the internal append-function and into the outermost, public functions. Now they have perfect control over when to wake up something and when to not do it.

lhecker · 2024-11-21T00:45:45Z

src/host/inputBuffer.cpp

-// - inEvents - Series of input records to insert into the buffer
-// Return Value:
-// - <none>
-void InputBuffer::_HandleTerminalInputCallback(const TerminalInput::StringType& text)


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

lhecker · 2024-11-21T00:45:56Z

src/host/readDataDirect.cpp

                                        amountToRead,
                                        false,
-                                        false,
+                                        true,


The first fix.

lhecker · 2024-11-21T00:46:44Z

src/server/ApiDispatchers.cpp

-        if (fIsWaitAllowed)
-        {
-            hr = ConsoleWaitQueue::s_CreateWait(m, waiter.release());
-            if (SUCCEEDED(hr))
-            {
-                *pbReplyPending = TRUE;
-                hr = CONSOLE_STATUS_WAIT;
-            }
-        }


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.

carlos-zamora · 2024-11-25T21:53:22Z

OpenConsole.sln

-        {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


(Double checking) intentional?

Huh, I was certain I reverted this...

carlos-zamora · 2024-11-25T21:54:22Z

src/host/directio.cpp

                                                       INPUT_READ_HANDLE_DATA& readHandleState,
                                                       const bool IsUnicode,
                                                       const bool IsPeek,
+                                                       const bool IsWaitAllowed,


nit: update the function comment above

carlos-zamora · 2024-11-25T22:02:43Z

src/host/inputBuffer.cpp

 // 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)


nit: setWaitEvent is still documented above. It should be removed.

…d-console-input

There were two bugs: * Ever since the conhost v1 -> v2 rewrite the `readDataDirect.cpp` implementation incorrectly passed `false` as the wait flag. The unintentional mistake is obvious in hindsight as the check for `CONSOLE_STATUS_WAIT` makes no sense in this case. * The ConPTY integration into `InputBuffer` was done incorrectly, as it would unconditionally wake up the readers/waiters without checking if the buffer is now actually non-empty. Closes #15859 ## Validation Steps Performed Test code: ```cpp #include <Windows.h> #include <stdio.h> int main() { HANDLE in = GetStdHandle(STD_INPUT_HANDLE); INPUT_RECORD buf[128]; DWORD read; SetConsoleMode( in, ENABLE_PROCESSED_INPUT | ENABLE_VIRTUAL_TERMINAL_INPUT ); for (int i = 0; ReadConsoleInputW(in, buf, 128, &read); ++i) { printf("%d read=%lu\n", i, read); } return 0; } ``` Run it under Windows Terminal and type any input. >50% of all inputs will result in `read=0`. This is fixed after this PR. (cherry picked from commit 5c55144) Service-Card-Id: PVTI_lADOAF3p4s4AmhmQzgaHLrQ Service-Version: 1.22

Properly implement WaitForData for ReadConsoleInput

2388624

lhecker force-pushed the dev/lhecker/15859-read-console-input branch from dab80c8 to 2388624 Compare November 21, 2024 00:43

lhecker commented Nov 21, 2024

View reviewed changes

carlos-zamora approved these changes Nov 25, 2024

View reviewed changes

lhecker added 3 commits December 3, 2024 17:18

Merge remote-tracking branch 'origin/main' into dev/lhecker/15859-rea…

4607d24

…d-console-input

Address feedback

402b21e

Fix AuditMode & tests

828fae1

DHowett approved these changes Dec 3, 2024

View reviewed changes

lhecker merged commit 5c55144 into main Dec 3, 2024

lhecker deleted the dev/lhecker/15859-read-console-input branch December 3, 2024 21:12

DHowett added this to Inbox Servicing Pipeline and 1.22 Servicing Pipeline May 7, 2025

github-project-automation bot moved this to To Cherry Pick in 1.22 Servicing Pipeline May 7, 2025

github-project-automation bot moved this to To Cherry Pick in Inbox Servicing Pipeline May 7, 2025

DHowett moved this from To Cherry Pick to Cherry Picked in 1.22 Servicing Pipeline May 7, 2025

DHowett mentioned this pull request May 22, 2025

Shell exits after processed input enabled by child process #18941

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Properly implement WaitForData for ReadConsoleInput#18228

Properly implement WaitForData for ReadConsoleInput#18228
lhecker merged 4 commits intomainfrom
dev/lhecker/15859-read-console-input

lhecker commented Nov 21, 2024

Uh oh!

lhecker Nov 21, 2024

Uh oh!

DHowett Dec 3, 2024

Uh oh!

lhecker Dec 3, 2024 •

edited

Loading

Uh oh!

lhecker Nov 21, 2024

Uh oh!

lhecker Nov 21, 2024

Uh oh!

lhecker Nov 21, 2024

Uh oh!

carlos-zamora Nov 25, 2024

Uh oh!

lhecker Nov 25, 2024

Uh oh!

carlos-zamora Nov 25, 2024

Uh oh!

carlos-zamora Nov 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

lhecker commented Nov 21, 2024

Validation Steps Performed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhecker Dec 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lhecker Dec 3, 2024 •

edited

Loading