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.

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

Console.Unix: reset terminal at exit in less cases to avoid blocking the parent. #42303

Merged
merged 5 commits into from
Oct 7, 2020

Conversation

tmds
Copy link
Member

@tmds tmds commented Sep 16, 2020

When a parent is fetching the Console.Cursor position, it configures the terminal
to not echo, writes an escape sequence to query the position, and then reads the
position from stdin.

Because this doesn't happen atomically a child process can overwrite the terminal
settings to echo before the parent starts reading. This causes the position to
be echoed to the user, and the parent gets stuck waiting for input on stdin.

Currently terminal settings are reset at exit for applications that use the
Console or Process class. This change tracks whether the application has
changes the terminal settings and only then resets the terminal settings.

This doesn't fix the issue, but makes it less likely to occur.

Contributes to #42247.

@stephentoub @eiriktsarpalis ptal.

…the parent.

When a parent is fetching the Console.Cursor position, it configures the terminal
to not echo, writes an escape sequence to query the position, and then reads the
position from stdin.

Because this doesn't happen atomically a child process can overwrite the terminal
settings to echo before the parent starts reading. This causes the position to
be echoed to the user, and the parent gets stuck waiting for input on stdin.

Currently terminal settings are reset at exit for applications that use the
Console or Process class. This change tracks whether the application has
changes the terminal settings and only then resets the terminal settings.

This doesn't fix the issue, but makes it less likely to occur.
@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@tmds
Copy link
Member Author

tmds commented Sep 16, 2020

This doesn't fix the issue, but makes it less likely to occur.

I think we can't fix this fully because it would require the parent to prevent any child from changing the terminal settings while it is retrieving the cursor position.

@danmoseley
Copy link
Member

It sounds like we are stuck with this potential hang. That don’t seem ideal. Is this just an intrinsic flaw in how Unix consoles work? Or our API design?

@tmds
Copy link
Member Author

tmds commented Sep 17, 2020

It sounds like we are stuck with this potential hang. That don’t seem ideal. Is this just an intrinsic flaw in how Unix consoles work? Or our API design?

Because multiple operations are involved, it is not possible to retrieve the cursor position in a thread-safe way. So it's a Unix limitation.

Since this is not a Windows limitations, .NET applications are not written to accommodate for it.

They can by doing this:

  • When launching a child that doesn't require the terminal redirect the streams (in particular: StandardInput). This avoids a .NET child from messing with the terminal.
  • When launching a child that requires the terminal, WaitForExit before further interacting with the Console. This makes clear whether the child or parent process is in control of the terminal.

@tmds
Copy link
Member Author

tmds commented Oct 6, 2020

@stephentoub, @danmosemsft can you merge this one?

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @tmds.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants