Fix debugger step through on Unix platforms #592

SeeminglyScience · 2017-12-30T17:06:19Z

This change resolves an issue where the debugger would appear hung until a key was pressed on Unix platforms.

This issue was caused by a quirk with the Unix implementation for System.Console. The way cursor position is retrieved is by writing the cursor position escape sequence query and reading it from stdin. To ensure those escape sequences don't trigger Console.ReadKey, the internal stdin buffer is locked while ReadKey is running. This means that while ReadKey is running, any attempts to query cursor position will block the thread until the pending ReadKey finishes.

The pipeline appears to hang on a debugger stop because there is a pending ReadKey call in another thread and various methods in both the internal PowerShell engine and in our host implementation obtain the cursor position between command executions to determine prompt location.

The best solution to this problem (credit goes to @rkeithhill) is to create an alternative ReadKey implementation that waits for KeyAvailable before blocking. However, if ReadKey is not currently running then any input is echoed to the console. Aside from being generally annoying, this presents a huge problem while trying to do something like Read-Host -AsSecureString.

To get around this, I stripped out the native code from corefx that disables console input echo, created a separate package that this project can consume, and implemented a WaitForKeyAvailable method that utilizes it.

This fix should be viewed as a temporary solution until either ReadKey acts like this by default or a ReadKeyAsync method is implemented into corefx.

@tylerl0706 I've tested on Windows and Linux, but it still needs to be tested on Mac. Thanks again for your help with building! :)

Resolves PowerShell/vscode-powershell#987
Resolves PowerShell/vscode-powershell#1107
Resolves #554

@rkeithhill

This change resolves an issue where the debugger would appear hung until a key was pressed on Unix platforms. This issue was caused by a quirk with the Unix implementation for System.Console. The way cursor position is retreived is by writing the cursor position escape sequence query and reading it from stdin. To ensure those escape sequences don't trigger `Console.ReadKey`, the internal stdin buffer is locked while `ReadKey` is running. This means that while `ReadKey` is running, any attempts to query cursor position will block the thread until the pending `ReadKey` finishes. The pipeline appears to hang on a debugger stop because there is a pending `ReadKey` call in another thread and various methods in both the internal PowerShell engine and in our host implementation obtain the cursor position between command executions to determine prompt location. The best solution to this problem (credit goes to @rkeithhill) is to create an alternative `ReadKey` implementation that waits for `KeyAvailable` before blocking. However, if `ReadKey` is not currently running then any input is echoed to the console. Aside from being generally annoying, this presents a huge problem while trying to do something like `Read-Host -AsSecureString`. To get around this, I stripped out the native code from corefx that disables console input echo, created a separate package that this project can consume, and implemented a `WaitForKeyAvailable` method that utilizes it. This fix should be viewed as a temporary solution until either `ReadKey` acts like this by default or a `ReadKeyAsync` method is implemented into corefx.

SeeminglyScience · 2017-12-30T18:05:26Z

@tylerl0706 Could you restart the AppVeyor build when you get a minute? I believe that's the test issue that happens intermittently.

TylerLeonhardt · 2017-12-30T18:08:54Z

Awesome! I'll test this on Mac later today. Let me restart that build for you...

TylerLeonhardt · 2017-12-30T18:12:00Z

Should I just go through the steps in each of those issues? Or do you have a specific set of steps you want me try?

SeeminglyScience · 2017-12-30T18:20:17Z

Good question, I would say the important things to try would be

Start debugging a file with break points, make sure you can
- Press F5 to continue to the next breakpoint
- Press F11 to step to the next sequence point
- Never have to hit enter in the integrated console to do any of that
Spam input to see if any input is lost or just feels bad (timeout between checking for a available key may be need to be adjusted)
Run $Host.UI.ReadLineAsSecureString() and ensure plain text characters aren't echoed to the screen

Edit: Also for anyone patiently awaiting this fix, if you want to try it early you can grab the AppVeyor build here.

TylerLeonhardt · 2018-01-01T03:32:13Z

NYE got in the way! I'll take care of this... next year 😄 (tomorrow or Tuesday). Happy New Year!

realonedet · 2018-01-02T18:25:00Z

Can anyone validate if this fix works with function breakpoints ? i'm crashing everytime i set a function breakpoint (for example, when using the sample scripts provided with the powershell plug in like in DebugTest.ps1)

TylerLeonhardt · 2018-01-02T18:55:31Z

@SeeminglyScience

1: Didn't have to hit ENTER a single time, nor did it crash 😄 @realonedet, I used the contents of DebugTest.ps1. This PR should help you.

2: I spammed the Integrated Console pretty hard and didn't see any hiccups!

3: 😄

/Users/tylerleonhardt/Desktop > $Host.UI.ReadLineAsSecureString()
********
System.Security.SecureString
/Users/tylerleonhardt/Desktop >

The code is good for macOS 🎉

TylerLeonhardt

LGTM but I think @rkeithhill and @daviwil should take a look at this.

TylerLeonhardt · 2018-01-02T18:59:39Z

PowerShellEditorServices.build.ps1

@@ -129,6 +129,10 @@ task TestPowerShellApi -If { !$script:IsUnix } {
 task Build {
    exec { & $script:dotnetExe build -c $Configuration .\src\PowerShellEditorServices.Host\PowerShellEditorServices.Host.csproj $script:TargetFrameworksParam }
    exec { & $script:dotnetExe build -c $Configuration .\src\PowerShellEditorServices.VSCode\PowerShellEditorServices.VSCode.csproj $script:TargetFrameworksParam }
+    exec { & $script:dotnetExe publish -c $Configuration .\src\PowerShellEditorServices\PowerShellEditorServices.csproj -f netstandard1.6 }


Just curious - why are you are calling dotnet publish? Does a normal build not have what you're looking for?

Yeah, dependencies aren't copied on build when you target standard or core. publish is supposed to the the final product with everything needed to run including packages, runtimes and framework assemblies. Most of that is already provided by being hosted in PowerShell though, so I just pick out what's needed.

realonedet · 2018-01-02T19:50:19Z

(newb question) i'm running version 1.5.1 of the VSCode Powershell plugin, will there be another release of the plugin added ? or if i want to test this i will need to rebuild the plugin myself ?

SeeminglyScience · 2018-01-02T22:36:42Z

@realonedet it would be in the next release. But if you want to try it out early, you can grab the built version from appveyor (here's a direct link). Extra testing before release is very helpful and always welcome :)

rkeithhill · 2018-01-02T22:43:22Z

PowerShellEditorServices.build.ps1

+    exec { & $script:dotnetExe publish -c $Configuration .\src\PowerShellEditorServices\PowerShellEditorServices.csproj -f netstandard1.6 }
+    Copy-Item $PSScriptRoot\src\PowerShellEditorServices\bin\$Configuration\netstandard1.6\publish\UnixConsoleEcho.dll -Destination $PSScriptRoot\src\PowerShellEditorServices.Host\bin\$Configuration\netstandard1.6
+    Copy-Item $PSScriptRoot\src\PowerShellEditorServices\bin\$Configuration\netstandard1.6\publish\runtimes\osx-64\native\libdisablekeyecho.dylib -Destination $PSScriptRoot\src\PowerShellEditorServices.Host\bin\$Configuration\netstandard1.6
+    Copy-Item $PSScriptRoot\src\PowerShellEditorServices\bin\$Configuration\netstandard1.6\publish\runtimes\linux-64\native\libdisablekeyecho.so -Destination $PSScriptRoot\src\PowerShellEditorServices.Host\bin\$Configuration\netstandard1.6


This is not my wheelhouse but are we comfortable that this .so (and the above .dylib) will work on all OS distributions supported by VSCode?

@rkeithhill Good question, I initially only tested on Ubuntu. I just pulled the microsoft/powershell docker images for centos7, opensuse42.1, and opensuse13.2 to make sure they at least can load the lib and turn off echo. But that's not a full test, and I don't know enough about docker and Linux to know if what I did is even an accurate test. I'm pretty comfortable because of how small the code is, but I definitely can't say with absolute certainly that it will work everywhere.

I think you could maybe be a little bit more certain on docker by installing vscode in the image along with some vnc server. Then you should be able to rdp/vnc into the container and mess around with VSCode to verify the fix.

It's possibly a bit lengthly, but if you want to be more certain, it will certainly help.

I'm not actually 100% certain if this is possible though - haven't tried it

@tylerl0706 Yeah for sure, though it sounds like about the same time and effort as spinning up a vm for each distro. Either way that's a bit more involved than I have the bandwidth for, but if someone wants to take that up I'm all for it.

I'll make some Dockerfiles one of these days that can grab the distros, install PowerShell, VNC, VSCode, and the extension so we have them for next time.

rkeithhill · 2018-01-03T01:44:02Z

So the extension supports feature flags via the setting:

"powershell.developer.featureFlags": [],

Those get passed down to PSES. Would it be reasonable to firewall the new code behind a feature flag and tell folks how to enable the feature on Linux/macOS? Not sure how hard that would be with this change. We used to firewall features like this on a cross-platform project I worked on back in the 90's - yeah dating myself a bit here. OTOH the current debugging situation on Linux/macOS is so horribly broken it is hard to imagine this could make it worse. :-)

TylerLeonhardt · 2018-01-03T18:03:31Z

IMO, I'm fine with just shipping it 🚢

rkeithhill

LGTM

TylerLeonhardt · 2018-01-04T02:12:15Z

Should we merge this, or wait for @daviwil? Your call @SeeminglyScience!

TylerLeonhardt · 2018-01-04T19:41:31Z

Just one question to the room, is there an opportunity to add tests for this PR at all?

SeeminglyScience · 2018-01-06T13:28:19Z

@tylerl0706 Let's go ahead and merge. Though, you're right, tests would be good. Not sure exactly when I'll get around to that. We can either keep this open until then, or merge it now and open an issue to track the need for tests. I'd prefer the latter, but I'll leave that up to you.

SeeminglyScience · 2018-01-06T13:30:08Z

Oh also, I'm open to putting this behind a feature flag. If only because it'd be nice to have that capability nailed down before PSReadLine, which will definitely need to be behind a feature flag.

daviwil

@SeeminglyScience is a superhero. Thanks a ton for working so hard to get this fixed!

daviwil · 2018-01-06T17:47:20Z

src/PowerShellEditorServices/Console/ConsoleReadLine.cs

@@ -466,41 +487,6 @@ public Task<string> ReadSimpleLine(CancellationToken cancellationToken)
            return null;
        }

-        private async Task<ConsoleKeyInfo> ReadKeyAsync(CancellationToken cancellationToken)


Really happy to get rid of this horrible piece of code :)

daviwil · 2018-01-06T17:50:36Z

PowerShellEditorServices.build.ps1

@@ -185,9 +189,12 @@ task LayoutModule -After Build {
    New-Item -Force $PSScriptRoot\module\PowerShellEditorServices\bin\Core -Type Directory | Out-Null

    Copy-Item -Force -Path $PSScriptRoot\src\PowerShellEditorServices.Host\bin\$Configuration\netstandard1.6\* -Filter Microsoft.PowerShell.EditorServices*.dll -Destination $PSScriptRoot\module\PowerShellEditorServices\bin\Core\
+    Copy-Item -Force -Path $PSScriptRoot\src\PowerShellEditorServices.Host\bin\$Configuration\netstandard1.6\UnixConsoleEcho.dll -Destination $PSScriptRoot\module\PowerShellEditorServices\bin\Core\


@tylerl0706 before you can ship these binaries with PSES and the PowerShell extension, you'll need to request OSS approval for Patrick's library. You may also need to include his copyright in a Third Party Notices file like the one in the PS extension. I highly doubt they will reject the request to use this code but better to follow the necessary process anyway :)

TylerLeonhardt · 2018-01-06T19:33:41Z

@SeeminglyScience I got approval to use your library 🎉

Let's merge this in and I'll add the Third Party Notices.

realonedet · 2018-01-07T06:26:48Z

@SeeminglyScience I tried the code from the appveyor link (replacing the directories in the zip in the powershell modules folders), the behavior did not change, i'm still crashing as soon as i debug code with breakpoints set (i'm just trying to debug the sample code that comes with the Powershell AddIn). Any ideas ? #StillFrustrated

SeeminglyScience · 2018-01-07T12:21:44Z

@realonedet Hmm that may be a different issue then. Could you open an issue and include verbose logs from right after a crash? (Capturing verbose logs)

Fix PowerShell#591: Sort "editor commands" by their name for menu display

SeeminglyScience requested review from daviwil, TylerLeonhardt and rkeithhill December 30, 2017 17:06

TylerLeonhardt approved these changes Jan 2, 2018

View reviewed changes

rkeithhill reviewed Jan 2, 2018

View reviewed changes

rkeithhill approved these changes Jan 3, 2018

View reviewed changes

This was referenced Jan 3, 2018

PowerShell Integrated Terminal Issue PowerShell/vscode-powershell#1146

Closed

When debugging a powershell script, every 2 times the debugger stops. PowerShell/vscode-powershell#1119

Closed

daviwil approved these changes Jan 6, 2018

View reviewed changes

TylerLeonhardt merged commit 8b12d75 into PowerShell:master Jan 6, 2018

SeeminglyScience mentioned this pull request Jan 6, 2018

Tests needed for ReadKeyAsync changes #604

Closed

SeeminglyScience deleted the unix-stepping-fix branch January 6, 2018 19:46

thezim mentioned this pull request Jan 22, 2018

Resizing window under macOS overrides text PowerShell/PowerShell#5983

Closed

JonZeolla mentioned this pull request Jan 25, 2018

MessageDispatcher error on macOS PowerShell/vscode-powershell#1173

Closed

SeeminglyScience mentioned this pull request Jan 27, 2018

Changes for Editor Hosts (VSCode/Atom) PowerShell/PSReadLine#626

Merged

TylerLeonhardt mentioned this pull request Jan 29, 2018

VS Code does not run PowerShell jobs correctly PowerShell/vscode-powershell#120

Closed

TylerLeonhardt pushed a commit to TylerLeonhardt/PowerShellEditorServices that referenced this pull request Feb 26, 2019

Merge pull request PowerShell#592 from daviwil/sort-extension-commands

635a1bb

Fix PowerShell#591: Sort "editor commands" by their name for menu display

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix debugger step through on Unix platforms #592

Fix debugger step through on Unix platforms #592

SeeminglyScience commented Dec 30, 2017

SeeminglyScience commented Dec 30, 2017

TylerLeonhardt commented Dec 30, 2017

TylerLeonhardt commented Dec 30, 2017

SeeminglyScience commented Dec 30, 2017 •

edited

Loading

TylerLeonhardt commented Jan 1, 2018 •

edited

Loading

realonedet commented Jan 2, 2018 •

edited

Loading

TylerLeonhardt commented Jan 2, 2018

TylerLeonhardt left a comment

TylerLeonhardt Jan 2, 2018

SeeminglyScience Jan 2, 2018

realonedet commented Jan 2, 2018

SeeminglyScience commented Jan 2, 2018

rkeithhill Jan 2, 2018

SeeminglyScience Jan 3, 2018

TylerLeonhardt Jan 3, 2018

SeeminglyScience Jan 3, 2018

TylerLeonhardt Jan 3, 2018

rkeithhill commented Jan 3, 2018

TylerLeonhardt commented Jan 3, 2018

rkeithhill left a comment

TylerLeonhardt commented Jan 4, 2018

TylerLeonhardt commented Jan 4, 2018

SeeminglyScience commented Jan 6, 2018

SeeminglyScience commented Jan 6, 2018

daviwil left a comment

daviwil Jan 6, 2018

daviwil Jan 6, 2018

TylerLeonhardt commented Jan 6, 2018 •

edited

Loading

realonedet commented Jan 7, 2018

SeeminglyScience commented Jan 7, 2018

Fix debugger step through on Unix platforms #592

Fix debugger step through on Unix platforms #592

Conversation

SeeminglyScience commented Dec 30, 2017

SeeminglyScience commented Dec 30, 2017

TylerLeonhardt commented Dec 30, 2017

TylerLeonhardt commented Dec 30, 2017

SeeminglyScience commented Dec 30, 2017 • edited Loading

TylerLeonhardt commented Jan 1, 2018 • edited Loading

realonedet commented Jan 2, 2018 • edited Loading

TylerLeonhardt commented Jan 2, 2018

TylerLeonhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

realonedet commented Jan 2, 2018

SeeminglyScience commented Jan 2, 2018

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

rkeithhill commented Jan 3, 2018

TylerLeonhardt commented Jan 3, 2018

rkeithhill left a comment

Choose a reason for hiding this comment

TylerLeonhardt commented Jan 4, 2018

TylerLeonhardt commented Jan 4, 2018

SeeminglyScience commented Jan 6, 2018

SeeminglyScience commented Jan 6, 2018

daviwil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerLeonhardt commented Jan 6, 2018 • edited Loading

realonedet commented Jan 7, 2018

SeeminglyScience commented Jan 7, 2018

SeeminglyScience commented Dec 30, 2017 •

edited

Loading

TylerLeonhardt commented Jan 1, 2018 •

edited

Loading

realonedet commented Jan 2, 2018 •

edited

Loading

TylerLeonhardt commented Jan 6, 2018 •

edited

Loading