Fix debugging output location

[TylerLeonhardt]

This fixes a lot of the prompt/output issues that we've been seeing.

fixes PowerShell/vscode-powershell#2372
fixes PowerShell/vscode-powershell#2407
fixes PowerShell/vscode-powershell#2408

• It removes a useless this.WriteOutput when instead ExecuteCommandAsync could just do it
• The WriteOutput in the finally block now only runs in the legacy readline experience

There's still a weird thing happening with prompts on F5... but that existed before this change so I'll try to address that in another PR.

[TylerLeonhardt]

Big thanks for @daxian-dbw for helping me out!

[SeeminglyScience]

I'd recommend manually testing as many scenarios as you can with this one. Aborting PSRL without actually stopping it in the host could lead to a race condition where PSRL starts again before this thread actually acquires a runspace handle

[daxian-dbw]

Aborting PSRL without actually stopping it in the host could lead to a race condition where PSRL starts again before this thread actually acquires a runspace handle

@SeeminglyScience calling AbortReadLine will cause PSRL to return from ReadLine(...), and it won't start until [PSConsoleReadLine]::ReadLine being called again in InvokeReadLineAsync. Are you saying that InvokeReadLineAsync could be called again in parallel after calling AbortReadLine here?

[SeeminglyScience]

Are you saying that InvokeReadLineAsync could be called again in parallel after calling AbortReadLine here?

Unfortunately yeah :/

Here's the relevant bits in the host. Those methods are called in the ExecutionStatusChanged event handlers to control whether ReadLine is currently running. Without stopping those, it'll queue up another invocation in a different thread.

[rjmholt]

Unfortunately yeah :/

Can we just lock it out? I know that means needing to acquire another lock for every prompt, but that's the cost of parallelism.

All we need to know is where we should yield the lock. I assume that's after the WriteOutput call

[SeeminglyScience]

Can we just lock it out?

It's not impossible, but with how it's set up it would be challenging. You'd have better luck moving the writeInputToHost check after the ExecutionStatusChanged event has been fired. That'd be the easiest fix, but would still require a little bit of architecturing since that event is fired in the inner most ExecuteCommand call (which doesn't currently have visibility into that parameter).

[TylerLeonhardt]

That last commit gets rid of:

• A random newline that appears when you F5
• A newline when the terminal buffer scrolls

tyleonha ~/Code/PowerShell/Misc 
❯❯❯ /Users/tyleonha/Code/PowerShell/Misc/foo.ps1
1
2
3
PS> /Users/tyleonha/Code/PowerShell/Misc/foo.ps1
1
2
3
PS> /Users/tyleonha/Code/PowerShell/Misc/foo.ps1
1
2
3
PS> /Users/tyleonha/Code/PowerShell/Misc/foo.ps1
1
2
3
PS> /Users/tyleonha/Code/PowerShell/Misc/foo.ps1
1
2
3
PS> 

I'm not sure where my prompt goes though...

[TylerLeonhardt]

If both WriteOutput and Starting the REPL relied on having lock access to the runspace, could that work, @SeeminglyScience?

[SeeminglyScience]

Just add a new internal property to ExecutionOptions as internal string CommandText { get; set; }. Then in this check:

PowerShellEditorServices/src/PowerShellEditorServices/Services/PowerShellContext/PowerShellContextService.cs

Lines 681 to 684 in 3b890c3

	if (executionOptions.WriteInputToHost)
	{
	this.WriteOutput(psCommand.Commands[0].CommandText, true);
	}

Change it to:

if (executionOptions.WriteInputToHost)
{
    this.WriteOutput(
        executionOptions.CommandText ?? psCommand.Commands[0].CommandText,
        true);
}

[TylerLeonhardt]

Ok I think I'm crazy... but we don't need the this.WriteOutput at all and should instead rely on WriteInputToHost = true, instead?

[SeeminglyScience]

Ok I think I'm crazy... but we don't need the this.WriteOutput at all and should instead rely on WriteInputToHost = true, instead?

Yessir that's right

[TylerLeonhardt]

Ok, I think this is ready for a proper review @SeeminglyScience

[SeeminglyScience]

LGTM! with one nit