Fix blocking wait when starting file associated with a Windows application by SteveL-MSFT · Pull Request #13750 · PowerShell/PowerShell · GitHub

PR Summary

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

user starts diskmgmt.msc
check if exe, this is not an exe so set _isWindowsApplication = false
check if associated with an exe
finds mmc.exe
check if exe, this is an exe, so launch this with previous "command" as the argument
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).

…ation

Steve Lee (@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" {

Ilya (@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.

Ilya (@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 :-) )

These names are so similar that it can be confused. Can we keep the old name for the method?

The old name with the Check makes it slightly harder to understand reading the code as it's not as clear what true/false means with Check vs Is. The alternative is to have the local variable be isConsoleApplication to differentiate I guess.

We could use bool windowsApplication without Bulgarian prefix.
If no I agreed with isWindowsApplication.

Since this code currently is only used in one place, I think the casing diff is ok and prefer the is making it a bit more readable

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.

Ilya (@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?

Ilya (@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.

I mean the code

PowerShell/src/System.Management.Automation/engine/NativeCommandProcessor.cs

Lines 478 to 490 in f2a3716

    
           lock (_sync) 
        
           { 
        
               if (_stopped) 
        
               { 
        
                   throw new PipelineStoppedException(); 
        
               } 
        
               try 
        
               { 
        
                   _nativeProcess = new Process() { StartInfo = startInfo }; 
        
                   _nativeProcess.Start(); 
        
               } 
        
               catch (Win32Exception)

If PowerShell is default shell the code will run PowerShell again and again.

Ilya (@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:

Set PowerShell as default shell
Run PowerShell unelevated
Run diskmgmt.msc

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

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

Steve Lee (@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?

Yes, both.

…ation (PowerShell#13750) * Fix blocking wait when starting file associated with a Windows application in elevated process * refactor check if app is WindowsApp

Fix blocking wait when starting file associated with a Windows applic…

4705158

…ation

Steve Lee (SteveL-MSFT) requested a review from Ilya (iSazonov) October 6, 2020 02:00

Steve Lee (SteveL-MSFT) assigned Ilya (iSazonov) Oct 6, 2020

Steve Lee (SteveL-MSFT) added this to the 7.1-Consider milestone Oct 6, 2020

Ilya (iSazonov) added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Oct 6, 2020

Add test

eda58c5

refactor check if app is WindowsApp

76feb55

Ilya (iSazonov) reviewed Oct 6, 2020

View reviewed changes

Ilya (iSazonov) approved these changes Oct 6, 2020

View reviewed changes

Ilya (iSazonov) merged commit 16cc9aa into PowerShell:master Oct 7, 2020

Steve Lee (SteveL-MSFT) deleted the mmc branch October 7, 2020 14:41

Aditya Patwardhan (adityapatwardhan) added the BackPort-7.1.x-Consider label Oct 19, 2020

Aditya Patwardhan (adityapatwardhan) removed this from the 7.1-Consider milestone Oct 19, 2020

Aditya Patwardhan (adityapatwardhan) added BackPort-7.1.x-Approved and removed BackPort-7.1.x-Consider labels Oct 19, 2020

ghost mentioned this pull request Oct 21, 2020

When PowerShell runs elevated on Windows, starting *.msc files locks the console #13744

Closed

Aditya Patwardhan (adityapatwardhan) added Backport-7.1.x-Done Backport to 7.1.x completed and removed BackPort-7.1.x-Approved labels Oct 27, 2020

                           _startPosition = new Host.Coordinates();
-                          CalculateIORedirection(out redirectOutput, out redirectError, out redirectInput);
+                          bool isWindowsApplication = IsWindowsApplication(this.Path);

Fix blocking wait when starting file associated with a Windows application by SteveL-MSFT · Pull Request #13750 · PowerShell/PowerShell · GitHub

Uh oh!

Fix blocking wait when starting file associated with a Windows application - #13750

Fix blocking wait when starting file associated with a Windows application#13750Ilya (iSazonov) merged 3 commits intoPowerShell:masterPowerShell/PowerShell:masterfrom SteveL-MSFT:mmcSteveL-MSFT/PowerShell:mmcCopy head branch name to clipboard

Conversation

Steve Lee (SteveL-MSFT) commented Oct 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

Ilya (iSazonov) commented Oct 6, 2020

Uh oh!

Steve Lee (SteveL-MSFT) commented Oct 6, 2020

Uh oh!

Ilya (iSazonov) commented Oct 6, 2020

Uh oh!

Steve Lee (SteveL-MSFT) commented Oct 6, 2020

Uh oh!

Ilya (iSazonov) commented Oct 6, 2020

Uh oh!

Ilya (iSazonov) Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

Steve Lee (SteveL-MSFT) Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

Steve Lee (SteveL-MSFT) Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) commented Oct 6, 2020

Uh oh!

Steve Lee (SteveL-MSFT) commented Oct 6, 2020

Uh oh!

Ilya (iSazonov) commented Oct 6, 2020

Uh oh!

Steve Lee (SteveL-MSFT) commented Oct 6, 2020

Uh oh!

Ilya (iSazonov) commented Oct 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Steve Lee (SteveL-MSFT) commented Oct 6, 2020

Uh oh!

Ilya (iSazonov) commented Oct 6, 2020

Uh oh!

Steve Lee (SteveL-MSFT) commented Oct 6, 2020

Uh oh!

Ilya (iSazonov) commented Oct 7, 2020

Uh oh!

Deleted user (ghost) commented Oct 21, 2020

Uh oh!

Deleted user (ghost) commented Nov 17, 2020

Uh oh!

God-damnit-all commented Dec 12, 2020

Uh oh!

Ilya (iSazonov) commented Dec 12, 2020

Uh oh!

God-damnit-all commented Dec 12, 2020

Uh oh!

Ilya (iSazonov) commented Dec 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Fix blocking wait when starting file associated with a Windows application#13750
Ilya (iSazonov) merged 3 commits into
PowerShell:masterfrom
SteveL-MSFT:mmc

Steve Lee (SteveL-MSFT) commented Oct 6, 2020 •
edited
Loading

Ilya (iSazonov) commented Oct 6, 2020 •
edited
Loading