Console: toggle terminal echo based on presence of interactive child processes

[tmds]

.NET applications echo characters during a Console.Read. By default,
Unix terminals echo characters as the user is typing them.

When a .NET Core application launches an interactive application (e.g. 'vi')
that application expects to find the terminal in an echoing state.

To make both work, corefx was disabling echo during Console.Read operations,
and turning it back on when the read is done.

This changes to echoing while there are interactive applications and not echoing
when there are none.

This means we no longer need to toggle echo off/on for each Console.Read.
And the terminal will no longer echo when there is no Console.Read going on.

[tmds]

No need to review this one yet, I'll make some more changes next week.

[stephentoub]

Thanks, @tmds.

[tmds]

This is ready for review.

[tmds]

@wtgodbe @stephentoub @danmosemsft can you please do a review of this PR.

I'm going to test some scenarios on my machine looking for things that break.

[tmds]

Scenario 1

Console.ReadKey();

pre-PR syscalls:

[pid  5707] ioctl(0, TCGETS, {B38400 opost isig icanon echo ...}) = 0
[pid  5707] ioctl(0, SNDCTL_TMR_START or TCSETS, {B38400 opost isig -icanon -echo ...}) = 0
[pid  5707] ioctl(0, TCGETS, {B38400 opost isig -icanon -echo ...}) = 0
[pid  5707] read(0, "t", 1024)          = 1
[pid  5707] ioctl(0, SNDCTL_TMR_START or TCSETS, {B38400 opost isig icanon echo ...}) = 0
[pid  5707] ioctl(0, TCGETS, {B38400 opost isig icanon echo ...}) = 0
[pid  5707] write(21, "t", 1t)           = 1

post-PR syscalls:

[pid  5923] read(0, "t", 1024)          = 1
[pid  5923] write(24, "t", 1t)           = 1

Scenario 2

            Console.ReadLine();
            Process.Start("nano").WaitForExit();
            Console.ReadLine();

For the user, this works the same pre and post PR. The echoing is not enabled during the read. Echoing is enabled while nano is running. Echoing is disabled again when nano terminated.

Scenario 3

            Console.ReadLine();
            Process.Start(new ProcessStartInfo { FileName = "sleep", Arguments = "10", RedirectStandardInput = true}).WaitForExit();
            System.Console.WriteLine("sleep terminated");
            Console.ReadLine();

Pre PR, the terminal echoed characters during the sleep process, and printed those characters again for the Console.ReadLine. Post PR, the terminal no longer echos during the sleep process and characters are printed on the Console.ReadLine.

Scenario 4

Process.Start("sleep", "0").WaitForExit();

This is started in background. The dotnet process finishes successfully (i.e. it is not stopped on SIGTTOU). This relies on the blockIfBackground: false implementation. Without this, the dotnet process would be stopped when we reconfigure the terminal from a background process.

[wtgodbe]

CC @bradygaster, might be worth testing your scenarios with this change to see if things improve

[wtgodbe]

Looks good overall, just a few places where some additional comments wouldn't hurt. Thanks!

[tmds]

Thanks for reviewing @wtgodbe , I've added some additional comments.

[stephentoub]

Thanks for working on this.

[tmds]

@stephentoub I addressed the PR feedback. I left open the more important comments for you to resolve or provide additional feedback.

[stephentoub]

Thanks, @tmds. I'll review again in the morning.

[stephentoub]

@wtgodbe, to get some extra eyes on this, could you try out writing a variety of apps that use console and process to do various things, and see if you see any oddities? Maybe try running various console-based tools, running dotnet in various ways, searching around for interesting examples of folks using console and trying them out?

[wtgodbe]

Sure, I can try out a few things later this week.

[stephentoub]

Thanks, @wtgodbe.

[stephentoub]

I'm going to test some scenarios on my machine looking for things that break.

Thanks for working through all those scenarios. What about other cases like:

• Launch multiple processes (and don't wait for them)
• Concurrently with them running, interact with the terminal
• Optionally have them interact with the terminal
  That all works "correctly"?

Or similarly:

• Launch two processes and wait for them both
• Have those processes interact with the terminal
  ?

[tmds]

The assumption made to minimize re-configuring terminal is:

Only one process is using the terminal at the same time interactively (that is: echo what gets typed to the screen).

A bit unfortunately for this is that by default, ProcessStartInfo is giving access to stdin/stdout to a child. So looking at those, a lot of processes have access to the terminal even if they don't plan to use it.
(I created https://github.com/dotnet/corefx/issues/35685 to have an easier way to disconnect child processes from the terminal).

When we see a Console.Read operation (on-going, or started when there are child processes that may use the terminal) we decide it is the .NET application (and not a child) that is using the terminal.

This scheme fails when the assumption is violated, which is: there is an interactive process (e.g. vi) and the application is calling Console.Read. In that case, the terminal will be configured to echo for the Read operation. During the Read the terminal will echo appropriate for the .NET application, when the Read finishes, the terminal isn't configured for the interactive application: we're not sure there is an interactive application and we assume our application will continue to interact with the terminal and that we don't want the terminal to echo.
(If the default for ProcessStartInfo were different, we might have reconfigured for the child at that point, because we'd be more confident the child is an interactive application).

[tmds]

So in practice, this means that:

• The application is responsible for not calling Read while it is executing interactive applications. Otherwise the child may not work properly because it expects the terminal to echo. (note: .NET Core children don't expect this).
• The application should configure ProcessStartInfo to indicate a child won't use the terminal. This is to avoid the unintentional echoing.

[stephentoub]

Thanks, @tmds. Let's get this in and then hopefully we'll get enough validation of it to feel confident in the approach. Thanks for working on it.

[tmds]

Getting some real-life usage will be the best validation this is working as intended. Thank you for thorough review @stephentoub !