PR Summary

Change #13481 modified some logic on how console apps are detected. The problem in this situation is:

1. user starts diskmgmt.msc
2. check if exe, this is not an exe so set _isWindowsApplication = false
3. check if associated with an exe
4. finds mmc.exe
5. check if exe, this is an exe, so launch this with previous "command" as the argument
6. check _isWindowsApplication to determine if we block waiting for exe to finish, since this value was cached and not updated on step 5, it treats as console exe and waits

Fix is to change CheckIfConsoleApplication() to be non-static so that the second time it's called it updates _isWindowsApplication to point to the new exe and not the previous file.

Tested manually. Since CI won't have a Windows app associated with a file, can't create a test case.

PR Context

Fix #13744

PR Checklist

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
• This PR is ready to merge and is not Work in Progress.
  • If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
• Breaking changes
  • None
  • OR
  • Experimental feature(s) needed
    • Experimental feature name(s):
• User-facing changes
  • Not Applicable
  • OR
  • Documentation needed
    • Issue filed:
• Testing - New and feature
  • N/A or can only be tested interactively
  • OR
  • Make sure you've added a new test if existing tests do not effectively test the code changed
• Tooling
  • I have considered the user experience from a tooling perspective and don't believe tooling will be impacted.
  • OR
  • I have considered the user experience from a tooling perspective and enumerated concerns in the summary. This may include:
    • Impact on PowerShell Editor Services which is used in the PowerShell extension for VSCode (which runs in a different PS Host).
    • Impact on Completions (both in the console and in editors) - one of PowerShell's most powerful features.
    • Impact on PSScriptAnalyzer (which provides linting & formatting in the editor extensions).
    • Impact on EditorSyntax (which provides syntax highlighting with in VSCode, GitHub, and many other editors).

@SteveL-MSFT I think we should simply revert a change in line 557
from
_isRunningInBackground = IsWindowsApplication;
to
_isRunningInBackground = IsWindowsApplication(_nativeProcess.StartInfo.FileName);

As for test, you could see last test in Invoke-Item.Tests,ps1 file:
Describe "Invoke-Item tests on Windows" -Tags "CI","RequireAdminOnWindows" {

@iSazonov the problem is if anyone updates the code and uses CheckIfConsoleApplication() it will have a similar problem as IsWindowsApplication doesn't get updated if we just do the change you are suggesting.

As for the test, it appears the hang only occurs if the associated process explicitly supports elevation like mmc.exe. I can add a test that runs if diskmgmt.msc and mmc.exe are on the system, otherwise it'll be skipped. At least it can work locally.

the problem is if anyone updates the code and uses CheckIfConsoleApplication() it will have a similar problem as IsWindowsApplication doesn't get updated if we just do the change you are suggesting.

My thoughts is that switching to mmc.exe is temporary and we should follow diskmgmt.msc. Otherwise it will very tricky code - input is diskmgmt.msc but result is for mmc.exe.
And I'd remove IsWindowsApplication and IsConsoleApplication because they is used once.

@iSazonov but diskmgmt.msc is not the process so following it doesn't make sense unless I'm misunderstanding. Removing IsWindowsApplication and IsConsoleApplication makes sense.

If we remove IsWindowsApplication we remove _isWindowsApplication and this resolves our discussion (til we will want to make IsWindowsApplication public :-) )

As unrelated notice. The code fallback to default shell (cmd.exe/bash) to invoke an item. But what if we assign PowerShell as default shell? Infinite cycle?
It seems we need to review all places where we fallback to default shell to understand the consequences.

@iSazonov can you point me to what code you are referring to?

In the context - line 488. If UseShellExecute is true we call PowerShell again?

@iSazonov that line with UseShellExecute only starts a new process if the previous attempt fails because it's not an executable and isn't associated with an executable so it tries to let the Shell try to start it. I don't see the problem.

@iSazonov ok, now I see what you mean. However, on my macBook I can see that Process.Start() doesn't start a new shell to run the exe.

I'd expect the cycle with:

1. Set PowerShell as default shell
2. Run PowerShell unelevated
3. Run diskmgmt.msc

In line 534 we will have UseShellExecute = true and run PowerShell again.

@iSazonov perhaps you can open a new issue to discuss that specific concern, I don't see additional pwsh processes, however

@SteveL-MSFT I looked at the UseShellExecute implementation. It does not use a call to default shell. On Windows it uses ShellExecuteEx P/Invoke, on Unix it uses x-bit. So the cycle would be only if PowerShell associated with the extension or file.
On Windows this is an incredible scenario. On Unix the code path is blocked in line 494.
On Unix I still had a concern because in line 488 UseShellExecute is false and Process.Start will just fork and execute - if the file has #!pwsh in header the pwsh would run again but I tried on WSL without problems. So the question is closed.

🎉v7.1.0-rc.2 has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes

🎉v7.2.0-preview.1 has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes

Is this in 7.1 stable? I don't see a revert to this commit in the diff log between 7.1 and 7.1-rc2, but it's re-incorporated in 7.2 preview 1.

It was included in v7.2.0-preview.1. You can check that 7.1 works well in the scenario.

It was included in v7.2.0-preview.1. You can check that 7.1 works well in the scenario.

So... both versions have it?