WIP Move to runspace synchronizer from completions

[TylerLeonhardt]

This RunspaceSynchronizer is used to "sync" the state of one runspace to another.
It's done by copying over variables and reimporting modules into the target runspace.
It doesn't rely on the pipeline of the source runspace at all, instead leverages Reflection to access internal properties and methods on the Runspace type.

Lastly, in order to trigger the synchronizing, you must call the Activate method which will trigger the syncing on the next OnIdle event. This is added to the PSReadLine key handler for ENTER.

Let me know what you think of the overall design. From my tests, it appears to be a bit faster 🙂

The idea is, we separate everything that uses the primary runspace (where PSRL likes to hang out)... and if we can achieve that, we can let PSRL take over the whole thread if it wants... Also, by moving the completions to another runspace, when we add async message handling, Completions wont be blocked by access to the default runspace.

TODO: add tests added

[TylerLeonhardt]

maybe this should go somewhere else...

[rkeithhill]

Cool. Presumably an F8 run or debug session would cause Activate to be called (to catch Import-Module and Add-Type calls)?

[TylerLeonhardt]

Very good point! I should add an Activate call to F8 and a few others

[TylerLeonhardt]

I've added support for F8... but I wonder... do we even want our own F8 support anymore when we can just use the "Run selected text in active terminal" command pallet action (which we could map to F8)

[SeeminglyScience]

Wouldn't this make the psm1 run twice?

Also in Windows PowerShell, static class methods have runspace affinity to the last runspace that processed the type definition. This second import will change the runspace that static class methods run in, even when invoked in the origin runspace.

[daxian-dbw]

This affinity issue has been fixed in PowerShell Core. See PowerShell/PowerShell#4209

[SeeminglyScience]

Yeah I did qualify it, I should have been more clear though. That said, afaik there aren't plans to drop support for it anytime soon right?

[daxian-dbw]

Right, there is no plan to port the fix back to Windows PowerShell.

[SeeminglyScience]

What I'm getting at is PSES still has to support Windows PowerShell. So it seems to me that the choices are:

1. Break static methods in Windows PowerShell
2. Maintain both this implementation and the current system
3. Stop supporting Windows PowerShell

Or am I missing something?

[daxian-dbw]

Or, 4. Only apply this optimization to PowerShell Core. On Windows PowerShell, keep calling into the integrated console for intellisense operations

[SeeminglyScience]

Just to clarify, that's what I meant by item 2.

[TylerLeonhardt]

I'm currently now doing 2. If you're using PowerShell Core, you will get this optimization.

[SeeminglyScience]

I've added support for F8... but I wonder... do we even want our own F8 support anymore when we can just use the "Run selected text in active terminal" command pallet action (which we could map to F8)

Yeah we do, PSRL custom key handlers often wreak havoc with bulk input. Plus if we ever want to support injecting support for $PSScriptRoot or breakpoints we'll need to do some trickery with it.

[SeeminglyScience]

I know this is WIP but I have a couple hard stops, unfortunately I think a few of them are not fixable.

[SeeminglyScience]

Completion isn't just done by type, member inference of a property expression is done by value (which would also be an issue for properties with a thread static backing field). This also doesn't account for PSObject's that may have gained additional members.

[daxian-dbw]

Completion isn't just done by type, member inference of a property expression is done by value ... This also doesn't account for PSObject's that may have gained additional members.

True, absolutely valid concerns. Maybe check on the object equality instead of type equality here.

[SeeminglyScience]

Maybe check on the object equality instead of type equality here.

You could do that, but thread static is still a concern.

Another issue to consider is any properties that attempt to marshal the call back to their origin thread, such as CommandInfo.Parameters.

[TylerLeonhardt]

now checking object equality

[SeeminglyScience]

This will only import the psm1, so it will miss nested/required modules, required assemblies, and any command filtering. We also can't actually determine if the module info object was imported via the psm1 directly or via the psd1, which could yield significantly different results.

[daxian-dbw]

This will only import the psm1

Can you elaborate on this? When a module is imported through a manifest, the ModuleInfo.Path points to the psd1 file. For example,

PS:34> $s = gmo Microsoft.PowerShell.Management
PS:35> $s.Path
F:\pscore70\Modules\Microsoft.PowerShell.Management\Microsoft.PowerShell.Management.psd1

[TylerLeonhardt]

@daxian-dbw for some reason... for script modules, the path points to the psm1 :( Patrick is right.

[daxian-dbw]

Ah, I see. The Microsoft.PowerShell.Management is a bad example. The assembly is listed as a NestedModule, so that's literally a manifest module :) If the RootModule or ModuleToProcess is specified, then the path would point to the specified module file I guess.

A bit more logic is needed here to find out if there is a module manifest file.

[TylerLeonhardt]

We can do something similar to:

$psmoduleinfo = Get-Module PowerShellGet
Join-Path $psmoduleinfo.ModuleBase "$($psmoduleinfo.Name).psd1"

[SeeminglyScience]

Problem is you can't tell whether they've imported the psm1 or the psd1.

One could, for instance import a psm1 directly to get intellisense on functions that are typically filtered by the psd1. Conversely if you always assume psm1, you miss out on nested/required modules/assemblies and type/format files.

[daxian-dbw]

For the psd1 vs. psm1 case, we can tell which one was used in loading by inspecting the version. If you are importing a psm1 file, the version would be 0.0. For a psd1, it's very unlikely 0.0.

[TylerLeonhardt]

Alternatively, we could override Import-Module in order to get the exact parameters used for Import-Module and then run the real import-module in each runspace.

[TylerLeonhardt]

I'm now importing the module base if the version is greater than 0.0. overriding Import-Module was more work than it was worth.

[SeeminglyScience]

If the debugger is currently stopped in a different session state (like a break point in a module function), this will throw all of the module's state into the global state for the completion runspace. Also stepping through something like Pester will dump a lot of vars into completion for the rest of the session.

[daxian-dbw]

Good point. Need to try out this in debugging scenario.

[TylerLeonhardt]

@rjmholt and I have brainstormed about this. For starters, we will skip synchronizing while the initial runspace is debugging. The belief here is that your main focus in debugging is reading, not writing. Unfortunately, this means stuff like $TestName and $TestDrive ala Pester will not be available in completions while debugging... but we think it's not critical enough for this PR.

Eventually, it would be nice to get to a point where after you finish debugging, it triggers a "resync -force" which blows away the target runspace and remakes it with a clean sync so we're back to what we would expect after debugging.

[SeeminglyScience]

The belief here is that your main focus in debugging is reading, not writing.

Writing at a breakpoint, along with $Host.EnterNestedPrompt() in general (which will also have the same problem) are the only ways to get intellisense based on a module's session state. I use this extensively.

[TylerLeonhardt]

I've disabled syncing while debugging.

[TylerLeonhardt]

Sorry for the sadness, like I said, eventually I'd like to do the "trigger resync force" - both for debugging and nested prompts but it's not the highest priority for the majority of customers and I 'm not quite sure how I would approach it just yet.

Any ideas?

[SeeminglyScience]

This won't work for argument completers that depend on module state. Also not related to this line but argument completers not loaded by a module won't be transferred either.

[SeeminglyScience]

This will cause some modules to throw. iirc a commonly installed version of the VMWare module does not handle being imported into multiple runspaces well. There's a few modules like that.

[daxian-dbw]

Also not related to this line but argument completers not loaded by a module won't be transferred either.

Can you elaborate the scenario for this one? The assembly that implements a argument completer not getting loaded by the module?

[daxian-dbw]

This will cause some modules to throw. iirc a commonly installed version of the VMWare module does not handle being imported into multiple runspaces well. There's a few modules like that.

This is a valid concern. Normally a module shouldn't be made only work when importing once in a process ... but we need to evaluate how bad it would be (a black list?)

[TylerLeonhardt]

Also not related to this line but argument completers not loaded by a module won't be transferred either

@rjmholt thought of the idea of overriding Register-ArgumentCompleter which will forward the call to the target runspace. That said, perhaps we should also ask ourselves, could running argument completers for Intellisense be a potential security concern and a valid thing to skip in the editor intellisense at least.

[SeeminglyScience]

Can you elaborate the scenario for this one? The assembly that implements a argument completer not getting loaded by the module?

Sorry that was confusing in the context of this line, I mean more generally. Like someone just dot sourcing a script or manually running Register-ArgumentCompleter.

[SeeminglyScience]

That said, perhaps we should also ask ourselves, could running argument completers for Intellisense be a potential security concern and a valid thing to skip in the editor intellisense at least.

Considering they have to run code in order to register an argument completer I don't think it's a security issue. We do automatically import modules during intellisense which could register an argument completer. That might be a security issue, but more because loading a module runs code, not necessarily related to the fact that a completer could be loaded.

[TylerLeonhardt]

I may override this cmdlet. It's not as complicated and high profile as Import-Module

[SeeminglyScience]

I don't think this change is the right path towards a responsive PSES. I believe this will lead to
a significant amount of inconsistencies and additional dead locks that will be more difficult to fix than
the problems we currently have.

Why

PowerShell's thread management

Many PowerShell related objects expect to be used on the same thread that they were created. When
they are used on a separate thread, they typically either throw, or attempt to use the event system
to route itself back to the runspace it was created in. Aside from PowerShell objects themselves,
a surprising amount of third party modules that I've come across expect or otherwise only account for
a single runspace per process.

This is in my opinion the area that represents the most risk. Unfortunately this is also the least
tangable because I believe this will mostly manifest in the way of very difficult to pin down race
conditions.

State based completion

There are too many peices of state that completion relies on that cannot be easily replicated.
Classes and commands not loaded by a module for instance are not currently accounted for. Commands
should be an easy fix, but classes will be much more difficult.

In the same vein we have things that use dynamicparam and argument completers that work based on
module session state. Yes, some of this does work based on variables intialized in the psm1, but
it's also often based on side effects from invocation of other exported commands.

Double invocation of psm1

In general I'm very uncomfortable with the idea of running a users code more times than they expect.
If you made the argument that a psm1 should not have side effects outside of the modules session
state than I'd certainly be inclined to agree. That said, it's not as uncommon as you may think,
especially while in the process of writing it. If I'm quickly sketching out a prototype for a module
in a psm1, I shouldn't have to think about how many times the code is going to be executed without
me explicitly running it.

Changing the session state of completion

A feature request that's come up a few times is intellisense for non-public functions, classes, and
variables for a module. @TylerLeonhardt I'm not sure if you remember but we actually had a pretty
extensive conversation about this awhile back. I told you that a big part of why I put in the effort
to implement $Host.EnterNestedPrompt was for that exact scenario. I mentioned that we could create
a function like this:

function Enter-ModuleScope {
    . (gmo Module) { $Host.EnterNestedPrompt() }
}

And if they ran that it would all just work. I mentioned we could even build in a "Select Project"
feature would work similarly to selecting a csproj in omnisharp. So while of course it is
frustrating to see something I put a lot of effort into thrown away, my sadness comes from this
change solidifying that future as an impossibility.

How

It seems to be the opinion that the problems PSES face are due to the fact that we only have one
runspace to work with. My experience is that it's not what we have but instead how we use it. Behind
every single dead lock, every time completion lags behind, is mismanagement of a thread. Most of the
time this is due to mixing async and sync.

Currently we have a single thread that does the majority of message processing. This is enforced with
a synchronization context that tries to keep all awaited tasks on that thread. In a lot of places
we have the illusion of async but it's not actually using the thread pool. In the few places where
we explicitly run a task on the thread pool with something like Task.Run we often run into more
issues because then we can have that task trying to return to the original thread (we don't use
ConfigureAwait in most places).

My recommendation would be to remove the sync context completely and replace it with three separate
parts.

Message management

A separate thread that is dedicated entirely to reading and writing messages, and dispatching.
No actual dispatch action would be invoked on the messaging thread, but it would be added as a job
in a queue that a separate thread or thread pool would pick up to process. Any processed jobs would
be returned via a separate queue that this thread would read so that it could write the response.

This would also make implementing an async queue significantly easier, as well as cancellations.

Pipeline controller

This thread would be the same one create by the main runspace. The class would be very similar to
the internal Executor in PowerShell. It must never give up the thread for anything that doesn't
explicitly require it. The main difference between ours and PowerShell's would be that ours would
work off a queue similar to as described in "Message management". Whenever it would typically be idle
it would need to block until there was a request (i.e. it would use BlockingCollection). Any time
a dispatch described above would need access to the pipeline thread it would need to create a job
that this thread would process.

Dispatch pool

This pool would be where the majority of analysis is done. Any parsing, AST analysis, file enumeration,
etc would be completed by this pool.

If a task requires access to the pipeline thread then it would queue the request for the pipeline
controller in the smallest form it can. For instance if a message needed to infer the type of a
statement, it would attempt to statically determine the type on it's current thread, and then if it
could not it might queue a single variable look up for the pipeline controller.

Jobs created by the message manager would be picked up by threads in this thread pool. Once completed
(or cancelled), they would send the result back to the message manager.

[rkeithhill]

mixing async and sync ... illusion of async but it's not actually using the thread pool

Agreed. Maybe it's just me but the current design can be really hard to reason about. It does seem like at the very least a re-design of the message handling code is needed (probably more as you've scoped out). The fact that PSES just gets further and further behind processing requests that VSCode doesn't need anymore is a bit of a killer. We need to be able to process cancelRequests and avoid starting unnecessary work.

[rjmholt]

@TylerLeonhardt, @daxian-dbw and I got a chance to discuss this at length yesterday. This is one of those tricky areas because the ideal architecture of a language server collides with PowerShell's assumed interactive experience in bad ways.

The idea for separating services out into different runspaces came about because of the native readline problem, which we've been chewing on for a while as to how to fix.

Considering the way PSScriptAnalyzer runs in another runspace (in part so it doesn't block anything), it seems like a valid experiment to offload completions as well, especially since they represent a core experience that users feel the most pain with. We actually discussed offloading them into another process first (and using Enter-PSHostProcess to get them back, and let you even hook up other PowerShell terminals), but @daxian-dbw noted that would be a much bigger change architecturally and possibly a lot of work to bring completions back to their existing experience.

Our main motivating factors for a direction to take are:

• Move past existing issues:
  • The native readline timeout issue on *nix, which has been the hard blocker for us bringing PSES 2.0 out of preview
  • Idiosyncracies with pasting/running script content in the console (like with F8)
  • Threading issues, both with PowerShell-derived objects and with other libraries that have their own thread handling logic
• Simplify existing logic for maintainability
  • PSES is currently just too complex to keep features working well, to keep up with VSCode and to add new demanded features all at the same time
  • Thread management is a cross-cutting concern that could silently affect (and be affected by) any code on the pipeline thread, and we want to minimise our need to implement or maintain that (as well as for performance reasons)
• Allow code reuse as much as possible (in part to reduce the amount of code in PSES), particularly looking at the Omnisharp LSP base to replace a lot of PSES's low level logic, and this library would limit our ability to do fine-grained thread control; it uses a lot of dynamically registered async handlers, in the vein of ASP.NET Core, and weaving a custom thread model through that as it changes would probably be impossible. That work would also buy us async message handling and cancellation logic and a few other things essentially for free.

Moving completions to a different runspace from console execution will buy us these things because it returns the integrated console to being almost entirely an ordinary PowerShell console, the divergence from which has been the nexus of the majority of our important and hard-to-fix issues.

However, the downsides you raise @SeeminglyScience are definitely legitimate and I think worth going through:

Issue	Proposed resolution
PS objects on multiple threads	The completion runspace will only be used for completions rather than to run user code. TabComplete{2} and registered argument completers will run here, but the vast majority of those functions will compute completions based on AST analysis and local computations. There will be examples where things break, but I think those will be rare enough that we can basically help to fix the issue in the module code.
State-dependent completions	Similar to the above, there probably aren't many modules/commands that depend on previous execution state in a non-idempotent manner. Something like Stop-Service depends on state but uses out-of-proc information, other commands that build a cache do so internally. There will be commands whose completions depend on some previously executed command, but our suspicion is that there won't be many and that we can again do specific work to address those cases.
Double module import	We discussed this a bit. PSScriptAnalyzer actually imports modules in some rules and so we already have this problem to some extent. The VMWare module that fails is the classic case. Without a double import, we don't get proper completions for dynamic parameters or argument completers in the completion runspace, so I think the best solution is to create an (editable) exclusion list for modules that don't re-import idempotently and instead create dummy modules for completions on them.
Module-private completions	Reading through, it looks like this should still be possible by invoking the completer on the completion thread in the module's scope. All of us were confident we could get most of the functionality around this working with the proposed change.
Static class method runspace affinity	In this case, it's likely there's no workaround. The runspace affinity of static class methods is specific to Windows PowerShell and is considered a bug. It's been fixed in PS Core. Discussing this, we felt that PS 6+ fixes this and static methods on PowerShell classes are uncommon enough that it's tolerable.

Although we could also try to move to a better-informed thread architecture to manage the pipeline thread better, there are issues with that:

• Better pipeline thread management won't solve the readline issues.
• Interlocked thread contention logic will require work to build, maintain and continue to understand. The problem with the existing thread logic is that it was created naively and then the understanding lost. The distinction from the second runspace model is that it will involve writing a lot more code, of the sort that will require expertise to write correctly and then to maintain. We don't think we have the resources to write or maintain that thread-oriented code, especially versus other work in the extension.

Ultimately there's no way to solve the problem perfectly here; PowerShell is the only programming language I've used that has a synchronous completion ecosystem, and it doesn't work well with the asynchronous IDE ecosystem. But forced to weigh up the integrated console not working like PowerShell against breaking completion experiences, I think making the console work comes first and then we can get completions working again; we might not be able to make PowerShell's existing completion logic hook in perfectly, but most of it will work and we can make sure we provide the right mechanisms for the rest.

Eventually I think we have to confront that PowerShell's native completion experience isn't always what we want for our IDE experience (we've closed numerous completion bugs/requests as wontfix because they come from PowerShell, we can only provide completions for the executing PowerShell environment) and that IDEs everywhere are confounding PowerShell's native completion model.

So with all that said, we want to give this a try and get user feedback on it. We think this will solve some otherwise insoluble problems, buy us a few other things along the way, open up some possibilities to reduce our technical debt, and still offer possibilities for yak-shaving on some of the foreseeable problems it might cause. Once we've got some usage, if the issues turn out to be too much we can go back to the drawing board.

Thanks for your really detailed write-up @SeeminglyScience ‐ we really value it and it's important, especially for a problem that has no obvious solution and where any resolution is likely to be contentious. Ideally that information will mean we can (1) identify and work around areas where issues might arise and (2) fail fast if we determine this is not going to work.

/cc @SteveL-MSFT @SydneyhSmith @joeyaiello for awareness

[SeeminglyScience]

@rjmholt Thank you for your detailed response.

I'd like to mention before continuing that I understand that a lot of what I'm going to quote is simply explaining the motivation behind pursuing this avenue. While I will be providing counter points, I want to make it clear that I fully understand the motivation and indeed the appeal of the idea. I in no way intend to disparage anyone who worked on it.

Considering the way PSScriptAnalyzer runs in another runspace (in part so it doesn't block anything), it seems like a valid experiment to offload completions as well, especially since they represent a core experience that users feel the most pain with.

I can see where you're coming from, but it only makes sense with PSSA because it's meant to be static. Yes it's true that some things do indeed use state like command lookup, but it creates the state from scratch. It's not trying to mimic an existing active runspace filled with objects that will be in use.

Our main motivating factors for a direction to take are:

• Move past existing issues:

  • The native readline timeout issue on *nix, which has been the hard blocker for us bringing PSES 2.0 out of preview
  • Idiosyncracies with pasting/running script content in the console (like with F8)

I'm not following you here, neither of these are threading related. I've explained this before but the former is due to a flaw in our logic where we're not resetting termios attributes correctly before invoking a native command. During every ReadKeyAsync we set the native timeout time, console echo and a few other attributes. It's set back correctly when PSRL isn't present but something about how it's invoked when we inject it into PSRL doesn't work the same. Really, I didn't get to test it on non-Windows. I wouldn't be surprised if PSRL's ReadKey thread just wasn't getting cancelled before we we invoked the native command. It's probably a relatively minor logic flaw.

With the latter I'm not 100% sure what you mean, but if it's a *nix specific issue then it's likely related to the above. If it's not related to the above, then it's a flaw in the really hacky async version of ReadKey that I had to do for non-Windows ages ago.

Are these issues no longer present with the changes in this PR?

• Simplify existing logic for maintainability

  • PSES is currently just too complex to keep features working well, to keep up with VSCode and to add new demanded features all at the same time

From my perspective this accomplishes the opposite of what was intended here. The whole process of trying to clone all of the state in a runspace seems a lot harder to understand and debug than managing access to one of them. Especially once you start having to worry about proxy commands.

to keep up with VSCode and to add new demanded features all at the same time

Again I know this is explaining original motivation but this PR breaks features we already have. Also while I agree that fixing our thread management would make it easier to add new features, I believe if it's done this way then we will encounter more things that have become impossible to do.

• Thread management is a cross-cutting concern that could silently affect (and be affected by) any code on the pipeline thread, and we want to minimise our need to implement or maintain that (as well as for performance reasons)

My concern here is that by lifting objects whole cloth from one thread to another we will be increasing the probability of this and reducing how easy it is to troubleshoot.

Right now if anything like that is accessed on another thread, it's done on a thread without a default runspace. That means in most instances it's either going to silently do nothing or throw. I don't recall any instances where we do that though.

Conversely, PowerShell often reacts differently when there is a default runspace for the current thread. In a lot more situations it tries to send it back. In some instances this may manifest as additional dead locks, in others it may go as far as corrupting state of something currently running. Due to PowerShell using the PSEventManager to accomplish this, it's possible that it could even happen between sequence points. While dead locks are pretty obvious, with the latter it will be very challenging for the user to determine that there was even an issue on our end, much less report something reproducible.

• Allow code reuse as much as possible (in part to reduce the amount of code in PSES), particularly looking at the Omnisharp LSP base to replace a lot of PSES's low level logic, and this library would limit our ability to do fine-grained thread control; it uses a lot of dynamically registered async handlers, in the vein of ASP.NET Core, and weaving a custom thread model through that as it changes would probably be impossible. That work would also buy us async message handling and cancellation logic and a few other things essentially for free.

That's an excellent idea and I agree 100%. I believe this fits very well with what I proposed. All we would need to do is start up the pipeline thread and the controller for it. Omnisharp's LSP base would run where ever it wants to, the pipeline thread wouldn't be on the thread pool so I doubt it would conflict at all. Queuing a job for the pipeline thread could be done from any of the registered async handlers, that whole process could easily be abstracted away. At the end of the day, the pipeline thread controller just looks like a normal synchronous class that's taking objects that represent tasks from a collection. All the handlers would need to know how to do is add to that collection, and that could all be abstracted into a few methods.

I realize that I'm over simplifying it a bit but I believe it is well within the realm of feasiblity.

Moving completions to a different runspace from console execution will buy us these things because it returns the integrated console to being almost entirely an ordinary PowerShell console, the divergence from which has been the nexus of the majority of our important and hard-to-fix issues.

I'm not going to try to say that properly managing the pipeline is easy, it's not and acknowledge that. I don't agree that that challenge the nexus of our problems though. We have an architecture that was built piecemeal, mostly by one individual (as fantastic as he is, it's still a whole lot). As a result there's a lot of choices that don't make sense for the product the way it is today, and a whole lot of simple human error related bugs.

However, the downsides you raise @SeeminglyScience are definitely legitimate and I think worth going through:

Issue Proposed resolution
PS objects on multiple threads The completion runspace will only be used for completions rather than to run user code. TabComplete{2} and registered argument completers will run here, but the vast majority of those functions will compute completions based on AST analysis and local computations.

I know at the very least dynamic parameters and type inference of properties will also evaluate during completions. Those have a higher probability of causing inconsistency.

There may also be more, I have not gone looking through all of completion and type inference analysis, but it wouldn't surprise me if there was more.

There will be examples where things break, but I think those will be rare enough that we can basically help to fix the issue in the module code.

In this one I'm not talking about a badly written module. Here's the series of events:

1. We copy an object to a different thread than it was created
2. We do something on with that object (even if indirectly through completion analysis) that causes code evaluation in some way, like for instance call a properties get accessor.
3. That object realizes it's on the wrong thread and interrupts or otherwise corrupts state of something else currently running in real runspace

State-dependent completions Similar to the above, there probably aren't many modules/commands that depend on previous execution state in a non-idempotent manner. Something like Stop-Service depends on state but uses out-of-proc information, other commands that build a cache do so internally. There will be commands whose completions depend on some previously executed command, but our suspicion is that there won't be many

It's more common with third party modules, especially those that require a connection or credentials to be established.

and that we can again do specific work to address those cases.

I'm having a hard time seeing how. Are we going to special case every instance we come across?

Double module import We discussed this a bit. PSScriptAnalyzer actually imports modules in some rules and so we already have this problem to some extent.

Not in the default rules. To be fair, if I was more involved in that project when that was being discussed I probably would have argued against it there as well, but at least you have to explicitly enable it.

The VMWare module that fails is the classic case.

Yeah that's the most well known one but I think it's more common than you'd think. I didn't realize until recently that one of my modules breaks on import in a second runspace. It's pretty easy to overlook doing something statically in a problematic way, even for someone like me who is hyper aware of the issues it can cause. Realistically, no one tests for it.

Without a double import, we don't get proper completions for dynamic parameters or argument completers in the completion runspace, so I think the best solution is to create an (editable) exclusion list for modules that don't re-import idempotently and instead create dummy modules for completions on them.

Based on the small list of issues I was able to come up with, the complexity of this change seems to be increasing exponentially. There is a significant amount of special casing and effort in general being put in to simply approach the same level of functionality we have today.

Although we could also try to move to a better-informed thread architecture to manage the pipeline thread better, there are issues with that:

• Better pipeline thread management won't solve the readline issues.

If this would solve the readline issues then so would better pipeline thread management, though it would be surprising to me if either did.

• Interlocked thread contention logic will require work to build, maintain and continue to understand. The problem with the existing thread logic is that it was created naively and then the understanding lost. The distinction from the second runspace model is that it will involve writing a lot more code, of the sort that will require expertise to write correctly and then to maintain. We don't think we have the resources to write or maintain that thread-oriented code, especially versus other work in the extension.

That makes sense, and I appreciate the honesty. I do think you're overestimating the expertise required, or underestimating the expertise that will be required for this change. Based on the couple of issues I've thought up off the top of my head we're already creating proxy commands and even entire dummy modules. Plus whoever is going to be troubleshooting all of the thread-specific object related issues is going to need to know a thing or two about threading and obscure internals of PowerShell.

If the PowerShell team doesn't have the resources to dedicate towards a rewrite of threading, then maybe work on fixing some of individual crashes and dead locks. Maybe just focus on fixing PSRL *nix issues (which I'm pretty confident aren't really related to our threading model) so we can finally get 2.0 out the door.

Eventually I think we have to confront that PowerShell's native completion experience isn't always what we want for our IDE experience (we've closed numerous completion bugs/requests as wontfix because they come from PowerShell, we can only provide completions for the executing PowerShell environment) and that IDEs everywhere are confounding PowerShell's native completion model.

I'm probably arguing semantics here but I'd consider that "Resolution-External" more than I would "wontfix". They are typically added as issues to PowerShell, though yes, completion enhancements are often left to the community to implement. Even so there has been a not insignificant amount of community contributions to completion, some even specifically targeting the scenario of an editor. For instance this incredible enhancement contributed by @powercode.

So with all that said, we want to give this a try and get user feedback on it. We think this will solve some otherwise insoluble problems, buy us a few other things along the way, open up some possibilities to reduce our technical debt, and still offer possibilities for yak-shaving on some of the foreseeable problems it might cause. Once we've got some usage, if the issues turn out to be too much we can go back to the drawing board.

There's a few issues that I've brought up that have been deemed acceptable, and I disagree with that assessment. I believe this change is significantly more complicated than a proper fix and due to the nature of the fix any testing period would need to be pretty lengthy. I'd likely be more receptive to such an experiment if we weren't already in the middle of an incredibly long preview release.

At the end of the day, if the decision is made to continue with this regardless, then it is what it is. If you run into issues or have questions I will of course continue to be available, but I can't approve this PR.

[TylerLeonhardt]

I’m going to close this.

Since .NET Core did a huge refactor to the Console APIs in .NET Core 3.0, PowerShell 7 with PSRL in PSES no longer has the native prompt canceling issue because one thing they fixed was what child processes use for termios settings. Since macOS & Linux users are small compared to Windows, and they are more likely to update their version of PowerShell, we will not attempt to get 6.1 & 6.2 working with the prompt issue. Instead, we will recommend moving to PS 7 which will most likely be GA before 2.0 of the PowerShell extension is GA.

Since this PR was opened to help fix that in addition to dealing with threading issues, we’ve lost half the weight of it’s usefulness (which is a good thing!) so I’m going to close it.

The path forward will be moving to Omnisharp’s LSP implementation which we think will give us a solid foundation of how we should handle messages. From that, we still need to figure out how messages will properly handle a single runspace.

What’s unclear at the moment is what we will use for the debug protocol. More investigation needs to be done.

[TylerLeonhardt]

Big shout out to @rjmholt for finding the CoreFX changes!

[rjmholt]

BTW, here's where they changed System.Diagnostics.Process.Start() that fixed us in PS 7.

One question is: do we need to make any changes to UnixConsoleEcho or our use of it to integrate with these changes better? In particular, there are some thread safety things addressed in the new corefx code that weren't there before.

[SeeminglyScience]

@rjmholt

This changes to echoing while there are interactive applications and not echoing
when there are none.

We might be able to drop UnixConsoleEcho altogether 😮

Lets hope 🤞

[rjmholt]

We might be able to drop UnixConsoleEcho altogether

Yeah that's what we were hoping too, but basically I want to understand more about the purpose of UCE in the first place, since it predates the PSReadLine integration; wasn't it also used in the old readline to address a particular issue with Up and Left keys?

[SeeminglyScience]

We might be able to drop UnixConsoleEcho altogether

Yeah that's what we were hoping too, but basically I want to understand more about the purpose of UCE in the first place, since it predates the PSReadLine integration; wasn't it also used in the old readline to address a particular issue with Up and Left keys?

So for a long time debugging was mostly broken on *nix because you can't throw ReadKey into another thread and use Console.CursorLeft/Top. If you tried to call either of those when ReadKey was pending, the calls will block until ReadKey returns.

To get around that we wrote the less than ideal implementation of ReadKeyAsync that checks KeyAvailable until it returns true. That worked, but since a read wasn't actually taking place, echo was still turned on. With no API to disable echo other than Console.ReadKey(true), I had to rip the native code that corefx uses out and into it's own library for PSES to consume.

So initially, it's sole purpose was to disable echo. That said, it's entirely possible that the other attributes we're setting may still be required to enable KeyAvailable to trigger when we expect it to.

Best way to test it is to just remove the calls to UnixConsoleEcho, and see if there are any special keys that don't seem to take affect right away. Backspace is a good test I believe.

[TylerLeonhardt]

This sounds like fun. I can maybe give it a go tomorrow.