Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Multiple System.Console fixes #6488

Merged
merged 3 commits into from
Mar 2, 2016
Merged

Conversation

stephentoub
Copy link
Member

  • Lifetime of terminal attribute settings. Previously we were setting up the terminal's attributes (e.g. disabling echo) the first time the console was used, and leaving those settings for the duration of the process. This has the very bad effect of leaving such settings on the terminal while other code using that terminal runs. For example, if you launch a process with Process.Start and that process expects input, echo will still be disabled, and you won't be able to see the input. This commit changes how we do these attributes. Instead of setting them and leaving them, we only set them while a read operation is in progress, reverting the settings immediately after. (This does mean that typing prior to a call to Console.ReadKey(intecept: true) will yield visible text, an unfortunate tradeoff, but it seems to be the better of the options.)
  • Reapplication of terminal settings upon process resumption. If the program was suspended with ctrl-z, when it was later resumed, both terminal settings and the application mode settings previously applied were lost. With this commit, we now properly restore the application mode (which impacts how some keys, such as the arrows, are interpreted). And if a read was in progress, we also restore the terminal settings.
  • EOL. Typing ctrl-D is supposed to signal an end-of-line. Previously we weren't doing anything special for it. Now we look up the EOL characters in the terminal settings and act on them appropriately.
  • Console.Read. This method, like ReadLine, is supposed to block until Enter, but it was only blocking until the first byte was available. This commit fixes that. It also makes it respond appropriately to EOL by returning -1.
  • Cursor position robustness. The protocol involved in getting the current cursor position involves writing an escape sequence to stdout and then reading a response that's written by the terminal to stdin. Since the user can also be providing stdin input concurrently, this can lead to some flakiness, and with the current implementation, if we were to miss some of the input that comes from the terminal, we could end up hanging, waiting indefinitely for that input to arrive. This commit adds a timeout so we'll only wait for a second on each read before giving up.

cc: @pallavit, @ellismg, @andschwa, @janvorli
Fixes #5902
Relies on dotnet/coreclr#3409

@andschwa, I'd appreciate it if you could run this through a variety of your scenarios and verify it does what you hope.

- Lifetime of terminal attribute settings. Previously we were setting up the terminal's attributes (e.g. disabling echo) the first time the console was used, and leaving those settings for the duration of the process.  This has the very bad effect of leaving such settings on the terminal while other code using that terminal runs.  For example, if you launch a process with Process.Start and that process expects input, echo will still be disabled, and you won't be able to see the input.  This commit significantly changes how we do these attributes.  Instead of setting them and leaving them, we only set them while a read operation is in progress, reverting the settings immediately after.

- Reapplication of terminal settings upon process resumption.  If the program was suspended with ctrl-z, when it was later resumed, both terminal settings and the application mode settings previously applied were lost.  With this commit, we now properly restore the application mode (which impacts how some keys, such as the arrows, are interpreted).  And if a read was in progress, we also restore the terminal settings.

- EOL.  Typing ctrl-D is supposed to signal an end-of-line.  Previously we weren't doing anything special for it.  Now we look up the EOL characters in the terminal settings and act on them appropriately.

- Console.Read.  This method, like ReadLine, is supposed to block until Enter, but it was only blocking until the first byte was available.  This commit fixes that.  It also makes it respond appropriately to EOL, return -1.

- Cursor position robustness.  The protocol involved in getting the current cursor position involves writing an escape sequence to stdout and then reading a response that's written by the terminal to stdin.  Since the user can also be providing stdin input concurrently, this can lead to some flakiness, and with the current implementation, if we were to miss some of the input that comes from the terminal, we could end up hanging, waiting indefinitely for that input to arrive.  This commit adds a timeout so we'll only wait for a second on each read before giving up.
@andyleejordan
Copy link
Member

@stephentoub and testing! I'll run it through its paces today.

@stephentoub
Copy link
Member Author

and testing! I'll run it through its paces today.

Thanks.

@andyleejordan
Copy link
Member

I've got a weird one. When running under straight PuTTY, with TERM=putty-256color (and the corresponding term file existing at /usr/share/terminfo/p/putty-256color), the program crashes with a segmentation fault.

(gdb) r
Starting program: /home/andrew/src/secret/bin/program
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
[New Thread 0x7ffff4c53700 (LWP 50741)]
[New Thread 0x7ffff4452700 (LWP 50742)]
[New Thread 0x7ffff3c51700 (LWP 50743)]
[New Thread 0x7ffff3450700 (LWP 50744)]
[New Thread 0x7ffff2c4f700 (LWP 50745)]
[New Thread 0x7ffff2429700 (LWP 50746)]
[New Thread 0x7fffeea58700 (LWP 50747)]
[New Thread 0x7fff57ffd700 (LWP 50748)]
[New Thread 0x7fff577fc700 (LWP 50749)]
[New Thread 0x7fff56ffb700 (LWP 50750)]
[New Thread 0x7fff567fa700 (LWP 50751)]
[New Thread 0x7fff55ff9700 (LWP 50752)]
[New Thread 0x7fff557f8700 (LWP 50753)]
[New Thread 0x7fff54ff7700 (LWP 50754)]
[New Thread 0x7fff37fff700 (LWP 50755)]
[New Thread 0x7fff377fe700 (LWP 50756)]
[New Thread 0x7fff36ffd700 (LWP 50757)]
[Thread 0x7fff377fe700 (LWP 50756) exited]
[New Thread 0x7fff377fe700 (LWP 50758)]
[New Thread 0x7fff367fc700 (LWP 50759)]
[Thread 0x7fff377fe700 (LWP 50758) exited]
[Thread 0x7fff367fc700 (LWP 50759) exited]
[New Thread 0x7fff367fc700 (LWP 50760)]

Program received signal SIGSEGV, Segmentation fault.
strlen () at ../sysdeps/x86_64/strlen.S:106
106     ../sysdeps/x86_64/strlen.S: No such file or directory.

This doesn't repro in a few cases so far: with TERM=xterm (PuTTY in default configuration without colors); TERM=screen-256color (Tmux, under PuTTY, with colors); and without the CLR/FX patches applied (CLR built from master which contains your patch, CoreFX built from master with this PR on top of it).

Now, I might have missed some binaries to update. I replaced my published libraries with:

libcoreclr.so
libcoreclrtraceptprovider.so
libdbgshim.so
libmscordaccore.so
libmscordbi.so
libryujit.so
libsosplugin.so
libsos.so
mscorlib.dll
mscorlib.ni.dll
System.Console.dll
System.Globalization.Native.so
System.IO.Compression.Native.so
System.Native.so
System.Net.Http.Native.so
System.Security.Cryptography.Native.so

I built released versions of the two repos, and normally publish with 23826. This may have nothing to do with the patches, but thought I'd report while I debug.

@stephentoub
Copy link
Member Author

What's the stack trace from the strlen where it's seg faulting?

@andyleejordan
Copy link
Member

Sorry, forgot to paste (I have to carefully edit these 😦)

(gdb) bt
#0  strlen () at ../sysdeps/x86_64/strlen.S:106
#1  0x00007ffff6e5b82e in __GI___strdup (s=0x0) at strdup.c:41
#2  0x00007fffef70431d in SystemNative_SetKeypadXmit ()
   from /home/andrew/src/secret/bin/System.Native.so
#3  0x00007fff7dd2ddef in ?? ()
#4  0x00007fffffffd1b0 in ?? ()
#5  0x000000001066c741 in ?? ()
#6  0x00007ffff6b781f0 in vtable for InlinedCallFrame ()
   from /home/andrew/src/secret/bin/libcoreclr.so
#7  0x00007fffffffd7d8 in ?? ()
#8  0x00007fff7de22778 in ?? ()
#9  0x00007fff7de22778 in ?? ()
#10 0x00007fffffffd1b0 in ?? ()
#11 0x00007fff7dd2ddef in ?? ()
#12 0x00007fffffffd1b0 in ?? ()
#13 0x00007fff7de22778 in ?? ()
#14 0x000000001066c741 in ?? ()
#15 0x0000000000000000 in ?? ()

@andyleejordan
Copy link
Member

Bumped the rest of the packages to 23829 so I don't think it's an incompatibility between slightly different code versions, but I can try to copy the rest of the assemblies over too (there's just a loooot of them), or build FX from the commit 23829 was built at.

@stephentoub
Copy link
Member Author

Silly mistake, easy fix. I'll push a fixed version when I'm back at my computer.

@andyleejordan
Copy link
Member

Yay! I love when it's not me 😄

The other odd thing is that, within Tmux (TERM=screen-256color), my readline prompt always starts with ^[[58;39R, and typing echo yields ^[[58;39Recho[[58;51R. I figure we're doing something incorrect in our usage. I couldn't find any of those in infocmp so I figure they're probably colors. Any tips to debug? I think it's the deal with something being "pressed" (by Tmux) before echo is disabled.

@andyleejordan
Copy link
Member

Fantastic, segmentation fault no longer repros.

@andyleejordan
Copy link
Member

Fixed the cursor position outputs as well. @stephentoub You rock. I think this LGTM. Opening up other processes works just fine.

Some oddities are still there, but are probably separate issues (like the up/down arrows changing to A/B after opening and closing, say, Vim).

@andyleejordan
Copy link
Member

Speifically this, except home and end now work (and opening/closing Bash does not cause it, just Vim so far; can try others). I'll open another issue.

@stephentoub
Copy link
Member Author

Speifically this

Can you try again with the latest in this PR?

@andyleejordan
Copy link
Member

Trying it out now.

@andyleejordan
Copy link
Member

Sorry, got distracted. 933d187 fixed that issue 👍

I don't see anything else that needs fixing right now.

@stephentoub
Copy link
Member Author

fixed that issue

Thanks for verifying.

@stephentoub
Copy link
Member Author

@pallavit, could you review this when you get a chance?

@pallavit
Copy link

pallavit commented Mar 2, 2016

@stephentoub Sorry for the delay. I will take a look at it today.

@andyleejordan
Copy link
Member

FWIW, in our readline loop that does quite a bit between calls to ReadKey (tokenization; colorization; tab expansion; etc.), I was not once able to type fast enough (even keyboard mashing) to get caught by the TTY echo and not ReadKey. So while technically possible, I think the vast majority of use cases will be just fine.

@stephentoub
Copy link
Member Author

Thanks, @andschwa 😄

@pallavit
Copy link

pallavit commented Mar 2, 2016

LGTM

@andyleejordan
Copy link
Member

Mm I should probably test on OS X. Didn't get a chance yesterday. Let me run it through for a final verification.

@andyleejordan
Copy link
Member

Confirmed, everything is working as expected on my MacBook (Mavericks 10.11.3 both inside and outside of Tmux, with TERM emulation of xterm and ansi). Bash to Vim and back, no problem with console.

With debug versions, things ran slow enough that I observed the terminal echoing some characters that weren't caught by ReadKey, but I expected as much (Debug is much slower than Release of course), so I don't think this is an issue.

Thank you so much for your work Stephen.

@stephentoub
Copy link
Member Author

Thanks for checking, @andschwa.

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