This repository was archived by the owner on Jan 23, 2023. It is now read-only.
[release/3.0] Reconfigure terminal for Console usage (#40563)#40661
Merged
stephentoub merged 1 commit intodotnet:release/3.0from Aug 29, 2019
Merged
[release/3.0] Reconfigure terminal for Console usage (#40563)#40661stephentoub merged 1 commit intodotnet:release/3.0from
stephentoub merged 1 commit intodotnet:release/3.0from
Conversation
…ut was used by the child (dotnet#40563)
Member
Author
|
This was approved. |
tmds
approved these changes
Aug 29, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Port relevant portion of #40563 to release/3.0
Fixes https://github.com/dotnet/corefx/issues/40557
cc: @tmds, @wtgodbe, @ericstj
Description
In order to handle behaviors for some Console APIs like Console.ReadKey correctly, we need to configure the Unix terminal to e.g. not echo characters as they're typed. But since multiple processes share a terminal, this can lead to problems if the .NET app for which we've configured the terminal then does a Process.Start and the started child (which may or may not be a .NET process) also wants to interact with the terminal. We don't know whether that process will want to interact with the terminal or not; in case it does, we need to reset terminal settings, but if we reset them too aggressively (when the child process doesn't actually need to interact with the terminal but the parent process is going to continue to while the child is alive), we could end with problems e.g. echo'ing characters unexpectedly/incorrectly.
So, we make a best-guess based on the settings with which the child process is created: our heuristic currently says "if the process is not redirecting standard input and it's not redirecting standard output, then it's interactive". But that fails for cases where the process is only redirecting one of them, e.g. a command like "less", where you might redirect its standard input to read from a piped input but where it then just writes to stdout to print the output to the terminal. The fix is to relax the heuristic to be an OR instead of an AND, e.g. "if the process is not redirecting standard input or it's not redirecting standard output, then it's interactive". We also factor in standard error.
Customer Impact
Gobbledegook on and misinterpreted input at the terminal when interacting with System.Console and System.Diagnostics.Process.Start in certain ways.
Regression?
Yes, from .NET Core 2.2. This was reported as a blocking issue for PowerShell upgrading to .NET Core 3.0.
Testing
All automated tests, plus manual validation of this case, plus PowerShell sign-off on the fix.
Risk
The biggest risk here is this area doesn't have a lot of automatic validation. We have tests for basic things, but more complicated interactions between starting processes and interacting with the console currently require manual/exploratory testing. Also, as there's a heuristic involved here, any changes to it could result in other cases that were "working" getting broken.
However, we believe the new heuristic is the "right" one, as it uses all of the information we have available to us to make a more informed choice: if parent starting the child process isn't redirecting everything, then it's effectively saying it may want to interact with the terminal. The risk there is that even if it doesn't redirect, the child process may not interact with the terminal, in which case if the parent process proceeds to interact with the terminal while the child is active, strangeness could result until the child terminates.