[Investigate] Fix PowerShell event handling

[rjmholt]

From PowerShell/PSReadLine#1679 (comment).

PowerShell events when run in a non-nested shell try to phone home back to the runspace in which they were registered, with the event handler calling in from another thread.

The assumption PowerShell makes is that when an action occurs and we pulse the pipeline, if the pipeline isn't nested, it must be on a different thread.

Of course in our case, the pipeline is on our thread and isn't nested, because we're running it under an application. This seems to be an oversight in the PowerShell hosting model and I think it needs to be addressed in PowerShell; any attempt to run a PowerShell pipeline on a thread not dedicated to it will have this issue.

Unfortunately for us though, this is a behaviour we're stuck with in Windows PowerShell and PS 7.0, 7.1 and 7.2, so we have to work out what mitigation we can provide.

There are a couple of possible solutions:

1. We can construct an artificial internal cmdlet as our "top level" pipeline and put all of our actual execution underneath it. This would be relatively simple and would probably only have a bit of overhead, but would mean our pipelines are always nested, which could possibly have side-effects.
2. We could experiment with using reflection to just lie to PowerShell about our top-level pipeline being nested. This would probably hit errors somewhere, but might be worth a try.
3. The PSLocalEventManager class is an internal extension of a public abstract class and most of its methods are virtual. We might conceivably be able to generate an override to this class and set it internally in the engine to redirect the eventing flow back to us. I'm not sure if this is possible, and if it is it would probably be a fair amount of work. But it would be the most "integrated" approach.
4. Abandon the event loop on the runspace thread and instead use it as a pump for a dedicated runspace thread. This would mean we can no longer run delegates on the runspace thread as we currently do (including at startup, when we implicitly run all our code on the runspace thread so that things like PSReadLine work correctly). We could address this by changing the delegate task implementation to use another internal cmdlet that simply executes the given delegate in its EndProcessing() block. But this would require some reimplementation inside the internal host.

It's also worth noting that, as far as we can tell, the ISE doesn't have any awareness of this scenario either. So it's not clear if this is a quiet bug in the ISE that either hasn't been noticed or hasn't been large enough to come to attention.

For now, I think, event handling in PSES isn't important enough to invalidate the work here and this is something I'm hopeful can be fixed later.

[andyleejordan]

It's also worth noting that, as far as we can tell, the ISE doesn't have any awareness of this scenario either. So it's not clear if this is a quiet bug in the ISE that either hasn't been noticed or hasn't been large enough to come to attention.

For now, I think, event handling in PSES isn't important enough to invalidate the work here and this is something I'm hopeful can be fixed later.

I agree with this. I think we can move forward with the caveat that the integrated terminal does not support PowerShell or .NET events (or at least, not well). Technically the integrated terminal isn't a fully-fledged PowerShell process, it's the integration point between the editor and editor services that just so happens to let the user run code in the runspace hosting the editor's services. I know it doesn't quite look like that to end user, but it is what it is, and presumably the ISE is somewhat similar.

[andyleejordan]

Is there a way we could, like, turn off PowerShell events? Throw an error if one tries to be registered? Or would that involve having to do one of the four proposed workarounds?

[rjmholt]

So the underlying event manager is directly available with $ExecutionContext.Events. We'd need to do (3) to override it.

Normally thought, Register-EngineEvent and Register-ObjectEvent would be what most people would use, and we could conceivably override those cmdlets to do something else with them...

[rjmholt]

/cc @daxian-dbw

[andyleejordan]

This seems like a problem where we could get 80% of it working (most use cases, like those two cmdlets) pretty easily (override them to queue the events on our thing), and it's just the last 20% (making all events including .NET-esque ones) is what would be very hard.

[daxian-dbw]

Quoted from PowerShell/PSReadLine#1679 (comment):

In order for PowerShell eventing to work with the dedicated pipeline thread design in PSES, PowerShell needs to allow disabling the pulse pipeline generation and OnIdle events.

I think we should do this in PowerShell engine. Other events can still be generated, and it's up to the hosting application to spin up pulse pipeline to allow event handling. Also, it's up to the hosting application to define when it is idle, and generate OnIdle event in such case.

[andyleejordan]

I think that'd be a great change, and it would fix this for future versions of PowerShell Core, but sadly it'll remain quite broken for Windows PowerShell 😫

[daxian-dbw]

Also, we (maybe me) need to go back review the ISE code closely to understand how it does things, for example, is it using a dedicated pipeline thread? how does it handle the PS eventing? It doesn't need to deal with PSReadLine which makes it a lot simpler, but still good to know how things work with it.

[SeeminglyScience]

Note that you can't actually implement PSEventManager externally since two abstract members are internal:

[Management.Automation.PSEventManager] | Find-Member -Force -Abstract | Find-Member -Not -AccessView Child

   ReflectedType: PSEventManager

Name                  MemberType   Definition
----                  ----------   ----------
AddForwardedEvent     Method       internal abstract void AddForwardedEvent(PSEventArgs forwardedEvent);
ForwardEvent          Event        internal abstract event EventHandler<PSEventArgs> ForwardEvent { add; remove; }

That was probably meant to be internal protected. Side note, those methods do not appear to be included in any of the ref assemblies either. So you can compile an implementation of PSEventManager, but it'll throw on type load with this:

Method 'AddForwardedEvent' in type 'TestEventManager.TestEventManager' from assembly 'TestEventManager, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null' does not have an implementation.

[andyleejordan]

2. We could experiment with using reflection to just lie to PowerShell about our top-level pipeline being nested. This would probably hit errors somewhere, but might be worth a try.

For what it's worth I tried this in CreateInitialPowerShell

            // HACK: This fools PowerShell into thinking that our top-level
            PropertyInfo isNestedProperty = typeof(PowerShell).GetProperty("IsNested");
            MethodInfo isNestedSetter = isNestedProperty.GetSetMethod(nonPublic: true);
            isNestedSetter.Invoke(pwsh, new object[]{ true });

And while it correctly set IsNested, yup, then threw an error:

Exception thrown: 'System.Management.Automation.PSInvalidOperationException' in System.Management.Automation.dll: 'You should only run a nested pipeline from within a running pipeline.'
   at System.Management.Automation.Runspaces.PipelineBase.DoConcurrentCheck(Boolean syncCall, Object syncObject, Boolean isInLock)

I think from here: https://cs.github.com/PowerShell/PowerShell/blob/6c22065d77f0d150484ac0234938b0afb7ee11f1/src/System.Management.Automation/engine/hostifaces/pipelinebase.cs?q=PipelineBase+DoConcurrentCheck#L696

[SeeminglyScience]

For what it's worth I tried this in CreateInitialPowerShell

Yeah it's Pipeline.IsNested. Unfortunately that won't exist until runtime.

---

Did we switch back to using ReuseThread at some point? I thought we were using CurrentThread which would save the correct thread.

If we did, we could also take the PSRL approach and just invoke some quick meaningless powershell like AddScript("0") to force it. Pretty dirty, but it should work.

[andyleejordan]

@SydneyhSmith Marking this as a discussion since we're tracking the "bug" (mitigation fix) in PowerShell/vscode-powershell#3701

[SeeminglyScience]

Does anyone remember what specifically they used to repro this? Seems to be working for me, it's possible I accidentally fixed this while getting attach to process working

[daxian-dbw]

I remember the repro is to register an event subscriber to the Engine.Idle event, in the integrated console.

[SeeminglyScience]

Then I think we're good 🎉

[SeeminglyScience]

FWIW I think the issue was here:

https://cs.github.com/PowerShell/PowerShellEditorServices/blob/dd870ec1887c3cd69f18459d3e226067ab4f2098/src/PowerShellEditorServices/Services/PowerShell/Host/PsesInternalHost.cs#L940

That just creates a instance of PowerShell and assigns the runspace directly. What needed to happen was PowerShell.Create(RunspaceMode.CurrentRunspace) needed to be called instead to mark it as nested. That might have thrown at that point in initialization, but setting Runspace.DefaultRunspace would fix that I believe.

Part of the changes I made for attach to process is creating a new PowerShell instance for every context frame rather than reusing the old one. When I did that, since I was using CurrentRunspace, it's now marked as nested.

I'm gonna go ahead and close this

[daxian-dbw]

@SeeminglyScience As far as I can remember, it was because sometimes there was not running pipeline in the default session, and when an event was triggered in that case, the event manager created a pulse pipeline. However, while that pulse pipeline was running, something happens in the extension that causes a task to be queued and executed. Since they both use the same Runsapce at that point, the "a pipeline already running" exception will be thrown.
Is it now guaranteed that there is always a running pipeline within the default session of the integrated console?

[SeeminglyScience]

So I think what was happening was we create an instance of PowerShell that is not marked as Nested. Then in our OnIdle delegate, we were calling PsesInternalHost.InvokePSCommand("0") to trigger event handling.

But since we were doing that while the pipeline thread was technically busy, we needed the PowerShell instance to be marked Nested so it knows we're already on the right thread. So it knows we're the one making it busy. Since it wasn't marked Nested, it just threw every time we tried to trigger PowerShell event processing.

[daxian-dbw]

we needed the PowerShell instance to be marked Nested so it knows we're already on the right thread

That explains it. This is great! I thought we would have to fix how event manager firing pulse pipeline in PowerShell, but this resolves the problem perfectly!

[andyleejordan]

I swear Rob and I both tried to get it marked Nested properly and ran into issues. IDK what you did, but you did good!

[daxian-dbw]

@SeeminglyScience Would be good to talk about how you did it (and maybe what was done wrong before) in the team meeting this week 😄

[SeeminglyScience]

I swear Rob and I both tried to get it marked Nested properly and ran into issues. IDK what you did, but you did good!

I would guess that y'all did try PowerShell.Create(RunspaceMode.CurrentRunspace) but hit this error here due to how early it was being called. If so, that could have been fixed with:

var oldRunspace = Runspace.DefaultRunspace;
try
{
    Runspace.DefaultRunspace = theOneWeJustMade;
    return PowerShell.Create(RunspaceMode.CurrentRunspace);
}
finally
{
    Runspace.DefaultRunspace = oldRunspace;
}

@SeeminglyScience Would be good to talk about how you did it (and maybe what was done wrong before) in the team meeting this week 😄

That's a good idea but I'll have to see if I can find a way to present the information in a way that's useful. So next week! 😁