dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.38k stars 4.75k forks source link

Console.ReadLine echoes first few hundred milliseconds of input on Linux/Mac #24456

Closed rahl0b10 closed 4 years ago

rahl0b10 commented 6 years ago

Background

I am not a console programmer, but was helping someone learn some foundations of programming and fundamental syntax of C# in a .NET Core console application. When demonstrating loops I found that following simple code has an unexpected behavior. On the first pass through the while loop, the input is echoed to stdout.

Example Program

            var retries = 10;
            var input = string.Empty;
            var answer = "Cod3";
            while(retries-- > 0 && input != answer){ 
                input = Console.ReadLine();
            }
            Console.WriteLine(retries > 0 ? "CONGRATS!" : "GAME OVER!");

Example Output 1: Values entered at slow pace

If I slowly entered the numbers 1 through 10 and hit return after each value, the following will result:

    1
    1  <-- Echoed to stdout, delayed several hundred milliseconds from the input.
    2
    3
    4
    5
    6
    7
    8
    9
    10
    GAME OVER!

Example Output 2: Values entered at rapid pace

If I rapidly entered the numbers 1 through 10 and hit return after each value, the following will result:

     1
     2
     3
     4
     4
     51  <-- Echo
     2  <-- Echo
     3  <-- Echo
     4  <-- Echo
     4  <-- Echo
     5  <-- Echo
     6
     7
     8
     9
     GAME OVER!

Environment

OS: OS X v10.13.1 Shell: zsh v5.3, GNU bash v3.2.57 .Net Core: v2.0.3

danmoseley commented 6 years ago

Thanks for the thorough bug report @rahl0b10 ... someone will likely look in the new year.

joperezr commented 6 years ago

I can repro this using the latest build of release/2.1. @joshfree I'm not sure when I will have some time to look at this one, do you think it should target 2.1?

joshfree commented 6 years ago

While I'd love to fix every bug in 2.1, I think this can move to the next release given no other reports of the issue thus far (less impact) since we're running out of time.

ghost commented 6 years ago

In addition to MacOS, it is reproducible on Linux (tried Debian and Alpine) as well, but not on Windows. Interestingly, it only happens when we type the input. If we redirect to stdin (which is lightning fast), line-by-line for example, then it doesn't show those numbers.

Tried in bash, ash and sh, so it probably is related to ConsolePal.Unix?

Debian:

Simple redirect:

dotnet run < /etc/*-release

Line by line redirect:

while read line; do dotnet run; done < /etc/*-release

@tmds, @omajid, might have some insights, otherwise just fyi. 😄

wfurt commented 6 years ago

This is probably race before reseting TTY. When stdin is terminal, it may have control logic attached to it. If somebody wants to experiment, "stty" may be useful. Some applications have explicit option to be either interactive (and perhaps for allocating pseudo TTY) or not. 'dotne' does not make that distinction as far as I know.

joperezr commented 6 years ago

I actually just looked into this. The main problem in here is that on Linux, by default, all input is echoed. We basically had two options when designing the Console.Read functionality, which was either initializing the terminal with the echo bit off or have it on and then just turning it off when we are reading stuff.

Turning it off for the whole terminal has a bunch of cons, mainly that this would be an effect on the entire app, so other processes that get launched with Process.Start would inherit the echo bit off. Because of this, we decided to go with the second option, which is to turn off echo only when we are reading, and then re-enable after we are done. This particular bug happens when stdin gets the input right before we PInvoke to turn off echoing so it echoes it to Std out. More info about this here

After chatting with @stephentoub, one way to fix this would probably be to try re-implementing console to use curses library since they already handle this case for you. Doing so will also bring additional benefits like not being broken by ncurses terminfo format updates. That said, this is probably a larger workitem that has to be investigated, so we won't be able to get this in time for 2.1.

joperezr commented 6 years ago

In case anybody is interested in helping fixing this, the next steps for this is to investigate what would it take to have Console use curses library.

ghost commented 6 years ago

Some related techniques https://stackoverflow.com/a/41933275

wfurt commented 6 years ago

Is there something "dotnet" binary can do @joperezr ? It would be also interesting to see what stand-alone app does. If this is concern for particular application we should provide enough functionality to reset terminal to expected state before reading. That would make this predictable. Switching to curses is whole different impact IMHO and adds unnecessary dependency - unless we want to do ASCII graphic.

ghost commented 6 years ago

@wfurt, you are spot on. I am unable to reproduce this issue if i execute the assembly directly:

dotnet bin/Release/netcoreapp2.1/console_test.dll

instead of dotnet run.

+1 on avoiding dependency on ncurses (if we can).

ghost commented 6 years ago

@wfurt, @joperezr, I have asked a question at https://github.com/dotnet/cli/issues/8917. Maybe there is something that can be optimized at dotnet/muxer level for dotnet run --no-build -c Release to mitigate (if not permanently fix) this issue?

joperezr commented 6 years ago

I think that just running dotnet exec bin\Release\yourApp.dll is already doing the exact same thing you want so I doubt they will add an option for no-build to dotnet run. It is true that it is harder to repro this when calling exec instead of run but in the end, there will still be a race where if the echoing is not turned off yet while the user is typing, then echoing will still continue to happen. That is why my suggestion on fixing this, should be not to work around it, but to instead either use curses or try to solve this problem in a similar fashion as how curses handle it.

ghost commented 6 years ago

dotnet run --no-build already exists. :)

Yeah we can check how neovim and ncurses are handling it

tmds commented 6 years ago

I'm looking a bit deeper into this issue, this is what I understand: On Windows, things are not echoed until they are ReadKey/ReadLined by the application. While on Linux, the default terminal settings will echo characters as they are typed by the user. When a .NET Core application does a Process.Start of an application on Linux, that application should gets the default echo behavior.

For this reason, the corefx implementation disables echo during ReadLine/ReadKey and enables it after. So when the user is typing while there is no ReadLine/ReadKey, he sees that output echoed (by terminal). And when he then does the ReadLine/ReadKey he sees it once more (by corefx).

I think an alternative to the current implementation (I may be missing some important implementation 'details') would be to make .NET Core application enable echo while there are child processes with RedirectStandardInput false, and disable it otherwise. That way we get close to the Windows behavior of having echo disabled and child processes still get echo by default. @joperezr @wfurt @kasper3 @stephentoub wdyt?

ghost commented 6 years ago

Would it have any impact if we are redirecting to stdin or from stdout to file? I am secretly trying to compare CoreFX with mono's implementation (Console.cs & console-unix.c), which apparently does not set ~ECHO during setup, but does it during a step called tty_teardown.

tmds commented 6 years ago

Would it have any impact if we are redirecting to stdin or from stdout to file?

I don't understand the question. If you are redirecting stdin to a file, it is no longer a terminal.

tmds commented 6 years ago

I am secretly trying to compare CoreFX with mono's implementation (Console.cs & console-unix.c), which apparently does not set ~ECHO during setup, but does it during a step called tty_teardown.

From what I see mono disables the echo when it initializes Console stuff: https://github.com/mono/mono/blob/d2383c037df43eae955add5981cf49fb65176b0d/mcs/class/corlib/System/TermInfoDriver.cs#L204 And it enables it when the application stops (in tty_teardown): https://github.com/mono/mono/blob/83ed33ba2c0b70fc43fbe92404e668126aa6b0d0/mono/metadata/console-unix.c#L214 So child processes won't echo on mono.

ghost commented 6 years ago

I don't understand the question. If you are redirecting stdin to a file, it is no longer a terminal.

Right, I was just thinking (loudly) what could break. 😄

tmds commented 6 years ago

The mono implementation has the downside that interactive children won't echo. It has the upside it is very simple to implement. Maybe we should consider this as an option too?

stephentoub commented 6 years ago

The mono implementation has the downside that interactive children won't echo. It has the upside it is very simple to implement. Maybe we should consider this as an option too?

That's effectively what we originally did. We changed it due to complaints.

danmoseley commented 6 years ago

@joperezr @stephentoub

one way to fix this would probably be to try re-implementing console to use curses library since they already handle this case for you

Do we know how they handle it? Could we do something similar, without moving entirely to curses? i am looking for something limited enough to potentially be suitable for servicing.

tmds commented 6 years ago

I wonder if curses actually supports what we want. The challenge is we want to support starting Processes that rely on ECHO being enabled, without knowing if/when that is needed.

Imagine we'd have a ProcessStartInfo.EnableEcho property. Then we want to echo as long as there are running child processes which have that set to true.

tmds commented 6 years ago

Something we could do is toggle echo on/off based on RedirectStandardOutput:

Any thoughts on this suggestion?

tmds commented 5 years ago

This is fixed by https://github.com/dotnet/corefx/pull/35621, which is included in .NET Core 3.0.

stephentoub commented 5 years ago

Thanks, Tom.