Changes for Editor Hosts (VSCode/Atom) #626

SeeminglyScience · 2018-01-25T23:27:32Z

I've been working on getting PSReadLine working in VSCode/Atom and I came across two things that
I haven't been able to workaround without changes.

Making ReadLine cancellable

For the most part I was able to get away with taking over the pipeline via the OnIdle events
that PSReadLine generates. For some things however, I needed a way to reliably cancel the ReadLine
pipeline. For example, stepping in the debugger or running a script that could trigger a debugger
stop. To accomplish this I added an overload to ReadLine that accepts a cancellation token and
added the wait handle to the ReadKey key request loop.

Workaround for Unix stdin locking

The Unix implementation of System.Console relies mainly on escape sequences. A side effect of
that implementation is you cannot use any of the cursor position API's (Console.CursorLeft, etc)
while Console.ReadKey is running or it'll block the thread until a key is pressed. The main reason
this is an issue is because I run most of the background commands like command completion by taking
over the pipeline when PSReadLine checks for event subscribers. Right before it checks, it saves
the cursor position so it can reposition the prompt if any of the events wrote to the pipeline.

You can actually see this outside of VSCode as well:

$timer = [System.Timers.Timer]::new(100)
$timer.Start()
Register-ObjectEvent -InputObject $timer -EventName Elapsed -Action { Write-Host Testing! }

The event will only fire after a key is pressed. If you attach to the process and pause the debugger
it'll be stopped at this line.

We ran into this issue in the current implementation of our ReadLine in the Editor Services host as well.
Every time the debugger stopped on Unix a key needed to be pressed before the pipeline could
continue because the host would check the cursor position to re-adjust the prompt.

The most obvious fix to this issue is to wait until Console.KeyAvailable is true before calling
Console.ReadKey. Unfortunately in Unix platforms input is echoed to the screen by default. The only
managed way we could find to disable that was to run Console.ReadKey(true). So I stripped out the
native code from corefx that they use to disable input echo and created a separate package specifically
to solve this issue.

cc @daviwil

Allow custom hosts to cancel ReadLine by supplying a cancellation token.

Wait for Console.KeyAvailable before calling Console.ReadKey so getting cursor position doesn't block the thread.

daviwil · 2018-01-25T23:29:57Z

I've been discussing this approach with Patrick for a few months now and I'm fully in support of it. This will enable PSReadline 2.0 to be a first class citizen in Editor Services!

lzybkr · 2018-01-25T23:48:49Z

PSReadLine.build.ps1

@@ -202,6 +202,9 @@ task LayoutModule BuildMainModule, BuildMamlHelp, {

    Copy-Item PSReadLine/bin/$Configuration/Microsoft.PowerShell.PSReadLine2.dll $targetDir
    Copy-Item PSReadLine/bin/$Configuration/System.Runtime.InteropServices.RuntimeInformation.dll $targetDir
+    Copy-Item PSReadLine/bin/$Configuration/UnixConsoleEcho.dll $targetDir
+    Copy-Item PSReadLine/bin/$Configuration/libdisablekeyecho.so $targetDir


Presumably this is an x64 binary? What about x86 or arm32 or arm64?

I see that the code comes from the CLR. Is there any reasonable way to ask the CLR to expose an API instead?

For x86, it looks it isn't supported on Linux and Mac. I do need to look into what I need to do for arm* though.

Is there any reasonable way to ask the CLR to expose an API instead?

That definitely would have been preferred. Here's the thread we asked about calling their library for disabling input echo. That's where they suggested stripping out their implementation. And here's the request to create an async version of ReadKey.

Can you just run stty -echo instead? If you do that once before the call to PSReadLine and once to restore after, it seems not too bad. See https://stackoverflow.com/questions/5633472/how-do-i-turn-off-echo-in-a-terminal.

I'm definitely open to the idea and looking into it. Just to clarify though, when you say the call to PSReadLine do you mean call it from inside the ReadLine method or only call it from editor services? Disabling input echo is needed to implement WaitForKeyAvailable. If it was only disabled by the editor services host, every key press would appear to be rendered twice when in the standard host.

Edit If the escape sequence to break out of ReadKey works then this can be ignored and the package will be removed.

Yeah, I was referring to the method ReadLine. On Windows, we save the console mode at the beginning and restore it at the end, that's similar to what I was proposing for stty.

Any reason why this needs to be supplied as an arch-dependent binary? Why not use P/Invokes like is done for Windows with Get/SetConsoleMode?

@parkovski ...well I wish I had a good explanation for why I originally decided it couldn't be done with pure P/Invokes. But you're 100% correct, it definitely can. I just wrote up something basic to test and it worked great.

Only needed to import tcsetattr and tcgetattr from libc. They should be available everywhere, yeah? Thank you, can't believe I over looked that. I'll get started on switching to P/Invokes.

lzybkr · 2018-01-25T23:51:46Z

PSReadLine/ConsoleLib.cs

+            {
+                while (!Console.KeyAvailable)
+                {
+                    System.Threading.Thread.Sleep(50);


I don't know about Linux, but on Windows, a sleep for less than 200ms or so (I forget exact details) prevents the CPU from going into low power mode because the kernel assumes the process is busy.

So I'm a bit concerned this might cause increased drain of the battery.

Is there some other way where we can block in the kernel instead of a busy loop?

I'm not having luck finding a way to reliably block until a key is available. But a possible solution would be to set the sleep to 200ms until the first key is pressed, then switch to SpinWait.SpinUntil with a timeout of a couple seconds. Then switch back to the 200ms sleep if the timeout is hit. Do you have any concerns with that approach?

That also sounds less than ideal.

If you want to unblock - maybe we could agree on an escape sequence that you generate and PSReadLine ignores, other than to break out of the Console.ReadKey call of course.

Hmm, that's an interesting thought. Early on when I was first looking into the issue for editor services I was experimenting with emitting the terminal information query escape sequence to break out of ReadKey. That worked but wasn't reliable because of different possible return sequences. If it's always the same and always ignored, that may work perfectly. I'll do some testing.

I think we'd need an invalid escape sequence so there's 0 chance the tty will interpret it. Alternatively, maybe we should use an invalid Unicode character.

It's not public, but there is already an interface to mock Console.ReadKey for the tests. The tests just change the mock via reflection, so you could do the same as an experiment.

That said, stdin feels like it is that "delegate". Ensuring stdin works for nearly everything helps in scenarios beyond PSES. (I think a few bindings are impossible on Windows with VT escape sequences but are possible with Console.ReadKey.)

I found a way to block until input is received. I can p/invoke poll(2), also from libc. That has an option to block indefinitely until input is received. It works, but it also triggers on Console.CursorLeft which spills some of the escape sequence into Console.ReadKey. Granted this is the exact problem corefx was trying to solve by locking.

If I can figure out a good way to filter that out, coupled with replacing the native libs with p/invokes, that should hopefully solve all issues.

The problem with filtering that out is whoever called CursorLeft will get an invalid result, which can make line editing glitchy, unless you can put it back in the input stream before anything else is read. It might work to make a new tty and pass the original input through a filter, but to make that reliable I think you'd have to predict the terminal's responses, which seems like not a good idea. I'm leaning toward the only way to solve this is by patching CoreCLR, unfortunately.

@parkovski Yeah I'm operating under the assumption I can find a way to determine if poll was triggered because of CursorLeft without actually consuming the input. If I can't, then I agree it's isn't feasible.

Unfortunately I ran into more issues. Changing the terminal settings via p/invoke isn't working as expected. I think there is an additional setting or two that is getting changed because new lines aren't triggering input unless CTRL is held.

On top of that, it looks like the termios stuct can be very different between architectures. Some have more members, some flags have different values, and the control characters array can vary in size. For some of those, getting them wrong results in a pretty nasty crash. That said, my experience with p/invoke (and Linux in general) is pretty limited, so here's the code I was testing with. It's very possible I missed something, but I don't think p/invoke will be reliable in this instance.

For Editor Services I'm going to move forward with the native binaries (or stty -echo) and the long/short busy loop because the Unix debugging experience is completely broken without it. But PSReadLine in VSCode may need be Windows only until the issue is fixed in corefx.

@lzybkr How would you like to proceed? Assuming you don't have any issues with the first commit, should I remove the second or open a new PR?

lzybkr

I have some comments and questions on the first commit.

I'm rejecting the second commit for now.

As for the approach, I mentioned this before, but I'm reluctant to add a new public api if it makes sense to switch to using standard input with a handshake character.

What about making it internal and "unsupported"? I think you might need reflection anyway to see if the api is available, so it shouldn't matter if it was internal. This way, it's a "private" contract between PSES and PSReadLine, subject to change, but not an officially supported api.

lzybkr · 2018-02-01T00:54:52Z

PSReadLine/ReadLine.cs

+        }
+
+        /// <summary>
+        /// Entry point - called from the PowerShell function PSConsoleHostReadLine


You haven't changed the psm1, so this comment is not correct.

I'd like an explanation of how this api gets called. It would seem that Microsoft.PowerShell.ConsoleHost is not being used.

I updated the comment with reflect intended usage. I kept the comment generic because it can be used by more than just the PSES host, but that's right we aren't using ConsoleHost. The way ReadLine is being called is more or less the same as in ConsoleHost, just a custom script instead of the PSConsoleHostReadLine function.

lzybkr · 2018-02-01T00:56:40Z

PSReadLine/ReadLine.cs

@@ -304,6 +331,8 @@ public static string ReadLine(Runspace runspace, EngineIntrinsics engineIntrinsi
                        _singleton.Initialize(runspace, engineIntrinsics);
                    }

+                    _singleton._cancelReadCancellationToken = cancellationToken;
+                    _singleton._requestKeyWaitHandles[2] = _singleton._cancelReadCancellationToken.WaitHandle;
                    return _singleton.InputLoop();
                }
                catch (OperationCanceledException)


The comment below (GitHub won't let me comment on the exact line) is no longer correct with this change - OperationCanceledException can now mean something other than the console is exiting.

The comment has been updated to reflect the new reason for the exception.

lzybkr · 2018-02-01T01:01:50Z

PSReadLine/ReadLine.cs

                ReadOneOrMoreKeys();
+                if (localCancellationToken.IsCancellationRequested)


If cancellation is requested, what should happen with buffered keys (keys pressed after cancellation, but before the next call to PSReadLIne)?

And what should happen to a partially typed command line (before the cancellation, but no Enter) - should that be restored. If you look at how AcceptAndGetNext is implemented, you can see it's not hard to restore.

If cancellation is requested it'll continue to the next loop and any buffered keys will remain in
_queuedKeys until the next key is requested.

Originally any partially typed command lines were just being discarded. Now the
command line is saved and restored when ReadLine is called again. Thanks for pointing out AcceptAndGetNext, I didn't know it would be that easy to implement and it feels much better now.

This reverts commit 537f5f2.

- Input from cancelled lines are now restored the next time ReadLine is called - Updated comments to reflect changes

SeeminglyScience · 2018-02-02T04:32:43Z

What about making it internal and "unsupported"? I think you might need reflection anyway to see if the api is available, so it shouldn't matter if it was internal. This way, it's a "private" contract between PSES and PSReadLine, subject to change, but not an officially supported api.

I have absolutely no issues with that. @tylerl0706 @daviwil @rkeithhill feel free to chime in if you disagree, but I don't have any objections.

TylerLeonhardt · 2018-02-02T17:06:32Z

Abstain. I don't know enough to feel strongly either way. 🙂 I trust your judgement.

rkeithhill · 2018-02-03T23:45:19Z

No objections from me.

Add the fields _readKeyOverride and _readKeyMethod to allow the ReadKey implementation used to be overridden.

SeeminglyScience · 2018-06-02T17:49:53Z

@lzybkr I know I've been absent too long and I appreciate your patience with me on this.

I've added the delegate we discussed. I'm able to override it from PSES without issue. There's still some bugs to work out with our ReadKey implementation but I've tested that the delegate we inject is the one that runs. So the rest needs to be worked out on our side.

I've also added, in a separate commit, a method to force event handling to happen immediately instead of after the wait timeout in PSConsoleReadLine.ReadKey. I added this because I'm using the checking for events to take over the pipeline without cancelling PSReadLine. The most common thing that is invoked this way is intellisense, which felt very sluggish if the 300ms timeout had just started.

I added this as another private member that we would access via reflection, as I'm not sure it makes sense as a public API.

lzybkr · 2018-06-04T05:05:14Z

PSReadLine/ReadLine.cs

                    handleId = WaitHandle.WaitAny(_singleton._requestKeyWaitHandles, 300);
-                    if (handleId != WaitHandle.WaitTimeout)
+                    if (handleId != WaitHandle.WaitTimeout && handleId != 3)


Can you add constants for each handleId and use those - it will make reading the code easier in the future.

Great idea, that's a lot easier to read now.

I assumed you'd prefer I omit the constant for _keyReadWaitHandle as it's index isn't referenced. Let me know if that's incorrect though.

lzybkr · 2018-06-04T05:07:23Z

After replacing the integer constants with named constants, I think this is ready to merge.

Add constants for each index in _requestKeyWaitHandles and use them instead of the value directly.

SeeminglyScience added 2 commits January 24, 2018 21:17

Add a cancellation token to ReadLine

277e831

Allow custom hosts to cancel ReadLine by supplying a cancellation token.

Fix stdin locking on Unix platforms

537f5f2

Wait for Console.KeyAvailable before calling Console.ReadKey so getting cursor position doesn't block the thread.

lzybkr reviewed Jan 25, 2018

View reviewed changes

lzybkr suggested changes Feb 1, 2018

View reviewed changes

SeeminglyScience added 2 commits February 1, 2018 17:12

Revert "Fix stdin locking on Unix platforms"

7b9a929

This reverts commit 537f5f2.

Addressing feed back

440246a

- Input from cancelled lines are now restored the next time ReadLine is called - Updated comments to reflect changes

SeeminglyScience mentioned this pull request Apr 2, 2018

Garbage characters get inserted into the input line while pressing left right arrow keys #658

Closed

SeeminglyScience added 3 commits June 2, 2018 12:39

Change ReadKey method use a overrideable delegate

f402081

Add the fields _readKeyOverride and _readKeyMethod to allow the ReadKey implementation used to be overridden.

Add comment explaining ReadKey private contract

31853df

Add a method to force immediate event handling

bf930bf

This was referenced Jun 2, 2018

PSReadLine integration (WIP) PowerShell/PowerShellEditorServices#671

Closed

PSReadLine integration PowerShell/PowerShellEditorServices#672

Merged

lzybkr reviewed Jun 4, 2018

View reviewed changes

Add constants for request key wait handle indexes

61e0df0

Add constants for each index in _requestKeyWaitHandles and use them instead of the value directly.

lzybkr merged commit 46960b1 into PowerShell:master Jun 5, 2018

cspotcode mentioned this pull request Aug 28, 2018

PSReadline cannot keep up with keyboard input; leaves raw escape codes on screen #753

Closed

SeeminglyScience mentioned this pull request Aug 9, 2019

There's no reliable way to read and suppress terminal output PowerShell/PowerShell#10327

Closed

SeeminglyScience mentioned this pull request Oct 9, 2019

OnIdle Doesn't Seem to Fire Until PowerShell is Not Idle #1092

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes for Editor Hosts (VSCode/Atom) #626

Changes for Editor Hosts (VSCode/Atom) #626

SeeminglyScience commented Jan 25, 2018

daviwil commented Jan 25, 2018

lzybkr Jan 25, 2018

SeeminglyScience Jan 26, 2018

lzybkr Jan 26, 2018

SeeminglyScience Jan 26, 2018 •

edited

Loading

lzybkr Jan 26, 2018

parkovski Jan 27, 2018

SeeminglyScience Jan 29, 2018

lzybkr Jan 25, 2018

SeeminglyScience Jan 26, 2018

lzybkr Jan 26, 2018

SeeminglyScience Jan 26, 2018

lzybkr Jan 26, 2018

lzybkr Jan 29, 2018

SeeminglyScience Jan 30, 2018

parkovski Jan 30, 2018

SeeminglyScience Jan 30, 2018

SeeminglyScience Jan 31, 2018

lzybkr left a comment

lzybkr Feb 1, 2018

SeeminglyScience Feb 2, 2018

lzybkr Feb 1, 2018

SeeminglyScience Feb 2, 2018

lzybkr Feb 1, 2018

SeeminglyScience Feb 2, 2018

SeeminglyScience commented Feb 2, 2018

TylerLeonhardt commented Feb 2, 2018

rkeithhill commented Feb 3, 2018

SeeminglyScience commented Jun 2, 2018

lzybkr Jun 4, 2018

SeeminglyScience Jun 4, 2018

lzybkr commented Jun 4, 2018

		ReadOneOrMoreKeys();
		if (localCancellationToken.IsCancellationRequested)

Changes for Editor Hosts (VSCode/Atom) #626

Changes for Editor Hosts (VSCode/Atom) #626

Conversation

SeeminglyScience commented Jan 25, 2018

Making ReadLine cancellable

Workaround for Unix stdin locking

daviwil commented Jan 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeeminglyScience Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzybkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeeminglyScience commented Feb 2, 2018

TylerLeonhardt commented Feb 2, 2018

rkeithhill commented Feb 3, 2018

SeeminglyScience commented Jun 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzybkr commented Jun 4, 2018

SeeminglyScience Jan 26, 2018 •

edited

Loading