PowerShell / PowerShellEditorServices

A common platform for PowerShell development support in any editor or application!
MIT License
632 stars 215 forks source link

Revamp pipeline thread handling #1295

Closed SeeminglyScience closed 2 years ago

SeeminglyScience commented 4 years ago

A lot of the problems we face is based around handling of the pipeline thread. In order to invoke something on it, we need to interact with the PowerShell class, making us invoke at least a small bit of script in most cases. The reason for this is that the actual thread used by PowerShell is internal to the runspace by default. The only way to access it is to invoke a command of some sort.

This is the default experience, but we can change it. Runspace has a property called ThreadOptions. One of the choices for that enum is UseCurrentThread. So what we can do is start our own thread, create the runspace there with that option, and never give up that thread.

One of the biggest wins here would be that we could call PSConsoleReadLine.ReadLine directly without having to invoke something. We could also ditch using the thread pool to wait for PowerShell.Invoke to finish (which probably causes some dead locks). We could get rid of a lot of the more complicated code in PowerShellContext.

I'm pretty confident that if this doesn't outright solve a lot of the sluggishness and dead locks, it'll at the very least be significantly easier to debug.

The rest of this post is taken from #980 since the idea is the same: Nvm, just look at @rjmholt's code and the rest of conversation. The linked post is pretty outdated.

rjmholt commented 4 years ago

I've been thinking about this too, and wrote a simple consumer that allows asynchronous dispatch of PowerShell commands like this:

    public class AsyncPowerShell
    {
        private readonly PowerShell _pwsh;

        private readonly Runspace _runspace;

        private readonly BlockingCollection<Tuple<PSCommand, TaskCompletionSource<IReadOnlyCollection<PSObject>>>> _commandQueue;

        private readonly Thread _pwshThread;

        private readonly CancellationTokenSource _cancellationSource;

        public AsyncPowerShell()
        {
            _commandQueue = new BlockingCollection<Tuple<PSCommand, TaskCompletionSource<IReadOnlyCollection<PSObject>>>>();
            _cancellationSource = new CancellationTokenSource();
            _runspace = RunspaceFactory.CreateRunspace();
            _runspace.Open();
            _pwsh = PowerShell.Create();
            _pwsh.Runspace = _runspace;
            _runspace.Debugger.DebuggerStop += OnDebuggerStop;
            _pwshThread = new Thread(RunPwshConsumer);
            _pwshThread.Start();
        }

        public Task<IReadOnlyCollection<PSObject>> ExecutePowerShellAsync(string command)
        {
            var completionSource = new TaskCompletionSource<IReadOnlyCollection<PSObject>>();
            _commandQueue.Add(new Tuple<PSCommand, TaskCompletionSource<IReadOnlyCollection<PSObject>>>(new PSCommand().AddCommand(command), completionSource));
            return completionSource.Task;
        }

        public void Stop()
        {
            _commandQueue.CompleteAdding();
            _cancellationSource.Cancel();
            _pwshThread.Join();
        }

        private void RunPwshConsumer()
        {
            try
            {
                foreach (Tuple<PSCommand, TaskCompletionSource<IReadOnlyCollection<PSObject>>> executionRequest in _commandQueue.GetConsumingEnumerable(_cancellationSource.Token))
                {
                    try
                    {
                        _pwsh.Commands = executionRequest.Item1;
                        executionRequest.Item2.SetResult(_pwsh.Invoke());
                    }
                    catch (OperationCanceledException)
                    {
                        executionRequest.Item2.SetCanceled();
                    }
                    catch (Exception e)
                    {
                        executionRequest.Item2.SetException(e);
                    }
                    finally
                    {
                        _pwsh.Commands.Clear();
                    }
                }
            }
            catch (OperationCanceledException)
            {
                // End nicely
            }
        }

        private void OnDebuggerStop(object sender, DebuggerStopEventArgs args)
        {
            Console.WriteLine("Debugger stopped");
        }
    }

I think with some added sophistication, we could build off such a model to add:

The only part I don't quite understand in your description @SeeminglyScience is why we need another thread for message dispatch.

@daxian-dbw might also be interested in this discussion

SeeminglyScience commented 4 years ago

The only part I don't quite understand in your description @SeeminglyScience is why we need another thread for message dispatch.

Oops that made sense in the context of the original thread, but OmniSharp already does that. I'll rip that out

SeeminglyScience commented 4 years ago

Love the code btw ❤️. Basically the same thing I was picturing, except include an option to queue a Action<EngineIntrinsics, CancellationToken> that sets Runspace.DefaultRunspace before invocation (maybe needed maybe not). That way we can maybe run ReadLine, CommandInvocationIntrinsics.GetCommand, and/or CommandCompletion.CompleteInput (we might already do this?) without a command processor.

Would also maybe make accessing some internals via reflection a bit safer. I think there have been a few situations where it would have been worth it to use an internal API, but threading concerns made it really risky.

rjmholt commented 4 years ago

The other thought I had was to also include a method to queue a non-PowerShell action, so we could run something on the pipeline thread without having the overhead of executing PowerShell

This is exactly what you're saying...

daxian-dbw commented 4 years ago

One of the choices for that enum is UseCurrentThread.

I think setting the Runspace.DefaultRunspace should be enough to directly calling ReadLine without requiring __Invoke-ReadLineForEditorService. But we will need to make sure the Runspace is not used by anything else when PSReadLine is doing its work.

Setting UseCurrentThread in ThreadOptions only indicates that when invoking a script/command with that PowerShell instance (say ps), the actual invocation happens on the calling thread. So if you later call ps.Invoke() to a different thread, then that thread will be used instead. Also, a gotcha is that PowerShell's BeginInvoke and EndInvoke methods will cause a deadlock when using UseCurrentThread. It's more like a bug to me.

@rjmholt @SeeminglyScience Do you mean the Dispatch pool section in the PR description is not applicable anymore?

SeeminglyScience commented 4 years ago

One of the choices for that enum is UseCurrentThread.

I think setting the Runspace.DefaultRunspace should be enough to directly calling ReadLine without requiring __Invoke-ReadLineForEditorService. But we will need to make sure the Runspace is not used by anything else when PSReadLine is doing its work.

I know there are a few other things that are thread static though. Drawing a blank on them, but iirc one has to do with DSC cache. Also I do think it would easier to manage access to the thread than it is to manage what is using the runspace (that's more or less what we do today).

Setting UseCurrentThread in ThreadOptions only indicates that when invoking a script/command with that PowerShell instance (say ps), the actual invocation happens on the calling thread. So if you later call ps.Invoke() to a different thread, then that thread will be used instead.

Yeah, the idea being is that ps.Invoke() will only be called on the thread we create.

Also, a gotcha is that PowerShell's BeginInvoke and EndInvoke methods will cause a deadlock when using UseCurrentThread. It's more like a bug to me.

That's interesting... I kind of expected that would throw. Similar to the "cannot invoke async on nested pipeline" exception.

@rjmholt @SeeminglyScience Do you mean the Dispatch pool section in the PR description is not applicable anymore?

He was talking about a different part I already removed, but yeah that one kind of isn't either. That's also already mostly handled by omnisharp. I went ahead and just removed all that since @rjmholt's code shows what I'm talking about better than I was explaining it.

rjmholt commented 4 years ago

The parts I'm not quite clear on here are:

daxian-dbw commented 4 years ago

fix other debugger issues like Wait-Debugger in an argument completer?

That sounds like an unexpected use to me. What is the scenario?

I looked into the PSES source code recently, and get the idea of how script execution for intellisense is done via the OnIdle event. ~The logic might not work if PSES starts to call ReadLine directly instead of from a pipeline. Or to be more precise, the OnIdle event might not work with PSReadLine, because the Runspace will be idle for most of the time (PSReadLine spinning in ReadKey), so a PulsePipeline will be automatically started by the Runspace from time to time, but ReadLine doesn't expect that something is running in the pipeline, and may invoke script/command in a nested pipeline (PowerShell.Create(CurrentRunspace)), and result in conflicts and failures.~ Never mind, PipelineBase.DoConcurrentCheck handles the case where a pulse pipeline is running. So this is probably not a problem.

What does the existing ThreadController do in PSES?

rjmholt commented 4 years ago

That sounds like an unexpected use to me. What is the scenario?

@daxian-dbw do you know if PSReadLine handles either of those?

daxian-dbw commented 4 years ago

For PowerShell/vscode-powershell#2448, PSReadLine doesn't handle breaking into debugger or the nested debugger prompt. The debugger prompt is handled by the PowerShell host. With ConsoleHost, it will call into PSConsoleHostReadLine function again from the nested prompt.

For PowerShell/vscode-powershell#2246, the script block is converted to a delegate which calls ScriptBlock.InvokeAsDelegateHelper to execute the script in the DefaultRunspace. BTW, I cannot reproduce the hang.

TylerLeonhardt commented 4 years ago

I like all of this. It sounds like we could really simplify how we manage running (PowerShell or not) on the pipeline thread.

My thinking is that this is something that can be totally standalone and not specific to PSES... That way it can be self-contained and easy to test. (I'm not saying it should be its own assembly - just that it can be in its own folder and is clear what the purpose is for)

We tackled handling async requests with Omnisharp... the next big hurdle is our management of the PowerShell runspace... and this discussion should address the fear and uncertainty I have with the existing code which is great.

rjmholt commented 4 years ago

My thinking is that this is something that can be totally standalone and not specific to PSES... That way it can be self-contained and easy to test. (I'm not saying it should be its own assembly - just that it can be in its own folder and is clear what the purpose is for)

Yeah that's my feeling too.

Because of how PowerShell is set up, this execution model is coupled to the host implementation, which is something I didn't appreciate until reading through the implementation; I believe debugging and nested prompts only work if the host attached to the code implements them (or something along those lines).

Basically, today we do a bunch of complicated stuff with the host implementation and instead my hope is to have the PowerShell executor and host implementation together in its own folder. Possibly for testing a dummy host will be needed.

rjmholt commented 4 years ago

Also thanks for looking at those @daxian-dbw -- have been meaning to ask you how those get handled when run in pwsh.exe. Down the line we might be able to do something similar

SeeminglyScience commented 4 years ago
  • How do we handle when the debugger gets dropped into?

ScriptDebugger.DebuggerStop would give control back to the thread owner.

  • Can we use this design to fix other debugger issues like Wait-Debugger in an argument completer?

Yeah probably. Though this would be way easier to do if we stopped using OnIdle. I don't know if I ever recorded it anywhere, but I regret using that. Instead, PSRL should have a private contract delegate field (or a public API) that it calls on idle if not null. Using PowerShell's eventing is too flimsy and prone to dead locks. Especially when two things try to marshal a call back to the pipeline thread using it, it gets real messy.

Edit: I should say that's all a guess. I haven't actually tried it, but I'd guess it's getting hung up when we try to retake PSRL with eventing while the event manager is already processing our first tab completion request. ConsoleHost doesn't have this problem because it has no reason to retake PSRL, any completion requests are going to come from PSRL itself.

  • How do we programmatically trigger a cancellation in the current PowerShell execution and what do we do if we are unable to cancel it? Do we try to abort it, clean it up and start a new one, or is that going to usually leave things in a bad state anyway?

I say we record the current "task" that's processing. I'm thinking the queue will take an interface like IJob or something that has a void Execute() method and a void Cancel() method. For the Action<> job it would trigger a cancellation token, for commands it would hit the appropriate cancel mechanism (StopProcessCommand/Stop).

If we can't stop, then there's not much we can do. We can't pretend that we cancelled because it'll still be executing on the pipeline thread.

  • Is there a simple way we can traverse the queue and sift out things that have been cancelled ahead of time? Or perhaps it's enough to just evaluate if something's been cancelled before we evaluate it? If so, do we set an exception or return some indicative value?

I think you just check for cancelled at the start of item processing, if it's cancelled move on to the next one.

SeeminglyScience commented 4 years ago
  • How do we programmatically trigger a cancellation in the current PowerShell execution and what do we do if we are unable to cancel it? Do we try to abort it, clean it up and start a new one, or is that going to usually leave things in a bad state anyway?

I say we record the current "task" that's processing. I'm thinking the queue will take an interface like IJob or something that has a void Execute() method and a void Cancel() method. For the Action<> job it would trigger a cancellation token, for commands it would hit the appropriate cancel mechanism (StopProcessCommand/Stop).

If we can't stop, then there's not much we can do. We can't pretend that we cancelled because it'll still be executing on the pipeline thread.

Or if you mean how do we cancel something that may or may not be currently processing (like might be the current item or might be further in the queue) then: We'd probably also want to store a lookup of job ID to job. The job itself would know if it's currently processing or not, if it is it'd call the appropriate cancel method, if not it'd just mark itself as cancelled.

SeeminglyScience commented 4 years ago

Because of how PowerShell is set up, this execution model is coupled to the host implementation, which is something I didn't appreciate until reading through the implementation; I believe debugging and nested prompts only work if the host attached to the code implements them (or something along those lines).

That's right. If no one subscribes to Debugger.DebuggerStop, nothing happens. You can see that with DefaultHost on a background runspace. Same with nested prompts, if the host doesn't implement PSHost.EnterNestedPrompt() it just throws.

SeeminglyScience commented 4 years ago

What does the existing ThreadController do in PSES?

It's kind of like what this issue is discussing, but only for nested prompts/debug stops. The current execution flow is sort of like this:

  1. Is something running?
    1. No - queue PowerShell.Invoke on the thread pool
    2. Yes - Is ReadLine what is running?
      1. No - Is request a nested prompt or debugger stop?
        1. No - Wait until finished
        2. Yes - We're on the pipeline thread, process requests via ThreadController
      2. Yes - Is command request interactive (e.g. F8)?
        1. No - Take control from PSRL via OnIdle
        2. Yes - Cancel PSRL, run command request, restart PSRL

Part of the problem with the above is that so much of the implementation of ExecutionCommand pretends to be (and typically is) asynchronous. However, if it's called on the pipeline thread, it's forced to be synchronous because as soon as you await something you could end up on another thread. If everything goes through a thread controller like class that doesn't even pretend to be async (but instead facilitate async like @rjmholt's example) then we'll have less dead locks.

daxian-dbw commented 4 years ago
  • Can we use this design to fix other debugger issues like Wait-Debugger in an argument completer?

Though this would be way easier to do if we stopped using OnIdle. I don't know if I ever recorded it anywhere, but I regret using that. Instead, PSRL should have a private contract delegate field (or a public API) that it calls on idle if not null. Using PowerShell's eventing is too flimsy and prone to dead locks. Especially when two things try to marshal a call back to the pipeline thread using it, it gets real messy.

A private delegate field can be considered, but not a public API, at least initially. It allows you to avoid the OnIdle event but the execution model is the same -- requests get executed 1-by-1, so we still need to handle the case where 2 things trying to marshal a call back to the pipeline thread. I'm not very clear about the problem of depending on the OnIdle event to marshal script execution to a pipeline thread. Could you please elaborate a little more on it?

The challenge I see is that PSReadLine will hold the pipeline thread (the thread with Runspace.DefaultRunspace set to the PSES Runspace) indefinitely until debug/F8 cancels it. This execution model won't support Wait-Debugger in an argument completer by design, because PSReadLine will be blocked on the CommandCompletion.CompleteInput call when the debugger is triggered, and by that point PSReadLine cannot be cancelled.

The current execution flow is sort of like this:

@SeeminglyScience this summary is helpful for me to read the code, thanks!

SeeminglyScience commented 4 years ago

A private delegate field can be considered, but not a public API, at least initially. It allows you to avoid the OnIdle event but the execution model is the same -- requests get executed 1-by-1, so we still need to handle the case where 2 things trying to marshal a call back to the pipeline thread.

The problem isn't necessarily two things trying to marshal back at the same time, it's the nesting of them (see below).

I'm not very clear about the problem of depending on the OnIdle event to marshal script execution to a pipeline thread. Could you please elaborate a little more on it?

Yeah for sure, lets take this series of events:

  1. PSRL starts
  2. Intellisense is requested
  3. PSES uses OnIdle to take over PSRL and run TabExpansion2
  4. An argument completer or GetDynamicParameters method starts a new thread to process in
  5. In that thread, a call to a PowerShell API that needs to run on the pipeline thread is called
  6. That PowerShell API (like CommandInfo.get_Parameters for instance) queues it's invocation in the pipeline thread via PSEventManager and blocks until completed
  7. PSEventManager is still processing the OnIdle event from step 3. Step 3 is blocking on step 6 completing, which it never will because that event queued in step 6 won't start processing until step 3 is done.

(this is a real example btw, PackageManagement was doing this before @rjmholt fixed it)

The challenge I see is that PSReadLine will hold the pipeline thread (the thread with Runspace.DefaultRunspace set to the PSES Runspace) indefinitely until debug/F8 cancels it. This execution model won't support Wait-Debugger in an argument completer by design, because PSReadLine will be blocked on the CommandCompletion.CompleteInput call when the debugger is triggered, and by that point PSReadLine cannot be cancelled.

Well Wait-Debugger will synchronously call our DebugStop event handler no? It shouldn't matter that PSRL is still running because it gave pipeline thread control to the argument completer, which then gave control to the debug stop event, which then gave it to us.

@SeeminglyScience this summary is helpful for me to read the code, thanks!

❤️

TylerLeonhardt commented 4 years ago

OnIdle, if we're talking about the Engine event, scares me because of odd behavior with PSRL:

Register-EngineEvent -SourceIdentifier PowerShell.OnIdle -Action { [System.Console]::WriteLine("hi") }

On Windows, you have a steady stream of hi's. On Unix, you get one... and then have to press ENTER for the next one to appear.

We get around this odd behavior with our injected ReadKey though... so maybe it's ok to depend on.

It just makes me nervous.

SeeminglyScience commented 4 years ago

@TylerLeonhardt That isn't really related to the engine event. That'll be the case with any method we use to take over PSRL. This happens because of internal locking on corefx's stdin handle. Also in case it wasn't clear, that event is what we currently use.

daxian-dbw commented 4 years ago
  1. An argument completer or GetDynamicParameters method starts a new thread to process

This is really unfortunate ☹️ I'm not sure how using a contract delegate field can help in this scenario:

  1. PSRL starts by directly calling PSReadLine on a thread with Runspace.DefaultRunspace set.
  2. Intellisense is requested.
  3. PSRL calls the private contract delegate, which calls TabExpansion2 in a pipeline. So now, there is a running pipeline.
  4. An argument completer or GetDynamicParameters method starts a new thread to process
  5. In that thread, a call to a PowerShell API that needs to run on the pipeline thread is called
  6. That PowerShell API (like CommandInfo.get_Parameters for instance) queues it's invocation in the pipeline thread via PSEventManager and blocks until completed
  7. PSEventManager attempts to call PulseEngine to start a pulse pipeline, but find there is a running pipeline, so PulseEngine does nothing and returns. Now PSEventManager is depending on the currently running pipeline to process the event action.
  8. The currently running pipeline is blocked in TabExpansion2 on the call to the argument completer or GetDynamicParameters and thus cannot handle the pending event action. Deadlock happens.

It's essentially the same as using OnIdle event. When using the OnIdle event, TabExpansion2 will be running in a pipeline started by PSRL calling ps.AddScript("0", useLocalScope: true).Invoke(). So when the GetDynamicParameters event comes in, PSEventManager finds that there is a running pipeline, and will just depend on the running pipeline to handle the event action. But the running pipeline is blocked, and hence deadlock.



The challenge I see is that PSReadLine will hold the pipeline thread (the thread with Runspace.DefaultRunspace set to the PSES Runspace) indefinitely until debug/F8 cancels it. This execution model won't support Wait-Debugger in an argument completer by design, because PSReadLine will be blocked on the CommandCompletion.CompleteInput call when the debugger is triggered, and by that point PSReadLine cannot be cancelled.

Well Wait-Debugger will synchronously call our DebugStop event handler no? It shouldn't matter that PSRL is still running because it gave pipeline thread control to the argument completer, which then gave control to the debug stop event, which then gave it to us.

But do you need to cancel PSRL in this scenario? PSRL is blocked on CompleteInput and cannot be canceled at that point. I thought PSRL needs to be cancelled for debugger to work properly in PSES, but I may be wrong.

SeeminglyScience commented 4 years ago

I'm not sure how using a contract delegate field can help in this scenario:

(...)

  1. The currently running pipeline is blocked in TabExpansion2 on the call to the argument completer or GetDynamicParameters and thus cannot handle the pending event action. Deadlock happens.

If that caused a deadlock though, it wouldn't function in the normal PowerShell console either. When I was troubleshooting that dead lock, it was either just waiting for the event to be queued or awaiting the events completion. I didn't step through a normal console to see the actual code path though, so I don't know the specifics around how the event normally gets processed even though a pipeline is running.

But do you need to cancel PSRL in this scenario? PSRL is blocked on CompleteInput and cannot be canceled at that point. I thought PSRL needs to be cancelled for debugger to work properly in PSES, but I may be wrong.

It needs to be canceled to launch debugging. Like to run a script, but not necessarily for debugging to occur. Even then I wouldn't really say that cancelling it is required, blocked should also do just fine. Thats assuming PSRL reacts alright to nested ReadLine calls (which may not be the case). Edit: to be clear I'm talking in the context of this new hypothetical implementation, currently it's always cancelled.

daxian-dbw commented 4 years ago

If that caused a deadlock though, it wouldn't function in the normal PowerShell console either.

I'm now curious how the execution flow works in pwsh console when this scenario happens.

SeeminglyScience commented 4 years ago

I'm now curious how the execution flow works when this scenario happens.

Right? I feel like I've seen other scenarios where pulse engine isn't afraid to run something at a questionable time. I've never actually given it much thought but yeah that's real strange. I wonder if there's just a hole in the logic that could cause some state corruption.

daxian-dbw commented 4 years ago

I took a look at the related code, here is why it works in a normal PowerShell console but not in PSES when running script via the OnIdle:

  1. CommandInfo.GetMergedCommandParameterMetadataSafely sets waitForCompletionInCurrentThread: true when generating the event. That means the call to Events.GenerateEvent will block until the event action is executed, but if the action cannot be processed on the pipeline thread of the expected Runspace (either a normal one or the pulse pipeline), then it will be executed on the calling thread instead. (This is a known hole that may case state corruption, and we have seen something similar in PowerShell class)
  2. However, before EventManager forces to run the action on the calling thread, it checks to see if an event action is being processed in the pipeline thread, and if so, it just assumes the action will get its turn soon enough and continue to wait.
    • In a normal PowerShell console, no event action is being processed when EventManager does the check, so the action is forced to run on the calling thread.
    • In PSES with OnIdle, the OnIdle event action is being processed, so EventManager keeps waiting. There, the deadlock.

A contract delegate field in PSReadLine will be the more reliable way to go.

daxian-dbw commented 4 years ago

Even then I wouldn't really say that cancelling it is required, blocked should also do just fine. Thats assuming PSRL reacts alright to nested ReadLine calls (which may not be the case)

The nested ReadLine call may be OK, but not the outer ReadLine call. The first thing ReadLine call does is to clear the line buffer, assuming what's in it was from the last ReadLine call and is no use anymore. So when returning back to the outer ReadLine call, the line buffer is no longer what it used to be. So are the other states, e.g. cursor position.

But that's how it works today in a normal PowerShell console for the repro steps in https://github.com/PowerShell/vscode-powershell/issues/2448 The behavior is certainly not right when you type q or c and Enter in the debugger prompt, but I guess still acceptable.

SeeminglyScience commented 4 years ago

The nested ReadLine call may be OK, but not the outer ReadLine call. The first thing ReadLine call does is to clear the line buffer, assuming what's in it was from the last ReadLine call and is no use anymore. So when returning back to the outer ReadLine call, the line buffer is no longer what it used to be.

I think that's fine though. Like ideally yeah PSRL would revert back to the previous state and fully rerender, but I think for the use case of quickly debugging an argument completer losing the original state is 👌. I mean that's what it does in a normal console anyway.

Run this:

Register-ArgumentCompleter -CommandName Get-ChildItem -ParameterName Filter -ScriptBlock { Wait-Debugger }

Then type Get-ChildItem -Filter TAB. It's a little funky when you continue but doable.

.....wait.... no that works real weird. If you c it executes the first file in the current directory. The default argument completer becomes the whole input buffer and PSRL still thinks input has been accepted.

Well we could mimic that behavior at least... but 🤷

Edit: Ah you also addressed that weird behavior, didn't see that.

TylerLeonhardt commented 4 years ago

This work can fix this issue as well: https://github.com/PowerShell/PowerShellEditorServices/issues/1296

TylerLeonhardt commented 4 years ago

So anyone wanna pick this up? 😅

rjmholt commented 4 years ago

So I've made some inroads here, in https://github.com/rjmholt/PowerShellEditorServices/tree/async-ps-consumer (see https://github.com/rjmholt/PowerShellEditorServices/tree/async-ps-consumer/src/PowerShellEditorServices/Services/PowerShell).

I'm currently at the stage where I'm calling PSConsoleReadLine.ReadLine() on the pipeline thread (directly as a delegate), however the readline call either blocks or, if I poll it, erases the input and causes the cursor to blink quickly.

At this point I think I need to understand more about how the current implementation and PSReadLine itself works, ideally from @daxian-dbw and @SeeminglyScience.

rjmholt commented 4 years ago

Ok I resolved the above by understanding that we rely heavily on the OnIdle engine event.

But the next problem I hit is that PSReadLine expects to be initialised and run from an active PowerShell pipeline, because it uses PowerShell.Create(RunspaceMode.CurrentRunspace). Here and here.

The solutions here are:

For this, the two things I need to understand are:

TylerLeonhardt commented 4 years ago

The internal cmdlets even have this issue : https://github.com/PowerShell/PowerShellEditorServices/issues/1296

So they're really not the right solution

rjmholt commented 4 years ago

Ok I've implemented the contract delegate and that seems to work very nicely (although we are now responsible for handling engine events ourselves, but that's fine...).

Next problem is that the completion handler is cancelling within the handler for some reason... So it never even tries to complete

rjmholt commented 4 years ago

The above is now working

rjmholt commented 4 years ago

So far I've got the REPL, F8, and other executions working, as well as setting the execution policy and loading profiles.

I've yet to start on integrating properly with remoting and debugging, but there are some things I feel I should understand first:

1

The old host we use reimplements a lot of logic already available in the console host, particularly in terms of prompting and colours. Is there a reason for that? For example:

https://github.com/PowerShell/PowerShellEditorServices/blob/ccec61c09c7da4e62b87da148764972d71934247/src/PowerShellEditorServices/Services/PowerShellContext/Session/Host/EditorServicesPSHostUserInterface.cs#L261-L310

Compare that to my current implementation where we just call into the console host. I'm wary that this might be because we don't want to call ConHost's ReadLine method, but in the small smoke tests I've done it doesn't seem to be an issue. In both old and new implementations, PSES blocks completions when waiting in the Prompt() call.

2

The old host also jumps through hoops to expose strange terminal colour properties. Like:

https://github.com/PowerShell/PowerShellEditorServices/blob/ccec61c09c7da4e62b87da148764972d71934247/src/PowerShellEditorServices/Services/PowerShellContext/Session/Host/TerminalPSHostUserInterface.cs#L80-L96

and

https://github.com/PowerShell/PowerShellEditorServices/blob/ccec61c09c7da4e62b87da148764972d71934247/src/PowerShellEditorServices/Services/PowerShellContext/Session/Host/EditorServicesPSHost.cs#L82-L237

I don't understand why it does this. In my implementation I don't seem to need to do this... The only strange thing I've noticed is that Get-Error does colourise as I'd like -- but not sure what to add there.

What are the colour properties for and are there any recommendations on how to provide them?

3

PowerShellExecutionContext has this concept of a PromptContext and a PromptNest, but I'm not sure why they exist or what they do... Do they just track how deep our nested pipelines/prompts are? I basically need to understand their purpose in order to work out what should remain of them.

4

I still don't fully understand the concept of a nested prompt. What is its purpose and what constraints does it impose? I don't need to understand this one quite so much, but it might help with my final question.

5

For remoting, how does the PowerShell session adopt the remote runspace. Is this done implicitly by Enter-PSSession by using the PushRunspace() method on the host, or is there something else I actively need to do to make this work? Again I haven't looked into this properly yet, but figure I should front-load on info.

SeeminglyScience commented 4 years ago

The old host we use reimplements a lot of logic already available in the console host, particularly in terms of prompting and colours. Is there a reason for that? For example:

For prompts, the main reason it's still like that is so it can be cancelled for F8/F5 I think? There's some scenario we need to programatically cancel it where ConsoleHost's version won't obey PowerShell.Stop requests...I think. I'm growing less sure by the second that that's still relevant.

Historically it was because it used to be tied to the VSCode GUI, and also because all original implementations didn't call back to ConsoleHost (no one thought to save onto $Host from the first runspace until I added that for WriteProgress support).

I don't understand why it does this. In my implementation I don't seem to need to do this... The only strange thing I've noticed is that Get-Error does colourise as I'd like -- but not sure what to add there.

What are the colour properties for and are there any recommendations on how to provide them?

It's for user color customization. The current impl mirrors what ConsoleHost does.

PowerShellExecutionContext has this concept of a PromptContext and a PromptNest, but I'm not sure why they exist or what they do... Do they just track how deep our nested pipelines/prompts are?

Pretty much. It basically keeps track of a...instance of a prompt. There's probably a term for what I'm talking about but I'm not sure, so I'll just give a few examples of what I consider to be a prompt instance:

  1. The initial REPL
  2. Debugger stop
  3. Remote session (Enter-PSHostProcess, Enter-PSSession, etc)
  4. A nested prompt (e.g. $Host.EnterNestedPrompt())

It's important to know what type of prompt instance you're currently on as it changes a few things about how commands are invoked:

  1. Debugger stop: changes whether we run PowerShell.Invoke or Debugger.ProcessCommand
  2. Nested prompt: must run synchronously since we're already on the pipeline thread. Not super applicable anymore if we always are
  3. Remote: A lot of things change subtly like a different overload for completion, some APIs are not support or unreliable. Also need to set the Runspace instead of PowerShell

I basically need to understand their purpose in order to work out what should remain of them.

A lot of it can probably go or at least be simplified. I wrote most of that pretty early into learning C# FWIW

I still don't fully understand the concept of a nested prompt. What is its purpose and what constraints does it impose? I don't need to understand this one quite so much, but it might help with my final question.

It's kinda like an impromptu debugger stop. It's significantly more simple in it's execution than debugging. No extra command processing (like c for continue, etc), no different downstream cmdlet (iirc ProcessCommand appends Out-String instead of Out-Default which messes with some things like ANSI), no extra variables. It's also instant, unlike Wait-Debugger which waits until the next sequence point.

The main thing that sets it apart though, is that it can be (almost) infinitely nested.

Here are some examples I use often:

type exit when done

# Creates a prompt within the executing advanced script, giving you access to a live
# instance of PSScriptCmdlet to explore
[CmdletBinding()]param()$p = $PSCmdlet; $Host.EnterNestedPrompt()
# Explore Pester internals from the perspective of the psm1 (scope wise)
# Useful for "exploratory surgery" style debugging when you don't know where to start.
# Also changes up the context of tab completion to be of a module's scope.  Includes intellisense.
. (gmo Pester) { $Host.EnterNestedPrompt() }

For remoting, how does the PowerShell session adopt the remote runspace. Is this done implicitly by Enter-PSSession by using the PushRunspace() method on the host, or is there something else I actively need to do to make this work? Again I haven't looked into this properly yet, but figure I should front-load on info.

Yeah PushRunspace is host's hook. From there you mainly want to make note of the fact that you're in a remote session (so you can invoke commands/readline differently), register debugger events, and load any state you want (like psedit and friends)

rjmholt commented 4 years ago

It's kinda like an impromptu debugger stop. It's significantly more simple in it's execution than debugging. No extra command processing (like c for continue, etc), no different downstream cmdlet (iirc ProcessCommand appends Out-String instead of Out-Default which messes with some things like ANSI), no extra variables. It's also instant, unlike Wait-Debugger which waits until the next sequence point.

Ah ok, so this implies that they are markedly different, rather than nested prompts being a general way to implement debugger stops. I assume PowerShell handles the special commands and sequence point semantics of the debugger and we just need to provide the REPL...?

SeeminglyScience commented 4 years ago

Ah ok, so this implies that they are markedly different, rather than nested prompts being a general way to implement debugger stops.

Yeah... iirc there was actually some difficulty in getting debugging to always enter a nested prompt. I know that's how ConsoleHost does it, but I think that's one of those things that are meant to apply generally but end up being difficult to implement outside of PowerShell itself. Either way, yeah that's right.

I assume PowerShell handles the special commands and sequence point semantics of the debugger and we just need to provide the REPL...?

Sooort of. Basically when we're in a debug stop we need to use Debugger.ProcessCommand instead of directly invoking a PowerShell instance. When you use that to invoke a command, breakpoints won't be triggered, and the debug commands work. If you don't use that and you set a breakpoint like sbp -Variable PSScriptRoot -Mode Write things explode quickly.

ProcessCommand will also tell you if there is a debug related action to take. For instance, if the user writes c, it'll tell us the debugger needs to continue. Which we then relay back to PowerShell via DebuggerStopEventArgs.ResumeAction. That's the other thing the PromptNest and what not currently controls, resume actions.

TylerLeonhardt commented 4 years ago

The fact that debugging uses Debugger.ProcessCommand is also why #1296 exists only when debugging.

rjmholt commented 4 years ago

Ok, I'm currently hitting an issue with nested prompts.

This occurs when I enter a nested prompt from the command line, like $Host.EnterNestedPrompt(), which calls this method.

I create a nested PowerShell here, set that for the current task and then invoke it (the extension method implementation is here).

When I go to invoke it, I hit the issue where the parent pipeline isn't running, so I can't run the nested one... Here's where the exception is thrown, because currentPipeline is null. Not sure what I'm doing wrong here.

I think it comes down to the fact that I end up with two threads, even though I'm using ReuseThread...

threads

Given my configuration I would expect my child invocation to be under the same call stack as the parent, but instead it's been push onto a new thread, and I think that's what I need to prevent...

SeeminglyScience commented 4 years ago

@rjmholt don't use PowerShell.CreateNestedPowerShell. It sets IsNested, but not IsChild. Dunno why. Not sure if that'll actually help, but try PowerShell.Create(RunspaceMode.CurrentRunspace) instead.

I think it comes down to the fact that I end up with two threads, even though I'm using ReuseThread...

The one we want is UseCurrentThread. Here's a breakdown of how the thread options work:

rjmholt commented 4 years ago

The one we want is UseCurrentThread

Yes, just realised that now

rjmholt commented 4 years ago

don't use PowerShell.CreateNestedPowerShell. It sets IsNested, but not IsChild

Ugh, how annoying. Ok

rjmholt commented 4 years ago

Ok so on the Out-Default/debugging point:

rjmholt commented 4 years ago

/cc @PaulHigin -- in case you know what the intended behaviour of the debugger is when Out-Default is used, or know if there's a "right" way to print command output from the debugger

rjmholt commented 4 years ago

Related question: will something like this cancel a command properly:

// Thread 1 - runs PowerShell command in debugger
pwsh.Runspace.Debugger.ProcessCommand(psCommand, outputCollection);

// Thread 2 - wants to cancel the PowerShell command while its running
pwsh.Stop();

This works when executing with pwsh.Invoke(), but does the debugger and its different entry point change this? Is there a way to cancel commands run in the debugger?

SeeminglyScience commented 4 years ago

Is there a way to cancel commands run in the debugger?

Yeah Runspace.Debugger.StopProcessCommand()

PaulHigin commented 4 years ago

I'm not sure what you mean by debugger and 'Out-Default' command. Can you provide an example?

I have not read all of the above threads, but it seems like it may be time to talk about changing the script debugging stop event mechanism. I've always disliked the fact that script debugging relies on the host holding on to the event thread. I'm sure this was done in ancient times to implement the first simple script debugger for PowerShell. But this made remote debugging extremely difficult to implement, and sounds like there are similar issues for PSES.

It seems to me that the script debugger stop/resume functions should be handled internal to the engine script debugger, and that there should be a straightforward debugger API. This would be a breaking change to existing third party PowerShell hosts (ISE, etc.). But since most hosts are for WindowsPowerShell, which we are no longer updating, now may be the time to make the change.

rjmholt commented 4 years ago

I'm not sure what you mean by debugger and 'Out-Default' command. Can you provide an example?

Yes, sorry. Basically when we implement our prompt/REPL, we quietly tack Out-Default onto the end of commands that we run, so that the output of your REPL-input command is piped to Out-Default, which renders and displays it in the Host's UI.

However, when we execute with Debugger.ProcessCommand(psCommand) rather than PowerShell.Invoke(), we see no output. I can upload a little GIF comparing the two while stepping through in the C# debugger too for clarification.

I haven't gotten to remote attachment yet, but anything that makes it easier is welcome, and as I do more work here and understand all the mechanisms in play better, I'm probably better placed to understand what's good and bad about the current API and what might be nice to have.

But since most hosts are for WindowsPowerShell, which we are no longer updating, now may be the time to make the change.

I think it's mostly safe to assume that: