Add TerminateStragglers option (Windows)

[jazzdelightsme]

PR Summary

Addresses #3745

There is a lot more detail about the problem this PR solves in the code comments and the linked Issue. In short, this PR:

• Adds a new "TerminateStragglers" option.
• When enabled, existing console-attached PIDs are gathered into an "allow" list (on Windows only).
• When the shell informs us that it is time to wait for input, right before we disable ctrl+c signals, (if enabled) we ensure that the shell will not be broken by other console-attached children that may also be attempting to read input.

How tested: added printfs and manually triggered the problem scenario, to verify that the code works as expected.

There are a few open questions; I will leave some comments in the PR (for example: if someone on non-Windows enables the option, should we write a warning, or just smile and accept it?).

PR Checklist

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Make sure you've added one or more new tests
• Make sure you've tested these changes in terminals that PowerShell is commonly used in (i.e. conhost.exe, Windows Terminal, Visual Studio Code Integrated Terminal, etc.)
• User-facing changes
  • Not Applicable
  • OR
  • Documentation needed at PowerShell-Docs
    • Doc Issue filed: Option -TerminateOrphanedConsoleApps was added to PSReadLine v2.3.2-beta2 MicrosoftDocs/PowerShell-Docs#10346

Microsoft Reviewers: codeflow:open?pullrequest=https://github.com/PowerShell/PSReadLine/pull/3764&drop=dogfoodAlpha

[daxian-dbw]

@jazzdelightsme Sorry for the late review. It looks good overall. Left some comments.

[daxian-dbw]

I need to build the changes and see what it does when PowerShell is working with child process with stdin redirected. Haven't got the chance to do so, but will reply back if I find any issue with that.

[daxian-dbw]

I played with the changes and found an issue:

$p = Start-Process pwsh -ArgumentList '-c Write-Host start $pid; sleep -seconds 30; Write-Host stop' -NoNewWindow -passThru

After the option is enabled, when starting a console application like above, the process will be terminated immediately even though it doesn't really consume console input.

I don't think this is the right behavior. In this case, the new process shouldn't be considered as an "orphaned process" from the user's viewpoint.

Maybe we should check if a console-attached process is actually an orphan by verifying if its parent process id still points to an alive process that was created earlier.

[jazzdelightsme]

Actually, this is exactly the sort of process that we want to kill. Although you came up with a mostly benign example (it only spews a few pieces of "junk" into the middle of your interactive session), this process is very much capable of hosing your shell (more than just writing output while you are at the prompt).

Here is a modified example, which demonstrates that it is connected to the console's input (even without PSReadLine loaded, it will have weird behavior):

$p = Start-Process pwsh -ArgumentList '-NoProfile -c Write-Host start $pid; write-host "input redirected? $([Console]::IsInputRedirected)" ; write-host "output redirected? $([Console]::IsOutputRedirected)" ; Write-Host "give me some input: " ; $userInput = [Console]::ReadLine() ; Write-Host "input was: $userInput" ; sleep -seconds 30; Write-Host stop' -NoNewWindow -passThru

Maybe you were thinking "well, it could read input, but if it doesn't, then it should be allowed". Unfortunately there is (of course) no way to tell if an arbitrary process is going to read input or not. But the fact that it can (while we are also waiting for input) qualifies it for termination.

There are some mitigating factors here:

1. (The biggest, IMO): it seems pretty unlikely that someone would do something like this directly from the command line. If they do it in a script, it is much more likely that the script is doing something sensible with the child process, and not returning to the prompt "early" (before the child process has exited), in which case everything would be fine.
2. This new option is off by default, so it definitely won't start causing a bunch of people to randomly have their processes killed.

Also, I think this aligns well with documented guidance for how/when to use Start-Process (MicrosoftDocs/PowerShell-Docs#6239).

[daxian-dbw]

Maybe you were thinking "well, it could read input, but if it doesn't, then it should be allowed". Unfortunately there is (of course) no way to tell if an arbitrary process is going to read input or not. But the fact that it can (while we are also waiting for input) qualifies it for termination.

I think the criteria should be whether a console-attached process is an orphan.

It's OK if a user explicitly starts a process from PowerShell that also consumes console input -- they are allowed to shoot their own foot. But we should not prevent a user from doing so to just run some background work.

The issue we want to address is to terminate the orphan processes spawned by a build process that was terminated, so we should make sure the changes doesn't go beyond that and break a legit scenario.

• for the target issue we want to address, it's desired for the user to not know a thing about PSReadLine killing those orphan processes, because they should have been gone when the user pressed ctrl+c to terminate the build parent process.
• for the side effect that cause start-process to not work as expected, it will result in lots of troubles for the user to diagnose why the process was killed.

[jazzdelightsme]

Okay, my personal feeling is "I wouldn't like that"... but I understand that it could be difficult for someone to figure out why such a process was killed; and given that it should not be a problem for the scenario I am trying to solve, I am willing to compromise :D so I added the check.

[daxian-dbw]

Thank you. I will do another review soon (first thing tomorrow morning if cannot be earlier :))

[daxian-dbw]

Immediate children may have children processes that accidentally derives the standard input (which I agree is a bad thing to do), so ideally we should check the parent id of the console-attached process to make sure it still points to an alive process that was created earlier.

But I think we can just check for immediate children for simplicity, and wait for feedback.

[daxian-dbw]

I played with the changes and found an issue:

$p = Start-Process pwsh -ArgumentList '-c Write-Host start $pid; sleep -seconds 30; Write-Host stop' -NoNewWindow -passThru

After the option is enabled, when starting a console application like above, the process will be terminated immediately even though it doesn't really consume console input.

I don't think this is the right behavior. In this case, the new process shouldn't be considered as an "orphaned process" from the user's viewpoint.

Maybe we should check if a console-attached process is actually an orphan by verifying if its parent process id still points to an alive process that was created earlier.

[daxian-dbw]

LGTM. Thanks!

[jazzdelightsme]

Thank you @daxian-dbw! QQ: I am having trouble finding a "roadmap" Discussion/Issue. I'm wondering what the shipping vehicle for this change will be; can you point me at the roadmap? TIA!

[daxian-dbw]

We don't have a published roadmap but we plan to get the 2.3 GA out along with PS 7.4 GA.