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

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

Open
zadjii-msft opened this issue Aug 21, 2023 · 1 comment · May be fixed by #18228
Open

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

zadjii-msft opened this issue Aug 21, 2023 · 1 comment · May be fixed by #18228
Assignees
@lhecker
Labels
Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Conhost For issues in the Console codebase
Milestone
Terminal v1.23

Comments

@zadjii-msft
Copy link
Member

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.
@zadjii-msft zadjii-msft added Product-Conhost For issues in the Console codebase Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Needs-Discussion Something that requires a team discussion before we can proceed labels Aug 21, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Aug 21, 2023
@lhecker
Copy link
Member

lhecker commented 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:

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 ↩

@lhecker lhecker added Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) labels Aug 21, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot removed Needs-Tag-Fix Doesn't match tag requirements labels Aug 21, 2023
@lhecker lhecker added Priority-2 A description (P2) and removed Priority-1 A description (P1) labels Aug 21, 2023
@zadjii-msft zadjii-msft removed Needs-Discussion Something that requires a team discussion before we can proceed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Aug 21, 2023
@zadjii-msft zadjii-msft added this to the Backlog milestone Aug 21, 2023
@zadjii-msft zadjii-msft mentioned this issue Aug 21, 2023
Closed
@zadjii-msft zadjii-msft changed the title discussion: ReadConsoleInput can read 0 characters and return success and we're just not sure if that's by-design ReadConsoleInput can read 0 characters and return success and it almost certainly shouldn't Aug 21, 2023
jazzdelightsme added a commit to jazzdelightsme/runtime that referenced this issue Feb 20, 2024
@jazzdelightsme
Console.ReadKey should not throw when read returns 0 records
c04ebb0 
This change fixes a problem where Console.ReadKey inappropriately throws
an InvalidOperationException.

The Console.ReadKey function can throw an InvalidOperationException when
the "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, which are waiting for input, and
one dies or is killed. In this case, the native read function returns a
"success" return code, but the "out" parameter indicating the number of
records read is 0, and in this case, Console.ReadKey is throwing, but it
should not be.

Note that this behavior of the underlying platform is "almost certainly"
a bug (microsoft/terminal#15859); however, it
is longstanding behavior, so we would like to avoid blowing up apps who
have the misfortune of stumbling into this situation.

More details in the linked Issue:

Fix dotnet#88697
@jazzdelightsme jazzdelightsme mentioned this issue Feb 20, 2024
Merged
adamsitnik pushed a commit to dotnet/runtime that referenced this issue May 22, 2024
@jazzdelightsme
Console.ReadKey should not throw when read returns 0 records (#98684)
7d8d6ac 
This change fixes a problem where Console.ReadKey inappropriately throws
an InvalidOperationException.

The Console.ReadKey function can throw an InvalidOperationException when
the "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, which are waiting for input, and
one dies or is killed. In this case, the native read function returns a
"success" return code, but the "out" parameter indicating the number of
records read is 0, and in this case, Console.ReadKey is throwing, but it
should not be.

Note that this behavior of the underlying platform is "almost certainly"
a bug (microsoft/terminal#15859); however, it
is longstanding behavior, so we would like to avoid blowing up apps who
have the misfortune of stumbling into this situation.

More details in the linked Issue:

Fix #88697
steveharter pushed a commit to steveharter/runtime that referenced this issue May 28, 2024
@jazzdelightsme @steveharter
Console.ReadKey should not throw when read returns 0 records (dotnet#…
7590837 
…98684)

This change fixes a problem where Console.ReadKey inappropriately throws
an InvalidOperationException.

The Console.ReadKey function can throw an InvalidOperationException when
the "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, which are waiting for input, and
one dies or is killed. In this case, the native read function returns a
"success" return code, but the "out" parameter indicating the number of
records read is 0, and in this case, Console.ReadKey is throwing, but it
should not be.

Note that this behavior of the underlying platform is "almost certainly"
a bug (microsoft/terminal#15859); however, it
is longstanding behavior, so we would like to avoid blowing up apps who
have the misfortune of stumbling into this situation.

More details in the linked Issue:

Fix dotnet#88697
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this issue May 30, 2024
@jazzdelightsme @Ruihan-Yin
Console.ReadKey should not throw when read returns 0 records (dotnet#…
ea3b600 
…98684)

This change fixes a problem where Console.ReadKey inappropriately throws
an InvalidOperationException.

The Console.ReadKey function can throw an InvalidOperationException when
the "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, which are waiting for input, and
one dies or is killed. In this case, the native read function returns a
"success" return code, but the "out" parameter indicating the number of
records read is 0, and in this case, Console.ReadKey is throwing, but it
should not be.

Note that this behavior of the underlying platform is "almost certainly"
a bug (microsoft/terminal#15859); however, it
is longstanding behavior, so we would like to avoid blowing up apps who
have the misfortune of stumbling into this situation.

More details in the linked Issue:

Fix dotnet#88697
@lhecker lhecker self-assigned this Aug 23, 2024
@lhecker lhecker modified the milestones: Backlog, Terminal v1.23 Aug 23, 2024
@lhecker lhecker mentioned this issue Aug 23, 2024
Closed
@dodexahedron dodexahedron mentioned this issue Sep 15, 2024
Open
@lhecker lhecker linked a pull request Nov 21, 2024 that will close this issue
Open
@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lhecker lhecker

Labels
Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Conhost For issues in the Console codebase
Projects
None yet
Milestone
Terminal v1.23
Development

Successfully merging a pull request may close this issue.

Properly implement WaitForData for ReadConsoleInput microsoft/terminal
2 participants
@lhecker @zadjii-msft