Repository navigation
[Investigate] Fix PowerShell event handling #1591
Description
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:
- 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.
- 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.
- The
PSLocalEventManagerclass 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. - 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.
Activity
andyleejordan commentedon Oct 21, 2021
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 commentedon Oct 21, 2021
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 commentedon Oct 21, 2021
So the underlying event manager is directly available with
$ExecutionContext.Events. We'd need to do (3) to override it.Normally thought,
Register-EngineEventandRegister-ObjectEventwould be what most people would use, and we could conceivably override those cmdlets to do something else with them...rjmholt commentedon Oct 21, 2021
/cc Dongbo Wang (@daxian-dbw)
andyleejordan commentedon Oct 21, 2021
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 commentedon Oct 27, 2021
Quoted from PowerShell/PSReadLine#1679 (comment):
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
OnIdleevent in such case.andyleejordan commentedon Oct 27, 2021
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 commentedon Oct 27, 2021
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.
15 remaining items
[-][Revamp pipeline thread handling] Fix PowerShell event handling[/-][+][Investigate] [Revamp pipeline thread handling] Fix PowerShell event handling[/+][-][Investigate] [Revamp pipeline thread handling] Fix PowerShell event handling[/-][+][Investigate] Fix PowerShell event handling[/+]SeeminglyScience commentedon May 9, 2022
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 commentedon May 10, 2022
I remember the repro is to register an event subscriber to the
Engine.Idleevent, in the integrated console.SeeminglyScience commentedon May 10, 2022
Then I think we're good 🎉
SeeminglyScience commentedon May 10, 2022
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 settingRunspace.DefaultRunspacewould fix that I believe.Part of the changes I made for attach to process is creating a new
PowerShellinstance for every context frame rather than reusing the old one. When I did that, since I was usingCurrentRunspace, it's now marked as nested.I'm gonna go ahead and close this
daxian-dbw commentedon May 10, 2022
Patrick Meinecke (@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 commentedon May 10, 2022
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 callingPsesInternalHost.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
Nestedso it knows we're already on the right thread. So it knows we're the one making it busy. Since it wasn't markedNested, it just threw every time we tried to trigger PowerShell event processing.daxian-dbw commentedon May 10, 2022
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 commentedon May 10, 2022
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 commentedon May 10, 2022
Patrick Meinecke (@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 commentedon May 11, 2022
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: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! 😁