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

SteveL-MSFT · 2020-10-06T02:00:04Z

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

…ation

iSazonov · 2020-10-06T03:49:14Z

@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" {

SteveL-MSFT · 2020-10-06T13:18:15Z

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

iSazonov · 2020-10-06T13:34:48Z

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.

SteveL-MSFT · 2020-10-06T13:40:46Z

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

iSazonov · 2020-10-06T14:10:09Z

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

iSazonov · 2020-10-06T14:22:30Z

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

            _startPosition = new Host.Coordinates();

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


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

iSazonov · 2020-10-06T14:52:20Z

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.

SteveL-MSFT · 2020-10-06T15:20:45Z

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

iSazonov · 2020-10-06T15:36:28Z

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

SteveL-MSFT · 2020-10-06T15:45:45Z

@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 · 2020-10-06T15:58:13Z

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.

SteveL-MSFT · 2020-10-06T16:04:15Z

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

iSazonov · 2020-10-06T18:06:22Z

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.

SteveL-MSFT · 2020-10-06T20:23:11Z

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

iSazonov · 2020-10-07T07:57:33Z

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

ghost · 2020-10-21T19:08:29Z

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

Handy links:

Release Notes

ghost · 2020-11-17T23:02:06Z

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

Handy links:

Release Notes

God-damnit-all · 2020-12-12T05:18:57Z

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.

iSazonov · 2020-12-12T06:39:02Z

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

God-damnit-all · 2020-12-12T07:35:36Z

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?

iSazonov · 2020-12-12T08:02:11Z

Yes, both.

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

4705158

…ation

SteveL-MSFT requested a review from iSazonov October 6, 2020 02:00

SteveL-MSFT assigned iSazonov Oct 6, 2020

SteveL-MSFT added this to the 7.1-Consider milestone Oct 6, 2020

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

iSazonov reviewed Oct 6, 2020

View reviewed changes

iSazonov approved these changes Oct 6, 2020

View reviewed changes

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

SteveL-MSFT deleted the mmc branch October 7, 2020 14:41

adityapatwardhan added the BackPort-7.1.x-Consider label Oct 19, 2020

adityapatwardhan removed this from the 7.1-Consider milestone Oct 19, 2020

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

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

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

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

Uh oh!

Conversation

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!

iSazonov commented Oct 6, 2020

Uh oh!

SteveL-MSFT commented Oct 6, 2020

Uh oh!

iSazonov commented Oct 6, 2020

Uh oh!

SteveL-MSFT commented Oct 6, 2020

Uh oh!

iSazonov commented Oct 6, 2020

Uh oh!

iSazonov Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

iSazonov Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT Oct 6, 2020

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Oct 6, 2020

Uh oh!

SteveL-MSFT commented Oct 6, 2020

Uh oh!

iSazonov commented Oct 6, 2020

Uh oh!

SteveL-MSFT commented Oct 6, 2020

Uh oh!

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

Uh oh!

Uh oh!

SteveL-MSFT commented Oct 6, 2020

Uh oh!

iSazonov commented Oct 6, 2020

Uh oh!

SteveL-MSFT commented Oct 6, 2020

Uh oh!

iSazonov commented Oct 7, 2020

Uh oh!

ghost commented Oct 21, 2020

Uh oh!

ghost commented Nov 17, 2020

Uh oh!

God-damnit-all commented Dec 12, 2020

Uh oh!

iSazonov commented Dec 12, 2020

Uh oh!

God-damnit-all commented Dec 12, 2020

Uh oh!

iSazonov commented Dec 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

SteveL-MSFT commented Oct 6, 2020 •

edited

Loading

iSazonov commented Oct 6, 2020 •

edited

Loading