ReadConsoleInput can read 0 characters and return success and it almost certainly shouldn't

[zadjii-msft]

More discussion notes in dotnet/runtime#88697. I'm filing this here so we have a local reference to the discussion, even if we just close this out.

Here are some miscellaneous noted comments from that thread:

The Console.ReadKey function can throw an InvalidOperationException when "application does not have a console or when console input has been redirected". That makes sense. However, there is another case where it throws an InvalidOperationException, which happens when there are multiple processes attached to the same console, both waiting for input, and one dies or is killed.

---

BTW it's interesting that the official docs say that it's impossible to read 0 characters on success:

The function does not return until at least one input record has been read.

---

Is it a doc bug, or a code bug?

---

Bear with me as I collect some notes

ApiDispatchers::ServerGetConsoleInput looks like the guy that handles ReadConsoleInput microsoft/terminal@b556594/src/server/ApiDispatchers.cpp#L127-L140

That calls into ApiRoutines::GetConsoleInputImpl microsoft/terminal@b556594/src/host/directio.cpp#L73-L78

That always passes WaitForData as true, in the read from the input buffer. Doc comment on that method:

// - WaitForData - if true, wait until an event is input (if there aren't enough to fill client buffer). if false, return immediately

oh but then I think we start getting into the realm of the console read waits. Those are nasty to try and track down. My gut says it's not impossible that there's a case where a client disconnecting might try to complete the other waits that were pending. Tracking that down would be trickier. I also have no idea which is right, certainly not off the top of my head.

---

[lhecker]

oh but then I think we start getting into the realm of the console read waits. Those are nasty to try and track down.

No actually, it's gonna be super easy, barely an inconvenience. ¹

GetConsoleInputImpl constructs a DirectReadData which is the callback that runs when input data is finally (supposedly) available sometime later. It passes false as WaitForData and so it returns STATUS_SUCCESS when the buffer is empty. It's been like that ever since the first commit on GitHub:

terminal/src/host/readDataDirect.cpp

Line 138 in d4d59fa

false,

But if we compare that with the conhost v1 code where the callback was a function called DirectReadWaitRoutine in directio.cpp then that one passes:

!(a->Flags & CONSOLE_READ_NOWAIT)

Which I think is the expected behavior. In other words, I believe that this is a conhost v1 -> v2 regression.

Footnotes

1. https://www.youtube.com/@PitchMeetings ↩