starred/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2026-02-04 05:35:20 +00:00
Code Issues 19.0k Packages Projects Releases 20 Wiki Activity

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

New Issue
Closed
opened 2026-01-31 07:12:18 +00:00 by claunia · 1 comment
claunia commented 2026-01-31 07:12:18 +00:00
Owner
Copy Link

Originally created by @zadjii-msft on GitHub (Aug 21, 2023).

Originally assigned to: @lhecker on GitHub.

More discussion notes in https://github.com/dotnet/runtime/issues/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.

Originally created by @zadjii-msft on GitHub (Aug 21, 2023). Originally assigned to: @lhecker on GitHub. More discussion notes in https://github.com/dotnet/runtime/issues/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](https://learn.microsoft.com/en-us/windows/console/readconsoleinput) 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](https://github.com/microsoft/terminal/blob/b556594793006ba49934d9e5291cbf235e18b4b3/src/server/ApiDispatchers.cpp#L127-L140) > > That calls into `ApiRoutines::GetConsoleInputImpl` [microsoft/terminal@`b556594`/src/host/directio.cpp#L73-L78](https://github.com/microsoft/terminal/blob/b556594793006ba49934d9e5291cbf235e18b4b3/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. ----
claunia added the Product-ConhostIssue-BugIn-PRNeeds-Tag-FixArea-ServerPriority-2 labels 2026-01-31 07:12:19 +00:00
claunia commented 2026-01-31 07:12:20 +00:00
Author
Owner
Copy Link

@lhecker commented on GitHub (Aug 21, 2023):

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

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:
d4d59fa339/src/host/readDataDirect.cpp (L138)

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.

@lhecker commented on GitHub (Aug 21, 2023): > 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. [^1] `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: https://github.com/microsoft/terminal/blob/d4d59fa3395012ca37ba12665da4ec11c7dcf9cb/src/host/readDataDirect.cpp#L138 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: ```cpp !(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. [^1]: https://www.youtube.com/@PitchMeetings
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
⛺ Reserved
A11yCO
A11yMAS
A11ySev1
A11ySev2
A11ySev3
A11yTTValidated
A11yUsable
A11yVoiceAccess
A11yWCAG
Area-Accessibility
Area-AtlasEngine
Area-AzureShell
Area-Build
Area-Build
Area-Chat
Area-CmdPal
Area-CodeHealth
Area-Commandline
Area-CookedRead
Area-DefApp
Area-Extensibility
Area-Fonts
Area-GroupPolicy
Area-i18n
Area-Input
Area-Interaction
Area-Interop
Area-Localization
Area-Output
Area-Performance
Area-Portable
Area-Quality
Area-Remoting
Area-Rendering
Area-Schema
Area-Server
Area-Settings
Area-SettingsUI
Area-ShellExtension
Area-ShellExtension
Area-ShellExtension
Area-Suggestions
Area-Suggestions
Area-TerminalConnection
Area-TerminalControl
Area-Theming
Area-UserInterface
Area-VT
Area-Windowing
Area-WPFControl
AutoMerge
Blocking-Ingestion
Culprit-Centennial
Culprit-WinUI
Disability-All
Disability-Blind
Disability-LowVision
Disability-Mobility
External-Blocked-WinUI3
Fixed
Gathering-Data
good first issue
HCL-E+D
HCL-WindowsTerminal
Help Wanted
Impact-Compatibility
Impact-Compliance
Impact-Correctness
Impact-Visual
In-PR
InclusionBacklog
InclusionBacklog-Windows TerminalWin32
InclusionCommitted-202206
Issue-Bug
Issue-Docs
Issue-Feature
Issue-Feature
Issue-Question
Issue-Samples
Issue-Scenario
Issue-Task
Needs-Attention
Needs-Author-Feedback
Needs-Bisect
Needs-Discussion
Needs-Repro
Needs-Tag-Fix
Needs-Tag-Fix
Needs-Triage
No-Recent-Activity
Priority-0
Priority-1
Priority-2
Priority-3
Product-Cmd.exe
Product-Colortool
Product-Colortool
Product-Colortool
Product-Conhost
Product-Conpty
Product-Meta
Product-Powershell
Product-Terminal
Product-WSL
pull-request
Mirrored from GitHub Pull Request
 Resolution-Answered
Resolution-By-Design
Resolution-Duplicate
Resolution-External
Resolution-Fix-Available
Resolution-Fix-Committed
Resolution-No-Repro
Resolution-Won't-Fix
Severity-Blocking
Severity-Crash
Severity-DataLoss
spam
this-will-be-a-breaking-change
Tracking-External
WindowsTerminal_Win32
Work-Item
zAskModeBug
zInbox-Bug
No Label Area-Server In-PR Issue-Bug Needs-Tag-Fix Priority-2 Product-Conhost
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
claunia
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/terminal#20390
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?