Add private contract delegate for PSES to handle idle on readline

[rjmholt]

PR Summary

Adds a private delegate field for PSES (and possibly other hosts) to override the OnIdle behaviour of PSRL while it's awaiting input.

Also adds PDBs to debug builds (needed in this PR so I could debug PSRL and PSES together).

Needed for PowerShell/PowerShellEditorServices#1295.

Today PSES executes script to run PSRL's readline function and depends on ordinary PowerShell event handling within PSRL to run most of its functionality (like completions) while a prompt is shown.

However, this has several drawbacks:

• We cannot run script like the PSRL readline invocation in ConstrainedLanguageMode, so we wrote cmdlets to perform the invocation instead. These cmdlets though are either exposed in the integrated console, or must be invoked with reflection within PSES. This represents complexity and de-cohesion to PSES,
• So with the new pipeline execution thread implementation, we bypass this to run the readline method directly on the pipeline thread (which we control with a consumer, allowing us to intersperse PowerShell commands and binary delegates). However, PS event handling requires a nested pipeline, meaning running the readline delegate directly crashes when events are handled.
• PS events themselves are not ideal for processing background tasks like completions. They seem to work differently between Windows and *nix (events are processed with a different cadence), and there are some operations that cannot be processed as events, instead causing deadlocks with PSRL like [Powershell Preview regression bug] Debugger hangs when a breakpoint inside a scriptblock is hit vscode-powershell#2448 and Integrated console hangs if PSReadline sets AddToHistoryHandler vscode-powershell#2246

Instead, a delegate allows PSES to take responsibility for all processing while a prompt is waiting; it can process events, perform completions, run commands, even cancel the current prompt and drop into the debugger

PR Checklist

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Make sure you've added one or more new tests
• User-facing changes
  • Not Applicable
  • OR
  • Documentation needed at PowerShell-Docs
    • Doc Issue filed:

[daxian-dbw]

@rjmholt Can you please summarize the reason of this change, so that we can get @lzybkr on the same page.
Also, can we remove the old private "trigger-the-onidle-event" property/code since PSES is moving away from it?

[SeeminglyScience]

Also, can we remove the old private "trigger-the-onidle-event" property/code since PSES is moving away from it?

That's not there for PSES, it needs to stay to process user registered events.

[rjmholt]

That's not there for PSES, it needs to stay to process user registered events.

Oh! Right, I thought @daxian-dbw meant something else. Yeah, I was under the impression that that's for event handling in normal conhost execution and we just happened to use it.

[SeeminglyScience]

Oh you know what, he might be talking about PSConsoleReadLine.ForcePSEventHandling. If so yeah that can probably go.

[SeeminglyScience]

Won't this continue hard stop event processing/cancellation handling?

[rjmholt]

So the current wait handle will handle cancellation -- that happens above the delegate call. We also own the cancellation token, so we have a lot of control there.

Event processing is what we need to prevent, because if we invoke PSRL directly as a delegate with no pipeline running above it, event handling will fail. PSES owns the pipeline and pretty much everything else at this point, so I think it's up to us to handle events in our own way.

[SeeminglyScience]

we invoke PSRL directly as a delegate with no pipeline running above it, event handling will fail

Even if you set Runspace.DefaultRunspace before calling PSRL? I was pretty certain Create(RunspaceMode.CurrentRunspace) worked as long as that's set. It's definitely supposed to at least, there's code for it.

Or is it failing some other way?

so I think it's up to us to handle events in our own way.

Yeah that's fine if it comes to that, just a shame to implement the same logic in two places I suppose.

[rjmholt]

Hmmm, well in fact looking into this, it may be that I'm not quite invoking the delegate the right way (i.e. not on the pipeline thread). I'll look into it and get back to you, because you may well be right! (Will keep this in draft until kinks like this are resolved)

[rjmholt]

Just looked into this, and yeah the delegate is marshalled back to the pipeline thread and called there (and we have Runspace.DefaultRunspace set). I'll see if I can try again, but that's the whole reason for the continue, since PSRL's default event handling didn't work for us in the top-level pipeline (I spent ages debugging it...)

[SeeminglyScience]

If the event handling doesn't work, make sure TabExpansion2 and script based custom key handlers still work.

I worry that if PSRL's event handling doesn't work, I'm not 100% sure how we're going to handle that better in PSES you know? If we lose the ability to process events it might be better to ditch the delegate.

[SeeminglyScience]

@rjmholt Can you please summarize the reason of this change, so that we can get @lzybkr on the same page.

Here's where I brought it up in PowerShell/PowerShellEditorServices#1295 (comment)

(...) 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.

An example of it getting messy is PowerShell/PowerShellEditorServices#762 (though fixed in oneget now).

[daxian-dbw]

Oh you know what, he might be talking about PSConsoleReadLine.ForcePSEventHandling.

Yeah, that's the one I was talking about. I was lazy to find out the exact member 😄

[andyleejordan]

Will we likely still need this for the PSES threading rewrite @rjmholt?

[rjmholt]

Will we likely still need this for the PSES threading rewrite @rjmholt?

Yes, this is required to allow us to run PSRL as a delegate rather than having to craft a secret cmdlet to run it via PowerShell

[rjmholt]

I'm marking this PR as ready for review, since PowerShell/PowerShellEditorServices#1459 is getting very close and will require a version of PSReadLine with the changes in this PR to be available in order to work as a release

[andyleejordan]

Woo! 🥳

[rjmholt]

The only remaining question here is whether the _handleIdleOverride delegate should take a CancellationToken. Thinking on it, I think maybe it should

[andyleejordan]

The only remaining question here is whether the _handleIdleOverride delegate should take a CancellationToken. Thinking on it, I think maybe it should

You know me, I'm a fan of cancellation tokens.

[andyleejordan]

@daxian-dbw We think we're in a place to publish a (very) preview release of PSES and the extension, but for that we'd need a build of this available. What would it look like to get this merged and a preview release on the gallery?

@rjmholt says we are completely settled on this change to the internal API of PSReadLine. So while this PR updates the version to 2.3.0, I think it would be reasonable to simply include it in 2.2.0 and publish a new beta. Stable PSES is still pinned to PSReadLine 2.1.0, so nothing would break and we could start to coordinate the update to 2.2.0!

/cc @SydneyhSmith @StevenBucher98

[daxian-dbw]

I will review this PR this afternoon and try to release 2.2.0-beta3 PSReadLine early next week.

[daxian-dbw]

By removing _forceEventWaitHandle, how do you break out WaitHandle.WaitAny immediately when you need to? Are you planning to use the cancellation token for that purpose?

[rjmholt]

Yeah, we just cancel in that scenario. It's not perfect because of how PSRL handles cancellation (i.e. it returns a value rather than throwing an OperationCanceledException) but because we know we cancelled, we can take the appropriate action.

[daxian-dbw]

Chatted with @rjmholt offline, and we decided to remove the _forceEventWaitHandle for now as it's not needed in the current PSES pipeline implementation. I will add it back if it turns out to be required in future.

[daxian-dbw]

The way how PowerShell eventing works won't play well with the design that has the pipeline thread to be a single consumer.
The way how eventing works is:

1. A event is generated by a timer (OnIdle) or an arbitrary user thread
2. When handling that event (on the timer event thread or other arbitrary user thread, or a thread pool thread), Runspace.Pulse is called to create a pulse pipeline when there is no current running pipeline.
3. The pulsePipeline.Invok will call into LocalPipeline.StartPipelineExecution with the ReuseThread option, which essentially will create a new thread, and assign the current Runspace to its Runspace.Default, and use it as the pipeline thread to invoke the pulse command (evaluating 0)

That means, when a event is generated and there is no running pipeline at the moment, powershell will create a new thread and use that as the pipeline thread.
However, a synchronous task may be triggered at any moment on the PSES's dedicated pipeline thread, and that means it's possible 2 threads are executing with the same Runspace at some period of time. That would be the source of all sorts of random failures you may observe.

Here is an example:

using System.Management.Automation;
using System.Management.Automation.Runspaces;

var rs = RunspaceFactory.CreateRunspace(InitialSessionState.CreateDefault2());
rs.Open();
Runspace.DefaultRunspace = rs;

var sb = ScriptBlock.Create("[Foo]::Sleep()");

// Create an OnIdle event subscriber. The OnIdle event is geneated by a timer in PowerShell
PSEventSubscriber subscriber = rs.Events.SubscribeEvent(null, null, PSEngineEvent.OnIdle, null, ScriptBlock.Create("[Foo]::EventRaised++"), false, false);
// Run a script block immeadiately
sb.Invoke();

// Check how many times the OnIdle event handler runs.
Console.WriteLine(Foo.EventRaised);

public class Foo
{
    public static int EventRaised { get; set; }
    public static void Sleep()
    {
        System.Threading.Thread.Sleep(5000);
        Console.WriteLine("Done Foo.Sleep!!");
    }
}

Running result is shown below, and that means the OnIdle event handler runs at the same time of sb.Invoke() using the same Runspace.

---

In order for PowerShell eventing to work with the dedicated pipeline thread design in PSES, PowerShell needs to allow disabling the pulse pipeline generation. Unfortunately, I have no idea how to achieve that without code changes in PS engine ☹️

[ghost]

🎉 v2.2.0-beta4 has been released which incorporates this pull request. 🎉