New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

powershell should run `ShellExecuteEx` on STA thread #2969

Closed
joeyaiello opened this Issue Jan 6, 2017 · 13 comments

Comments

Projects
None yet
6 participants
@joeyaiello
Member

joeyaiello commented Jan 6, 2017

[Edited by @daxian-dbw]

PR #3281 adds support to ShellExecute in powershell core when it's running on Windows full SKUs. With that fix, Start-Process is able to run MSI in powershell core.

However, it's not a complete solution. It calls ShellExecuteEx directly in MTA thread instead of from STA thread as suggested in MSDN. Therefore, the current solution may not work for some shell extensions that require COM.

In .NET Core, managed threads are all eagerly initialized with MTA mode, so to call ShellExecuteEx from a STA thread, we need to create a native thread using CreateThread function and initialize COM with STA on that thread. In #3281, we are calling ShellExecuteEx directly on MTA thread, and it works for things like opening a folder in File Explorer, opening a file with the application that is associated with its extension in shell, opening URL in web browser and etc, but it's not guaranteed to work in all ShellExecution scenarios.

Will keep this issue open to track the "invoke-on-STA-thread" work.


PS:4> Start-Process .\PowerShell_6.0.0-alpha.17-win10-win2016-x64.msi -PassThru

 NPM(K)    PM(M)      WS(M)     CPU(s)     Id  SI ProcessName
 ------    -----      -----     ------     --  -- -----------
      9     6.15       7.55       0.03  20092   3 msiexec

PS:5> $PSVersionTable

Name                           Value
----                           -----
PSVersion                      6.0.0-alpha
WSManStackVersion              3.0
PSRemotingProtocolVersion      2.3
PSEdition                      Core
GitCommitId                    v6.0.0-alpha.16-54-gc86a287726fe366895723dfcc7739f93038615ea
CLRVersion
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}
SerializationVersion           1.1.0.1
BuildVersion                   3.0.0.0
@iSazonov

This comment has been minimized.

Show comment
Hide comment
@iSazonov

iSazonov Jan 6, 2017

Collaborator

I test this for .txt - it don't work too. Seems windows type associations don't work here at all.

Collaborator

iSazonov commented Jan 6, 2017

I test this for .txt - it don't work too. Seems windows type associations don't work here at all.

@mattpwhite

This comment has been minimized.

Show comment
Hide comment
@mattpwhite

mattpwhite Jan 11, 2017

The .NET Core version of System.Diagnostics.Process doesn't support using ShellExecute() to start processes. The .NET Framework version does and does so by default, with an alternate path that uses CreateProcess(). ShellExecute is what makes starting a file launch its associated executable (and other stuff like starting a URL launching your default browser). Have to imagine this is a breaking change for a lot of existing scripts (and that it's probably not trivial to fix).

https://referencesource.microsoft.com/#System/services/monitoring/system/diagnosticts/ProcessStartInfo.cs,48

https://github.com/dotnet/corefx/blob/912cba3675a15718515417e7e9b46504c0303fbc/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartInfo.Windows.cs#L85

mattpwhite commented Jan 11, 2017

The .NET Core version of System.Diagnostics.Process doesn't support using ShellExecute() to start processes. The .NET Framework version does and does so by default, with an alternate path that uses CreateProcess(). ShellExecute is what makes starting a file launch its associated executable (and other stuff like starting a URL launching your default browser). Have to imagine this is a breaking change for a lot of existing scripts (and that it's probably not trivial to fix).

https://referencesource.microsoft.com/#System/services/monitoring/system/diagnosticts/ProcessStartInfo.cs,48

https://github.com/dotnet/corefx/blob/912cba3675a15718515417e7e9b46504c0303fbc/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessStartInfo.Windows.cs#L85

@daxian-dbw

This comment has been minimized.

Show comment
Hide comment
@daxian-dbw

daxian-dbw Mar 11, 2017

Member

#3281 is to add ShellExecute support in powershell core.
It's not a complete solution however. It calls ShellExecuteEx directly in MTA thread instead of from STA thread as suggested in MSDN. Therefore, the current solution may not work for some shell extensions that require COM.

In .NET Core, managed threads are all eagerly initialized with MTA mode, so to call ShellExecuteEx from a STA thread, we need to create a native thread using CreateThread function and initialize COM with STA on that thread. In #3281, we are calling ShellExecuteEx directly on MTA thread, and it works for things like opening a folder in File Explorer, opening a file with the application that is associated with its extension in shell, opening URL in web browser and etc, but it's not guaranteed to work in all ShellExecution scenarios.

I will keep this issue open even after merging that PR to track the "invoke-on-STA-thread" work.

Member

daxian-dbw commented Mar 11, 2017

#3281 is to add ShellExecute support in powershell core.
It's not a complete solution however. It calls ShellExecuteEx directly in MTA thread instead of from STA thread as suggested in MSDN. Therefore, the current solution may not work for some shell extensions that require COM.

In .NET Core, managed threads are all eagerly initialized with MTA mode, so to call ShellExecuteEx from a STA thread, we need to create a native thread using CreateThread function and initialize COM with STA on that thread. In #3281, we are calling ShellExecuteEx directly on MTA thread, and it works for things like opening a folder in File Explorer, opening a file with the application that is associated with its extension in shell, opening URL in web browser and etc, but it's not guaranteed to work in all ShellExecution scenarios.

I will keep this issue open even after merging that PR to track the "invoke-on-STA-thread" work.

@SteveL-MSFT SteveL-MSFT added this to the 6.0.0-beta1 milestone Mar 11, 2017

@daxian-dbw daxian-dbw changed the title from Start-Process will not execute an MSI to powershell should run `ShellExecuteEx` on STA thread Mar 14, 2017

@SteveL-MSFT SteveL-MSFT modified the milestones: 6.0.0-beta2, 6.0.0-beta1 Mar 15, 2017

@iSazonov

This comment has been minimized.

Show comment
Hide comment
@iSazonov

iSazonov Mar 18, 2017

Collaborator

CoreFX issue dotnet/corefx#522 milestone is 1.0.0-rtm so I set "Waiting - NetStandard20"

Collaborator

iSazonov commented Mar 18, 2017

CoreFX issue dotnet/corefx#522 milestone is 1.0.0-rtm so I set "Waiting - NetStandard20"

@joeyaiello

This comment has been minimized.

Show comment
Hide comment
@joeyaiello

joeyaiello Mar 27, 2017

Member

@iSazonov So I'll be the first to admit that this thread is totally over my head, but 1.0.0-rtm in the CoreFX repo implies that that work already shipped in .NET Core 1.0 RTM.

Is .NET Standard 2.0 still relevant here?

Member

joeyaiello commented Mar 27, 2017

@iSazonov So I'll be the first to admit that this thread is totally over my head, but 1.0.0-rtm in the CoreFX repo implies that that work already shipped in .NET Core 1.0 RTM.

Is .NET Standard 2.0 still relevant here?

@iSazonov

This comment has been minimized.

Show comment
Hide comment
@iSazonov

iSazonov Mar 27, 2017

Collaborator

I can't still find full support STA in CoreFX 😕

Collaborator

iSazonov commented Mar 27, 2017

I can't still find full support STA in CoreFX 😕

@daxian-dbw

This comment has been minimized.

Show comment
Hide comment
@daxian-dbw

daxian-dbw Mar 27, 2017

Member

The CoreFX issue was not addressed. It was closed as I quoted here:

We should consider this when we have a concrete motivating scenario that requires it.

Member

daxian-dbw commented Mar 27, 2017

The CoreFX issue was not addressed. It was closed as I quoted here:

We should consider this when we have a concrete motivating scenario that requires it.

@jeffbi

This comment has been minimized.

Show comment
Hide comment
@jeffbi

jeffbi Mar 27, 2017

Contributor

If it helps motivate them, without STA we cannot use IGroupPolicyObject (#3353)

Contributor

jeffbi commented Mar 27, 2017

If it helps motivate them, without STA we cannot use IGroupPolicyObject (#3353)

@daxian-dbw

This comment has been minimized.

Show comment
Hide comment
@daxian-dbw

daxian-dbw Mar 28, 2017

Member

@jeffbi Please leave a comment in the CoreFX issue. Maybe even reactivate the issue if you think it's a concrete scenario that should be supported in .NET Core.

Member

daxian-dbw commented Mar 28, 2017

@jeffbi Please leave a comment in the CoreFX issue. Maybe even reactivate the issue if you think it's a concrete scenario that should be supported in .NET Core.

@jeffbi

This comment has been minimized.

Show comment
Hide comment
@jeffbi

jeffbi Mar 28, 2017

Contributor

@daxian-dbw I have added a comment on the CoreFX issue, but I don't think I can reopen it.

Contributor

jeffbi commented Mar 28, 2017

@daxian-dbw I have added a comment on the CoreFX issue, but I don't think I can reopen it.

@joeyaiello

This comment has been minimized.

Show comment
Hide comment
@joeyaiello

joeyaiello Mar 30, 2017

Member

@jeffbi it's probably better to open a new issue, explain the scenario for our purposes, and then list references to this issue and the original closed issue. Sound okay?

Member

joeyaiello commented Mar 30, 2017

@jeffbi it's probably better to open a new issue, explain the scenario for our purposes, and then list references to this issue and the original closed issue. Sound okay?

@SteveL-MSFT SteveL-MSFT modified the milestones: 6.0.0-beta, 6.0.0 May 15, 2017

@joeyaiello joeyaiello modified the milestones: 6.1.0, 6.0.0 May 16, 2017

@SteveL-MSFT SteveL-MSFT self-assigned this Jul 26, 2017

@SteveL-MSFT SteveL-MSFT modified the milestones: 6.0.0-HighPriority, 6.1.0 Jul 26, 2017

@SteveL-MSFT

This comment has been minimized.

Show comment
Hide comment
@SteveL-MSFT

SteveL-MSFT Jul 26, 2017

Member

STA is now in corefx

Member

SteveL-MSFT commented Jul 26, 2017

STA is now in corefx

@daxian-dbw

This comment has been minimized.

Show comment
Hide comment
@daxian-dbw

daxian-dbw Jul 26, 2017

Member

Sweet! We need to clean up ApartmentState related workarounds in powershell now.

Member

daxian-dbw commented Jul 26, 2017

Sweet! We need to clean up ApartmentState related workarounds in powershell now.

@iSazonov iSazonov referenced this issue Jul 27, 2017

Open

Remove FullCLR code #4357

1 of 145 tasks complete

@daxian-dbw daxian-dbw closed this in #4362 Aug 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment