@vexx32, your last commit had 40 failures in PowerShell-CI-macos
(These are 5 of the failures)

try/catch/finally statement parsing.Error Statement expected: <<try {1} catch [int],[char] {2} catch>>.$asts[6].Extent.Text

Expected strings to be the same, but they were different.
Expected length: 3
Actual length:   5
Strings differ at index 0.
Expected: '{2}'
But was:  '[int]'
at <ScriptBlock>, /Users/vsts/agent/2.153.1/work/1/s/test/tools/Modules/HelpersLanguage/HelpersLanguage.psm1: line 125
125:             It "`$asts[$($i + 1)].Extent.Text" { $asts[$i + 1].Extent.Text | Should Be $a[$i] }

try/catch/finally statement parsing.Error Statement expected: <<try {1} catch [int],[char] {2} catch>>.$asts[7].Extent.Text

Expected strings to be the same, but they were different.
Expected length: 1
Actual length:   6
Strings differ at index 0.
Expected: '2'
But was:  '[char]'
at <ScriptBlock>, /Users/vsts/agent/2.153.1/work/1/s/test/tools/Modules/HelpersLanguage/HelpersLanguage.psm1: line 125
125:             It "`$asts[$($i + 1)].Extent.Text" { $asts[$i + 1].Extent.Text | Should Be $a[$i] }

try/catch/finally statement parsing.Error Statement expected: <<try {1} catch [int],[char] {2} catch>>.$asts[8].Extent.Text

Expected strings to be the same, but they were different.
Expected length: 1
Actual length:   3
Strings differ at index 0.
Expected: '2'
But was:  '{2}'
at <ScriptBlock>, /Users/vsts/agent/2.153.1/work/1/s/test/tools/Modules/HelpersLanguage/HelpersLanguage.psm1: line 125
125:             It "`$asts[$($i + 1)].Extent.Text" { $asts[$i + 1].Extent.Text | Should Be $a[$i] }

try/catch/finally statement parsing.Error Statement expected: <<try {1} catch [int],[char] {2} catch>>.$asts[10].Extent.Text

Expected strings to be the same, but they were different.
Expected length: 5
Actual length:   1
Strings differ at index 0.
Expected: '[int]'
But was:  '2'
at <ScriptBlock>, /Users/vsts/agent/2.153.1/work/1/s/test/tools/Modules/HelpersLanguage/HelpersLanguage.psm1: line 125
125:             It "`$asts[$($i + 1)].Extent.Text" { $asts[$i + 1].Extent.Text | Should Be $a[$i] }

try/catch/finally statement parsing.Error Statement expected: <<try {1} catch [int],[char] {2} catch>>.$asts[11].Extent.Text

Expected strings to be the same, but they were different.
Expected length: 6
Actual length:   1
Strings differ at index 0.
Expected: '[char]'
But was:  '2'
at <ScriptBlock>, /Users/vsts/agent/2.153.1/work/1/s/test/tools/Modules/HelpersLanguage/HelpersLanguage.psm1: line 125
125:             It "`$asts[$($i + 1)].Extent.Text" { $asts[$i + 1].Extent.Text | Should Be $a[$i] }

@vexx32, your last commit had 40 failures in PowerShell-CI-linux
(These are 5 of the failures)

try/catch/finally statement parsing.Error Statement expected: <<try {1} catch [int],[char] {2} catch>>.$asts[6].Extent.Text

Expected strings to be the same, but they were different.
Expected length: 3
Actual length:   5
Strings differ at index 0.
Expected: '{2}'
But was:  '[int]'
at <ScriptBlock>, /home/vsts/work/1/s/test/tools/Modules/HelpersLanguage/HelpersLanguage.psm1: line 125
125:             It "`$asts[$($i + 1)].Extent.Text" { $asts[$i + 1].Extent.Text | Should Be $a[$i] }

try/catch/finally statement parsing.Error Statement expected: <<try {1} catch [int],[char] {2} catch>>.$asts[7].Extent.Text

Expected strings to be the same, but they were different.
Expected length: 1
Actual length:   6
Strings differ at index 0.
Expected: '2'
But was:  '[char]'
at <ScriptBlock>, /home/vsts/work/1/s/test/tools/Modules/HelpersLanguage/HelpersLanguage.psm1: line 125
125:             It "`$asts[$($i + 1)].Extent.Text" { $asts[$i + 1].Extent.Text | Should Be $a[$i] }

try/catch/finally statement parsing.Error Statement expected: <<try {1} catch [int],[char] {2} catch>>.$asts[8].Extent.Text

Expected strings to be the same, but they were different.
Expected length: 1
Actual length:   3
Strings differ at index 0.
Expected: '2'
But was:  '{2}'
at <ScriptBlock>, /home/vsts/work/1/s/test/tools/Modules/HelpersLanguage/HelpersLanguage.psm1: line 125
125:             It "`$asts[$($i + 1)].Extent.Text" { $asts[$i + 1].Extent.Text | Should Be $a[$i] }

try/catch/finally statement parsing.Error Statement expected: <<try {1} catch [int],[char] {2} catch>>.$asts[10].Extent.Text

Expected strings to be the same, but they were different.
Expected length: 5
Actual length:   1
Strings differ at index 0.
Expected: '[int]'
But was:  '2'
at <ScriptBlock>, /home/vsts/work/1/s/test/tools/Modules/HelpersLanguage/HelpersLanguage.psm1: line 125
125:             It "`$asts[$($i + 1)].Extent.Text" { $asts[$i + 1].Extent.Text | Should Be $a[$i] }

try/catch/finally statement parsing.Error Statement expected: <<try {1} catch [int],[char] {2} catch>>.$asts[11].Extent.Text

Expected strings to be the same, but they were different.
Expected length: 6
Actual length:   1
Strings differ at index 0.
Expected: '[char]'
But was:  '2'
at <ScriptBlock>, /home/vsts/work/1/s/test/tools/Modules/HelpersLanguage/HelpersLanguage.psm1: line 125
125:             It "`$asts[$($i + 1)].Extent.Text" { $asts[$i + 1].Extent.Text | Should Be $a[$i] }

As this is a change to the core language it should go through the RFC process.

@BrucePay please see PowerShell/PowerShell-RFC#207 😄

Rebased onto #14297 to get CI passing

Thanks, Rob! 💖

Hmm, that failure's a new one. I'm pretty sure I haven't touched anything that would affect that test. Retriggering CI to verify failure, I don't have a Linux box to test on atm really.

Hmm, that failure's a new one.

Well CodeFactor likes to include changes outside the actual diff (just in the same file). It's not a blocker for me

Yeah, I'm not too worried about CodeFactor here, this PR'd be twice the size if I did 😂

There was a transient failure on the Linux build, something about the logging picking up something odd. I'm gonna say it's not something I did given it disappeared when I got it to rebuild. 🙂 All looks good now, ty for the help! 💖

However, I have run this branch now, with the following script:

function Test-Cleanup
{
  cleanup
  {
    Write-Host "Enter message:"
    $msg = Read-Host
    Write-Host "Cleanup: $msg"
  }
}

Test-Cleanup

The Read-Host is pretty artificial, but is an easy way to trigger a pipeline stop (by using Ctrl+C). Basically I wanted to see what happens when we put the lock logic under some stress.

When we do that, I've seen a couple of things happen:

• The ReadLine call is cancelled without the pipeline being stopped and PSReadLine then gets caught in a double prompt state between the host PowerShell and the dev PowerShell (in my case):

  

  At this point, it's actually possible to interact with processes alternately; every second keystroke goes to the host process, so you can get two results for $PID in the same session

• More importantly, the consolehost thread races the pipeline thread for the monitor, so the pipeline thread is able to re-enter the critical section before the consolehost is able to stop it. I'm not yet sure what the consequences are beyond general pipeline thread state corruption, but I would characterise it as unwanted behaviour.

  

I think this issue is solvable, but I want to spend a little bit more time working on the solution to present one to you that I like. In particular some questions are:

• How do we prevent a deadlock between the pipeline stopping and the disposal if the cleanup block tries to wait for the pipeline to stop? Does that even make sense as a scenario, or is the escape hatch higher level than that and we just need to account for it in the disposal logic (so that we can dispose and then stop)?
• If a pipeline is stopped mid-disposal, how do we communicate that stopping failed? Or again, do we complete the cleanup block and then stop? In particular, if the pipeline is stopped from PSReadLine, how do we block the caller properly on the stop so that we don't simply proceed through the cleanup as if the stop worked?

I suspect the answer to this might simply be to force the stopper to hold the lock, but that makes stopping harder. I need to think about what the actual scenarios are and whether there's more cleanup state we need to take care of.

Interesting!

Okay so... I -thought- that I'd already accounted for that case, but it looks like the case where a user presses Ctrl+C during normal operation of cleanup{} is a bit different to hitting Ctrl+C on something that is actively asking the user for input. Very interesting, nice find! (Zero idea if there's a way to actually put together a test for that specific case though!)

I think stopping the pipeline within cleanup{} should be essentially impossible, short of terminating pwsh entirely. If something within the cleanup block triggers that in some way, I think we have two options (I prefer the first, personally):

1. It is ignored, because the purpose of cleanup{} is to circumvent pipeline stops to ensure critical code paths can be followed.
2. It is treated as a "normal" exception that occurs within cleanup{} and the remainder of the current cleanup block is terminated.

In this specific case where it is explicitly cancelling a specific command because the Ctrl+C is received from this input query directly (which I think is kind of an unusual edge case; as far as I've been able to find, any other attempt to Ctrl+C during operation of the dispose block is successfully basically ignored until the cleanup{} script completes) I think it makes sense to treat it as an exception thrown from the Read-Host command itself and terminate the rest of the block.... maybe? 😅

It is ignored, because the purpose of cleanup{} is to circumvent pipeline stops to ensure critical code paths can be followed.

I'd say this is most consistent with the general scenario where this will occur, which is where a hosted PowerShell runtime will have Stop() called on it from another thread. The cleanup block could be doing anything at that point, so throwing an exception could be impossible or could do anything, which is basically what we're trying to avoid.

So I think it makes sense to block the Stop() call on the cleanup block completing. The issue is then that if the cleanup block itself is triggering the Stop() call, we have a deadlock we need to resolve somehow. It might be possible for us to resolve that with a check and then throwing an exception -- that way we don't need to consider an API change...

Yeah, I'd agree there. I'm not sure of the best way to check that, though... Perhaps setting a value on the pipeline stopper which indicates which thread/runspace "holds" the critical section, and if Stop() is called by that same runspace/thread it either returns (no-op, allowing the cleanup{} block to continue?) or throws an exception (which frankly I'm not entirely sure how things further up the stack would handle... but is probably OK?)

Hm. 🤔

I guess I could try to check that the same thread or runspace that's currently running cleanup{} isn't calling Stop(), but I'm not sure how I'd get at that. Any ideas? 😕

If a pipeline is stopped mid-disposal, how do we communicate that stopping failed? Or again, do we complete the cleanup block and then stop? In particular, if the pipeline is stopped from PSReadLine, how do we block the caller properly on the stop so that we don't simply proceed through the cleanup as if the stop worked?

This part I'm less sure about. My inclination is that if the cleanup script itself errors out normally, we should just emit that error (and currently, we do). For a straight up PipelineStoppedException as it hits in the case you raised, I'm wondering if perhaps a silent termination of that block is okay... it's an edge case that I'm not sure how else folks might run into, but in the majority of cases it shouldn't be possible to even hit that.

Perhaps at most we could rewrap the PipelineStoppedException with another exception that indicates that a stop was explicitly demanded / unavoidable during the cleanup{} operation? I can't think of any other way to handle it at the moment.

How do we prevent a deadlock between the pipeline stopping and the disposal if the cleanup block tries to wait for the pipeline to stop? Does that even make sense as a scenario, or is the escape hatch higher level than that and we just need to account for it in the disposal logic (so that we can dispose and then stop)?

So there's two types of stops for the most part:

1. A stop triggered via the pipeline stopper (e.g. PowerShell.Stop or similar)
2. Inline stop triggered via a PipelineStoppedException throw within pipeline invocation

During the execution of the cleanup block itself, there should only ever be the latter. It would be pretty hard for the cleanup block to actually get an object to trigger the former. That said, in the event that it does, the lock will already be taken on that thread so it should just abort the cleanup block normally.

if the pipeline is stopped from PSReadLine, how do we block the caller properly on the stop so that we don't simply proceed through the cleanup as if the stop worked?

So if PSRL tries to stop the pipeline mid cleanup, this series of calls will happen (roughly):

1. PowerShell.Stop
2. LocalPipeline.Stop
3. PipelineStopper.AwaitPipelineCriticalSection
4. Block until the cleanup block exits and calls PipelineStopper.ExitCriticalSection
5. Continue with stop

I can't think of a scenario where PSRL will be on the pipeline thread, while a cleanup block is running, and want to stop the pipeline. But if there is one it should work like the my first paragraph.

Open PRs should not be assigned to milestone, so they are not assigned to the wrong milestone after they are merged. For backport consideration, use a backport label.

Also, @rjmholt:

At this point, it's actually possible to interact with processes alternately; every second keystroke goes to the host process, so you can get two results for $PID in the same session

FWIW I'm seeing this same behaviour from a dev build on the master branch as well; just call Read-Host and Ctrl+C out of it from a running dev build on the master branch.

Is this likely to make it into 7.2?

That's the hope. Depends if we get the final kinks ironed out and reviews approved, I suppose. 🙂

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@daxian-dbw @rjmholt @SteveL-MSFT please let me know if more is needed. I've responded to all concerns raised as best as I can. 🙂