-
Notifications
You must be signed in to change notification settings - Fork 214
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
PSReadLine integration #672
PSReadLine integration #672
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a quick review from a code point of view without understanding what it does. Overall it looks like solid and well structured code. The special logic and while loops inside some of the dispose methods makes me feel a bit nervous though.
{ | ||
Logger.Write( | ||
LogLevel.Error, | ||
"Exception occurred while awaiting debug launch task.\n\n" + e.ToString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest using string interpolation and use platform independent newlines:
$"Exception occurred while awaiting debug launch task.{Environment.Newline}{Environment.Newline}{e.ToString()}"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency I'd like to keep this the same for now. Ultimately we shouldn't be making our own strings while logging at all, that would better be handled by Serilog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine, I just wanted to bring it up as some people are not aware of some of the new C# features
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to use Logger.WriteException
here and it will handle the newlines/indentation.
// Create a dummy variable for index 0, should never see this. | ||
this.variables.Add(new VariableDetails("Dummy", null)); | ||
// Create a dummy variable for index 0, should never see this. | ||
this.variables.Add(new VariableDetails("Dummy", null)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed? This looks like a code smell if the code cannot cope with an empty list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. There's still some issues around how the variables/stack traces are built, particularly when doing something like holding down F11
. I added a SemaphoreSlim
to control access which fixed a lot of the race conditions around that code. But I'll open an issue to investigate whether that's needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree, an issue seems to be better to deal with technical debt in this case.
|
||
logger.Write( | ||
LogLevel.Verbose, | ||
string.Format( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
string interpolation $"Getting completions at offset {fileOffset} (line: {cursorPosition.LineNumber}, column: {cursorPosition.ColumnNumber})"
would make it more readable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's hard to tell without hiding whitespace changes but most of this file is unaltered. This is another place where I think it makes sense to wait until we can fix this correctly with Serilog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Theoretically, the Serilog change has already happened. I agree we should use Serilog's format string mechanism, but as is, we just use it internally and preserve the old interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rjmholt right, I had assumed we intended to write a custom ITextFormatter
at some point to avoid building all those verbose strings if verbose logging wasn't enabled. If that wasn't planned I'll see if I can whip something up after this PR merges.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was about to type out a long reply, but I'll open an issue to move the discussion there.
return null; | ||
} | ||
|
||
Stopwatch stopwatch = new Stopwatch(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, but DRY principle:
var stopwatch = new Stopwatch();
Also, you could delay initialization and later just call StopWatch.StartNew()
@@ -0,0 +1,23 @@ | |||
namespace Microsoft.PowerShell.EditorServices.Session |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this new file needs a licensing header (at least that is needed in the main PowerShell repo). Applies to other changes as well
|
||
await SetInvocationRequestAsync( | ||
new InvocationRequest( | ||
pwsh => request.Execute().GetAwaiter().GetResult())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pwsh
would indicate it is only PSCore, would the more generic powershell
be better? Same in other places as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So there's actually a specific reason why I have a habit of using pwsh
, and that's entirely so method chaining lines up nicely.
using (var pwsh = PowerShell.Create())
{
pwsh.AddCommand("Get-ChildItem")
.AddParameter("Path", "C:\\")
.Invoke();
}
Personally I think it ends up being more readable, and powershell
kind of has the opposite problem imo. But obviously that's just a style choice so if others agree I don't have any issues changing it :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, interesting point, I see now where you are coming from.
As an aside/alternative: In the old Windows PowerShell code, people were using pwrsh
quite often (and of course the good old msh
) and some PSSA code (not by me) just uses posh
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think ps
is probably too terse and powershell
is too verbose, and of the remaining options I personally like pwsh
the most for clarity, but can see other arguments too. Just my 2 cents
_executionOptions); | ||
|
||
var unusedTask = Task.Run(() => _resultsTask.SetResult(results)); | ||
// TODO: Deal with errors? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copied from existing code (this was a nested class in PowerShellContext
initially), but yeah that doesn't belong there anymore. Nice catch :)
|
||
public void StopCommandInDebugger(PowerShellContext powerShellContext) | ||
{ | ||
// TODO: Possibly save the pipeline to a field and initiate stop here. Or just throw. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO ?
/// </summary> | ||
internal class PromptNest : IDisposable | ||
{ | ||
private ConcurrentStack<PromptNestFrame> _frameStack; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The style here is inconsistent with other style. Personally I prefer underscores for private variables but other code that was changed did not use underscores...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we don't really have any defined style rules or anything. But for new files, I use the PowerShell repo's style guide, and for existing files I try to maintain the existing style of the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also prefer the underscores. It's hard to justify sweeping through the repo and changing it all at once, but my current policy is that if I touch a file and add/change a private variable, I change them all over. I'm personally in favour of new private variables using an underscore and tolerating the inconsistency as an interim phenomenon.
The Shift+Enter issues are related to PowerShell/PSReadLine#645 I'm guessing? |
I'm going to do a proper review a bit later btw 😄 |
Well... sort of. That was actually the case prior to the change he made to ignore shift in keybinds. My guess is that the console used by vscode acts like the unix consoles in that it also doesn't report shift. |
|
||
namespace Microsoft.PowerShell.EditorServices.Session | ||
{ | ||
using System.Management.Automation; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The style this code base has been using does not put using inside namespaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So when that using statement is outside of the namespace, any calls to the PowerShell API fail to compile with 'PowerShell' is a namespace but is used like a type [PowerShellEditorServices]
Not sure what is causing that, I couldn't find a namespace that ends in PowerShell
other than Microsoft.PowerShell
. And there's no using statement for Microsoft
.
I put that using statement inside the namespace because that's how PowerShellContext is set up. But I agree, I'm not particularly comfortable with that solution. Anyone have any thoughts on how to better handle this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using System.Management.Automation = SMA
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rjmholt that would probably work, but would we prefer having SMA.PowerShell
all over the place over having a using statement or two in the namespace?
I even tried using PowerShell = System.Management.Automation.PowerShell
with no luck. I think this is the lesser evil so to speak.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reckon I'm about 1/3 of the way through my code review. Want to deliver what I've got so far to cut down any latency. Please excuse any terseness on my part -- just want to get the comments out. This is so great though.
I haven't actually pulled and run the change yet, so I'll have even more comments once I've done that I imagine 😄.
Thanks so much for this @SeeminglyScience!
@@ -287,7 +287,8 @@ try { | |||
-BundledModulesPath $BundledModulesPath ` | |||
-EnableConsoleRepl:$EnableConsoleRepl.IsPresent ` | |||
-DebugServiceOnly:$DebugServiceOnly.IsPresent ` | |||
-WaitForDebugger:$WaitForDebugger.IsPresent | |||
-WaitForDebugger:$WaitForDebugger.IsPresent ` | |||
-FeatureFlags:$FeatureFlags |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was under the impression that we only needed the colon for Switch parameters -- can this just be passed like the -BundledModulesPath
parameter above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! That's a mistake :)
|
||
namespace Microsoft.PowerShell.EditorServices.Console | ||
{ | ||
internal static class ConsoleProxy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment to describe what the class is and what it does could be very helpful in the future
static ConsoleProxy() | ||
{ | ||
// Maybe we should just include the RuntimeInformation package for FullCLR? | ||
#if CoreCLR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the style convention is to put #if
s starting in column 0, so they don't get missed. But here I agree with your comment - if the FullCLR defines the same method, I think we're better off removing the #if
. They're more pain than they're worth.
/// in a bitwise combination of ConsoleModifiers values, whether one or more Shift, Alt, | ||
/// or Ctrl modifier keys was pressed simultaneously with the console key. | ||
/// </returns> | ||
internal static ConsoleKeyInfo UnixReadKey(bool intercept, CancellationToken cancellationToken) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Admitably most of that comes from System.Console.ReadKey, so I can't take much credit there 😉
WaitForKeyAvailableAsync = LongWaitForKeyAsync; | ||
} | ||
|
||
internal ConsoleKeyInfo ReadKey(bool intercept, CancellationToken cancellationToken) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand what the code's doing here (all of the waiting and interleaving). Could you add some comments in the body describing what's going on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some comments, they are a complicated bunch unfortunately. Let me know if it's still unclear.
_lock.Release(); | ||
} | ||
|
||
_powerShellContext.ForcePSEventHandling(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does ForcePSEventHandling
do here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So while PSReadLine is running it has full control of the pipeline. While it's waiting for a key it will timeout once every 300 milliseconds and check for PowerShell events to process. When it does that, we use the subscriber to take over the pipeline thread. ForcePSEventHandling
is a "private contract" method in PSRL that skips the 300 millisecond wait and goes straight to event processing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also worth mentioning the reason why being on the pipeline thread is important. When there's no active pipeline you can start PowerShell asynchronously on any thread. But while there's an active pipeline you need to be on the same thread, and you can't call BeginInvoke
. The PowerShell instance must also be marked as nested (created via PowerShell.Create(RunspaceMode.CurrentRunspace)
). Otherwise Invoke
will throw an InvalidOperationException
stating you're on the wrong thread.
|
||
// This should be safe as PSReadline should be waiting for pipeline input due to the | ||
// OnIdle event sent along with it. | ||
typeof(PSEventSubscriber) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't read closely enough to see if we're using this a lot, but if we are, we should probably cache the PropertyInfo
so we don't need to perform the reflection call more than once
@@ -0,0 +1,51 @@ | |||
using System.Threading; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copyright header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
@@ -0,0 +1,192 @@ | |||
using System.Linq; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copyright header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
_consoleReadLine = new ConsoleReadLine(powerShellContext); | ||
_readLineProxy = readLineProxy; | ||
|
||
#if CoreCLR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing about #if
being in column 0
@SeeminglyScience is there a way to build the branch rather than install the module or the VSIX? |
If you have your PowerShellEditorServices repo next to the vscode-powershell repo, you can checkout out the branch for the PR and then build & debug the extension and then the debug extension host will be running with PRSL. Oh, and you have to enable the feature flag in your user settings. This is what I do. |
@rkeithhill Hmmmm, yes I tried that... Will have another couple of goes and will get back to you if it still doesn't work (trying to build for Ubuntu 18.04 right now). |
Oh yeah, from @SeeminglyScience on Slack:
https://ci.appveyor.com/api/buildjobs/cfioquswa7y64qvd/artifacts/bin%2FRelease%2FPSReadLine.zip |
Adding to @rkeithhill's instructions since you are on nix:
In other words
And a word of warning
That will need to be solved before it has any real usability in MacOS/Linux. I'll try to brain dump into an issue about that, in the mean time there's some more details on the Slack if you want to jump into it. |
Yes perfect! (Your warning is the main reason for wanting to build it myself 😄) |
Right, I've got it integrated -- spent a while trying to get it running but after speaking to @tylerl0706 realised it's not an installation issue. Problem is happening here:
|
@rjmholt that means it's loading the wrong module somewhere. If they come out null, then it's loading an old assembly. Do you have any other copies of PSRL in your module path? And the DLL has been replaced in the pwsh install directory? |
Got the following in ordinary terminal:
|
I renamed Microsoft.PowerShell.PSReadLine2.dll to not have the "2" and put it under the publish directory of my PowerShell build |
Hmmmm I think 2.0.0 is installed somewhere else... One sec |
Ok your advice has fixed it @SeeminglyScience! I just overrode the 2.0.0 install in my |
Not sure how I already had a 2.0.0 install on my machine, but anyway... |
A note for those trying this out, make sure you disable automatic extension update, or set the package.json version to something like version 999. I had my "test" version keep getting overwritten on the next vscode restart, and it wasn't immediately intuitive what the problem was. Otherwise, fantastic work @SeeminglyScience, from my cursory use, everything works great both in direct use and in debugging mode. |
I've tried this on 3 systems and found that it works when running as Admin (or as admin through UAC), but when running as a non-admin, not only does PSReadLine not auto-load. If you load it manually, it doesn't work based on Ctrl+Arrow not working, and no syntax highlighting. Suggestions? |
@uttampcu Do you have any other versions of PSReadLine installed? My hunch is that there is a version on your normal Check |
@SeeminglyScience I double checked, and also looked at all 4 startup locations under $profile (none of those scripts even exist). Yet VSCode as admin loads PSReadline, VSCode as user doesn't load PSReadLine. When I ipmo PSReadLine, it shows the same assembly location in both ways. |
readLineProxy = new PSReadLineProxy(psReadLineType); | ||
} | ||
catch (InvalidOperationException) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be a good place to log info -- can imagine issues getting opened because PSRL doesn't work and wanting this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was just thinking about that. I didn't do it originally because I knew it'd be better to tighten up the import so it picks the best module to import. That still needs to happen, but I'm realizing we'll always want the logs anyway.
{ | ||
await this.debuggerStoppedQueue.EnqueueAsync(e); | ||
// We need to ensure this is ran on a different thread than the on it's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ran
-> run
, on
-> one
PowerShell.Runspace = null; | ||
PowerShell.Dispose(); | ||
}, | ||
null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe name the null
argument here if you can
b88a9be
to
86ab115
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I finally got around to reviewing this - mostly nitpicks and questions :)
I'm so excited to finally get this in!
/// An object that describes the ConsoleKey constant and Unicode character, if any, | ||
/// that correspond to the pressed console key. The ConsoleKeyInfo object also describes, | ||
/// in a bitwise combination of ConsoleModifiers values, whether one or more Shift, Alt, | ||
/// or Ctrl modifier keys was pressed simultaneously with the console key. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: keys were pressed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change it, but it's from the docs for Console.ReadKey.
@@ -146,10 +132,30 @@ public async Task<SecureString> ReadSecureLine(CancellationToken cancellationTok | |||
|
|||
private static async Task<ConsoleKeyInfo> ReadKeyAsync(CancellationToken cancellationToken) | |||
{ | |||
return await s_consoleProxy.ReadKeyAsync(cancellationToken); | |||
return await ConsoleProxy.ReadKeyAsync(cancellationToken); | |||
} | |||
|
|||
private async Task<string> ReadLine(bool isCommandLine, CancellationToken cancellationToken) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be called ReadLineAsync?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't change it because it's an existing method, but it's private so shouldn't be a big deal.
/// A task object representing the asynchronus operation. The Result property on | ||
/// the task object returns the user input string. | ||
/// </returns> | ||
internal async Task<string> InvokeLegacyReadLine(bool isCommandLine, CancellationToken cancellationToken) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be called InvokeLegacyReadLineAsync
{ | ||
if (await SpinUntilKeyAvailable(SHORT_READ_TIMEOUT, cancellationToken)) | ||
// Check frequently for a new key to be buffered. | ||
if (SpinUntilKeyAvailable(ShortWaitForKeyTimeout, cancellationToken)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you talk about the difference between the SpinWait.SpinUntil
approach that you do in the ShortWait vs the while (!IsKeyAvailable(cancellationToken))
that you do in the long one?
Why can't they both just be while
s or SpinWait.SpinUntil` with different timeouts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SpinUntil
will constantly run the predicate and the timeout will only happen if the predicate has returned false
for the entire timeout. This is used to allow quick responses to key presses until there has been no input for the timeout length.
The while
loop only has one long delay. This is used to let us wait for input without doing too much processing that we prevent the CPU from entering low power mode (and in general reduce consumption).
Also all of this code is already in PSES, I'm just adding non-async versions.
|
||
VariableDetailsBase variable = variableContainer.Children[name]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if variableContainer becomes null at this point, this will throw an exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's going to throw an exception anyway in the try...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldn't be null, if it is than there's an issue in the caller that needs to be fixed.
internal void PopPromptContext() | ||
{ | ||
PromptNestFrame currentFrame; | ||
lock (_syncObject) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the significance of this _syncObject
and what does locking it do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Locking _syncObject
is done to synchronize access to the frame stack. It prevents race conditions that would occur if two threads tried to pop a context at the same time.
// is in process. | ||
if (isReadLine && !_powerShellContext.IsCurrentRunspaceOutOfProcess()) | ||
{ | ||
GetRunspaceHandleImpl(cancellationToken, isReadLine: false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you need to return here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, when PSRL is running in the main runspace it controls the pipeline thread. So the ReadLine handle and the main thread handle need to be obtained.
// is in process. | ||
if (isReadLine && !_powerShellContext.IsCurrentRunspaceOutOfProcess()) | ||
{ | ||
await GetRunspaceHandleImplAsync(cancellationToken, isReadLine: false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you need to return here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, when PSRL is running in the main runspace it controls the pipeline thread. So the ReadLine handle and the main thread handle need to be obtained.
ReleaseRunspaceHandleImpl(runspaceHandle.IsReadLine); | ||
if (runspaceHandle.IsReadLine && !_powerShellContext.IsCurrentRunspaceOutOfProcess()) | ||
{ | ||
ReleaseRunspaceHandleImpl(isReadLine: false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you call this function twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both the main thread handle and the ReadLine handle need to be released.
await ReleaseRunspaceHandleImplAsync(runspaceHandle.IsReadLine); | ||
if (runspaceHandle.IsReadLine && !_powerShellContext.IsCurrentRunspaceOutOfProcess()) | ||
{ | ||
await ReleaseRunspaceHandleImplAsync(isReadLine: false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you call this function twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both the main thread handle and the ReadLine handle need to be released.
Debug Interactive crashed on me today.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reapproving. There are some outstanding comments, but I think if it's too hard to get those in in a final commit, we can just merge this and open those as issues.
@JustinGrote that crash may be an unrelated instability in the debugger engine |
On Unix like platforms some native applications do not work properly if our event subscriber is active. I suspect this is due to PSReadLine querying cursor position prior to checking for events. I believe the cursor position response emitted is being read as input. I've attempted to fix this by hooking into PSHost.NotifyBeginApplication to temporarly remove the event subscriber, and PSHost.NotifyEndApplication to recreate it afterwards.
Love that PS ReadLine works in the Powershell Terminal. However when working on a .ps1 file, the Terminal automatically switches to the PS Integrated Console where PS ReadLine doesn't work. If I do a side-by-side with a PS terminal, my variables don't come over to the new terminal. |
@uttampcu Those two terminals are different OS processes, totally different PowerShell sessions — there's no way to share fully-instantiated variables across them. It might be possible to remote from one to the other perhaps... Once we this PR is merged (which is soon — we've just been working out a couple of the larger kinks), then PSRL will work in the Integrated Console, delivering exactly the experience you're wanting. |
@tylerl0706, @rkeithhill what remains before we can merge this? I'm thinking unless there's anything particularly pressing, we should try to merge it in and treat any outstanding problems as bugs. |
I haven't seen any issues other than the debug interactive crash, but I
can't definitively say this it is isn't more general. Seems mergeable to me
otherwise except for those null pointer errors in the log I provided
previously not being handled.
|
Yes, I'm waiting on @SeeminglyScience to revert the last commit since it didn't fix the problem. After that, this can go into the 2.0.0 branch |
This reverts commit 1682410.
Ok, testing has passed on my machine. I'm going to fix the testing as well btw, but want to get this merged first. Going to test it a little bit on my machine before I do though |
Ok, only thing I'll mention here is that PSReadLine worked immediately with PS Core but I needed to add a feature flag for Windows PowerShell. That might be a configuration quirk of my VSCode session... But in case it's not I'm documenting it here - especially since the ideal behaviour would be that in PS Core it doesn't use PSReadLine without the feature flag enabled |
Also I tested on my machine and all tests passed. It seems a particular test is hanging -- might be waiting for input on stdio. We can fix that later, it's clearly affecting tests everywhere |
* Add infrastructure for managing context Adds classes that manage the state of the prompt, nested contexts, and multiple ReadLine implementations of varying complexity. (cherry picked from commit 7ca8b9b) * Console related classes changes Change ReadLine method to call out to PowerShellContext. This lets the PowerShellContext determine which ReadLine implementation to use based on available modules. Also includes some changes to the System.Console proxy classes to account for PSReadLine. (cherry picked from commit 59bfa3b) * Rewrite command invocation operations for PSRL Refactor PowerShellContext to have a more robust system for tracking the context in which commands are invoked. This is a significant change in that all interactions with the runspace must be done through methods in PowerShellContext. These changes also greatly increase stability. (cherry picked from commit 21e6b5f) * Rewrite direct SessionStateProxy calls All interactions with the runspace must be done through PowerShellContext now that nested PowerShell instances are encountered frequently. Also fix a bunch of race conditions that were made more obvious with the changes. (cherry picked from commit fa2faba) * Pass feature flags to Start-EditorServicesHost * Address feedback and fix travis build error - Address feedback from @bergmeister - Fix a few other similar mistakes I found - Fix travis build failing due to missing documentation comment tag * Fix all tests except ServiceLoadsProfileOnDemand - Fix an issue where intellisense wouldn't finish if PSReadLine was not running - Fix a crash that would occur if the PSHost was not set up for input like the one used in our tests - Fix a compile error when building against PSv3/4 - Fix a hang that occurred when the PromptNest was disposed during a debug session - Fix some XML documentation comment syntax errors * Fix extra new lines outputted after each command Removed a call to WriteOutput where it wasn't required. This was creating extra new lines which failed tests (and obviously didn't look right). * Remove unused field from InvocationEventQueue And also fix spacing between the other fields. * Remove copying of PDB's in build script @rjmholt did a better job of this in a different PR that we can merge into 2.0.0 later. It also doesn't make sense in this PR. * Add AppVeyor tracking to branch 2.0.0 * Fix ambiguous method crash on CoreCLR Simplify delegate creation in PSReadLineProxy and fix the immediate ambiguous method crash the complicated code caused on CoreCLR. * first round of feedback changes * Some more feedback changes * add a bunch of copyright headers I missed * remove KeyAvailable query * Get the latest PSReadLine module installed * Add PSReadLine installation to build script * the file should be downloaded as a .zip * Address remaining feedback * Attempt to fix issue with native apps and input On Unix like platforms some native applications do not work properly if our event subscriber is active. I suspect this is due to PSReadLine querying cursor position prior to checking for events. I believe the cursor position response emitted is being read as input. I've attempted to fix this by hooking into PSHost.NotifyBeginApplication to temporarly remove the event subscriber, and PSHost.NotifyEndApplication to recreate it afterwards. * Revert "Attempt to fix issue with native apps and input" This reverts commit 1682410. * Fix build failure
* Add infrastructure for managing context Adds classes that manage the state of the prompt, nested contexts, and multiple ReadLine implementations of varying complexity. (cherry picked from commit 7ca8b9b) * Console related classes changes Change ReadLine method to call out to PowerShellContext. This lets the PowerShellContext determine which ReadLine implementation to use based on available modules. Also includes some changes to the System.Console proxy classes to account for PSReadLine. (cherry picked from commit 59bfa3b) * Rewrite command invocation operations for PSRL Refactor PowerShellContext to have a more robust system for tracking the context in which commands are invoked. This is a significant change in that all interactions with the runspace must be done through methods in PowerShellContext. These changes also greatly increase stability. (cherry picked from commit 21e6b5f) * Rewrite direct SessionStateProxy calls All interactions with the runspace must be done through PowerShellContext now that nested PowerShell instances are encountered frequently. Also fix a bunch of race conditions that were made more obvious with the changes. (cherry picked from commit fa2faba) * Pass feature flags to Start-EditorServicesHost * Address feedback and fix travis build error - Address feedback from @bergmeister - Fix a few other similar mistakes I found - Fix travis build failing due to missing documentation comment tag * Fix all tests except ServiceLoadsProfileOnDemand - Fix an issue where intellisense wouldn't finish if PSReadLine was not running - Fix a crash that would occur if the PSHost was not set up for input like the one used in our tests - Fix a compile error when building against PSv3/4 - Fix a hang that occurred when the PromptNest was disposed during a debug session - Fix some XML documentation comment syntax errors * Fix extra new lines outputted after each command Removed a call to WriteOutput where it wasn't required. This was creating extra new lines which failed tests (and obviously didn't look right). * Remove unused field from InvocationEventQueue And also fix spacing between the other fields. * Remove copying of PDB's in build script @rjmholt did a better job of this in a different PR that we can merge into 2.0.0 later. It also doesn't make sense in this PR. * Add AppVeyor tracking to branch 2.0.0 * Fix ambiguous method crash on CoreCLR Simplify delegate creation in PSReadLineProxy and fix the immediate ambiguous method crash the complicated code caused on CoreCLR. * first round of feedback changes * Some more feedback changes * add a bunch of copyright headers I missed * remove KeyAvailable query * Get the latest PSReadLine module installed * Add PSReadLine installation to build script * the file should be downloaded as a .zip * Address remaining feedback * Attempt to fix issue with native apps and input On Unix like platforms some native applications do not work properly if our event subscriber is active. I suspect this is due to PSReadLine querying cursor position prior to checking for events. I believe the cursor position response emitted is being read as input. I've attempted to fix this by hooking into PSHost.NotifyBeginApplication to temporarly remove the event subscriber, and PSHost.NotifyEndApplication to recreate it afterwards. * Revert "Attempt to fix issue with native apps and input" This reverts commit 1682410. * Fix build failure
* Add infrastructure for managing context Adds classes that manage the state of the prompt, nested contexts, and multiple ReadLine implementations of varying complexity. (cherry picked from commit 7ca8b9b) * Console related classes changes Change ReadLine method to call out to PowerShellContext. This lets the PowerShellContext determine which ReadLine implementation to use based on available modules. Also includes some changes to the System.Console proxy classes to account for PSReadLine. (cherry picked from commit 59bfa3b) * Rewrite command invocation operations for PSRL Refactor PowerShellContext to have a more robust system for tracking the context in which commands are invoked. This is a significant change in that all interactions with the runspace must be done through methods in PowerShellContext. These changes also greatly increase stability. (cherry picked from commit 21e6b5f) * Rewrite direct SessionStateProxy calls All interactions with the runspace must be done through PowerShellContext now that nested PowerShell instances are encountered frequently. Also fix a bunch of race conditions that were made more obvious with the changes. (cherry picked from commit fa2faba) * Pass feature flags to Start-EditorServicesHost * Address feedback and fix travis build error - Address feedback from @bergmeister - Fix a few other similar mistakes I found - Fix travis build failing due to missing documentation comment tag * Fix all tests except ServiceLoadsProfileOnDemand - Fix an issue where intellisense wouldn't finish if PSReadLine was not running - Fix a crash that would occur if the PSHost was not set up for input like the one used in our tests - Fix a compile error when building against PSv3/4 - Fix a hang that occurred when the PromptNest was disposed during a debug session - Fix some XML documentation comment syntax errors * Fix extra new lines outputted after each command Removed a call to WriteOutput where it wasn't required. This was creating extra new lines which failed tests (and obviously didn't look right). * Remove unused field from InvocationEventQueue And also fix spacing between the other fields. * Remove copying of PDB's in build script @rjmholt did a better job of this in a different PR that we can merge into 2.0.0 later. It also doesn't make sense in this PR. * Add AppVeyor tracking to branch 2.0.0 * Fix ambiguous method crash on CoreCLR Simplify delegate creation in PSReadLineProxy and fix the immediate ambiguous method crash the complicated code caused on CoreCLR. * first round of feedback changes * Some more feedback changes * add a bunch of copyright headers I missed * remove KeyAvailable query * Get the latest PSReadLine module installed * Add PSReadLine installation to build script * the file should be downloaded as a .zip * Address remaining feedback * Attempt to fix issue with native apps and input On Unix like platforms some native applications do not work properly if our event subscriber is active. I suspect this is due to PSReadLine querying cursor position prior to checking for events. I believe the cursor position response emitted is being read as input. I've attempted to fix this by hooking into PSHost.NotifyBeginApplication to temporarly remove the event subscriber, and PSHost.NotifyEndApplication to recreate it afterwards. * Revert "Attempt to fix issue with native apps and input" This reverts commit 1682410. * Fix build failure
* Add infrastructure for managing context Adds classes that manage the state of the prompt, nested contexts, and multiple ReadLine implementations of varying complexity. (cherry picked from commit 7ca8b9b) * Console related classes changes Change ReadLine method to call out to PowerShellContext. This lets the PowerShellContext determine which ReadLine implementation to use based on available modules. Also includes some changes to the System.Console proxy classes to account for PSReadLine. (cherry picked from commit 59bfa3b) * Rewrite command invocation operations for PSRL Refactor PowerShellContext to have a more robust system for tracking the context in which commands are invoked. This is a significant change in that all interactions with the runspace must be done through methods in PowerShellContext. These changes also greatly increase stability. (cherry picked from commit 21e6b5f) * Rewrite direct SessionStateProxy calls All interactions with the runspace must be done through PowerShellContext now that nested PowerShell instances are encountered frequently. Also fix a bunch of race conditions that were made more obvious with the changes. (cherry picked from commit fa2faba) * Pass feature flags to Start-EditorServicesHost * Address feedback and fix travis build error - Address feedback from @bergmeister - Fix a few other similar mistakes I found - Fix travis build failing due to missing documentation comment tag * Fix all tests except ServiceLoadsProfileOnDemand - Fix an issue where intellisense wouldn't finish if PSReadLine was not running - Fix a crash that would occur if the PSHost was not set up for input like the one used in our tests - Fix a compile error when building against PSv3/4 - Fix a hang that occurred when the PromptNest was disposed during a debug session - Fix some XML documentation comment syntax errors * Fix extra new lines outputted after each command Removed a call to WriteOutput where it wasn't required. This was creating extra new lines which failed tests (and obviously didn't look right). * Remove unused field from InvocationEventQueue And also fix spacing between the other fields. * Remove copying of PDB's in build script @rjmholt did a better job of this in a different PR that we can merge into 2.0.0 later. It also doesn't make sense in this PR. * Add AppVeyor tracking to branch 2.0.0 * Fix ambiguous method crash on CoreCLR Simplify delegate creation in PSReadLineProxy and fix the immediate ambiguous method crash the complicated code caused on CoreCLR. * first round of feedback changes * Some more feedback changes * add a bunch of copyright headers I missed * remove KeyAvailable query * Get the latest PSReadLine module installed * Add PSReadLine installation to build script * the file should be downloaded as a .zip * Address remaining feedback * Attempt to fix issue with native apps and input On Unix like platforms some native applications do not work properly if our event subscriber is active. I suspect this is due to PSReadLine querying cursor position prior to checking for events. I believe the cursor position response emitted is being read as input. I've attempted to fix this by hooking into PSHost.NotifyBeginApplication to temporarly remove the event subscriber, and PSHost.NotifyEndApplication to recreate it afterwards. * Revert "Attempt to fix issue with native apps and input" This reverts commit 1682410. * Fix build failure
* Add infrastructure for managing context Adds classes that manage the state of the prompt, nested contexts, and multiple ReadLine implementations of varying complexity. (cherry picked from commit 7ca8b9b) * Console related classes changes Change ReadLine method to call out to PowerShellContext. This lets the PowerShellContext determine which ReadLine implementation to use based on available modules. Also includes some changes to the System.Console proxy classes to account for PSReadLine. (cherry picked from commit 59bfa3b) * Rewrite command invocation operations for PSRL Refactor PowerShellContext to have a more robust system for tracking the context in which commands are invoked. This is a significant change in that all interactions with the runspace must be done through methods in PowerShellContext. These changes also greatly increase stability. (cherry picked from commit 21e6b5f) * Rewrite direct SessionStateProxy calls All interactions with the runspace must be done through PowerShellContext now that nested PowerShell instances are encountered frequently. Also fix a bunch of race conditions that were made more obvious with the changes. (cherry picked from commit fa2faba) * Pass feature flags to Start-EditorServicesHost * Address feedback and fix travis build error - Address feedback from @bergmeister - Fix a few other similar mistakes I found - Fix travis build failing due to missing documentation comment tag * Fix all tests except ServiceLoadsProfileOnDemand - Fix an issue where intellisense wouldn't finish if PSReadLine was not running - Fix a crash that would occur if the PSHost was not set up for input like the one used in our tests - Fix a compile error when building against PSv3/4 - Fix a hang that occurred when the PromptNest was disposed during a debug session - Fix some XML documentation comment syntax errors * Fix extra new lines outputted after each command Removed a call to WriteOutput where it wasn't required. This was creating extra new lines which failed tests (and obviously didn't look right). * Remove unused field from InvocationEventQueue And also fix spacing between the other fields. * Remove copying of PDB's in build script @rjmholt did a better job of this in a different PR that we can merge into 2.0.0 later. It also doesn't make sense in this PR. * Add AppVeyor tracking to branch 2.0.0 * Fix ambiguous method crash on CoreCLR Simplify delegate creation in PSReadLineProxy and fix the immediate ambiguous method crash the complicated code caused on CoreCLR. * first round of feedback changes * Some more feedback changes * add a bunch of copyright headers I missed * remove KeyAvailable query * Get the latest PSReadLine module installed * Add PSReadLine installation to build script * the file should be downloaded as a .zip * Address remaining feedback * Attempt to fix issue with native apps and input On Unix like platforms some native applications do not work properly if our event subscriber is active. I suspect this is due to PSReadLine querying cursor position prior to checking for events. I believe the cursor position response emitted is being read as input. I've attempted to fix this by hooking into PSHost.NotifyBeginApplication to temporarly remove the event subscriber, and PSHost.NotifyEndApplication to recreate it afterwards. * Revert "Attempt to fix issue with native apps and input" This reverts commit 1682410. * Fix build failure Use latest PSReadLine (PowerShell#745) * Use PSRL release and filter out previous betas * Use fully qualified type name for broad PowerShell support * Fix CI module installation
I messed up the merge conflict resolution in #671, many thanks to the great @markekraus for helping me resolve that ❤️
PSReadLine Integration
This change brings integration of PSReadLine to the PSES integrated console. Here's a short list
of the cool stuff this brings to the console experience
Set-PSReadLineKeyHandler
Additionally, integrating PSReadLine required reworking a large portion of our code around debugging
and the way we manage invoking commands. Part of this rework required finding a lot of the places
the debugger would occasionally hang or crash. This should result in a big boost to stability for
those having issues with the debugger.
Maintainers/Contributers
With this change comes some complexity in the state of the runspace. There are now several different
types of contexts in which a command can be ran.
on the pipeline thread.
Debugger.ProcessCommand
whichmust be invoked on the pipeline thread
Because of this complexity, when at all possible route any interactions with the runspace through
PowerShellContext.ExecuteCommand
. All of the logic for determining what context to invoke thecommand in is handled via that method. If it can't (or shouldn't) be invoked in PowerShell, but
still must be ran on the pipeline thread, you can use
PowerShellContext.InvokeOnPipelineThread
.Along with these changes I also implemented
$Host.EnterNestedPrompt()
. I did this because I alreadyneeded a system to track multiple layers of "context" and what PowerShell instance to give them, what
thread to send them to, etc. So it with that requirement, it wasn't all that much extra work to throw
that in.
Installing to test
PSReadLine
gps powershell | ? id -ne $pid | spps
)2.0.0
directory if required. Should besomething like
"$(Split-Path $profile.CurrentUserAllHosts)/Modules/PSReadLine/2.0.0/<contents-here>"
The Extension
Prebundled (easier)
You can grab the PowerShell-v2-insiders.vsix from AppVeyor here
To install it:
ctrl/cmd+shift+p
)VSIX
and select the install option and navigate to the vsix you downloadedBundle yourself (harder)
gmo PowerShellEditorServices|% ModuleBase
withinthe integrated console.
<yourExtensionsFolder>\ms-vscode.powershell-1.7.0\modules\PowerShellEditorServices
Troubleshooting
If you start VSCode and the integrated console and PSReadLine doesn't load (easy way to tell is no syntax highlighting) then try the following
Clear out any other versions of PSReadLine. The module import logic needs some refining, sometimes
this is especially a problem if you have other versions of the pre-release 2.0 installed.
Ensure run/execute permission is enabled for the DLL's of both modules. In Windows ensure they
are "unblocked" (
Unblock-File
).If running PowerShell Core, you need to replace the
Microsoft.PowerShell.ReadLine.dll
with theMicrosoft.PowerShell.ReadLine2.dll
from the prerelease build. Hopefully that's a bug in mymodule import code and not something that will be required at release.
Known issues
Please note this is not being merged into
master
, but into2.0.0
. This needs a lot more testingand fixes before it can be recommended for use in production environments.
When selecting code in the editor and pressing F8 to send it to the console, the syntax of the script
is not highlighted
Unix platforms are probably a bit buggy at the moment. We have to inject our
ReadKey
implementationinto PSReadLine, and I didn't get that working perfectly. I'll need some help from @tylerl0706 and
@rjmholt with that as I don't have a great linux test environment.
The stream for the debug adapter occasionally closes unexpectedly which makes PSES hang.
Not sure if this is a new bug, or just one of the existing hangs I didn't get to fix yet.
Key bindings are a little different, closer to Unix based terminals. For example, to bind
ctrl+space
you must use the chord
ctrl+@
and to bindctrl+backspace
you need to bindctrl+w
. Mostnotably, it does not appear that there is any way to get
shift+enter
to register. I believethat's an issue with nodepty. This is more VSCode/Atom specific than PSES, but it's worth noting.