Make ShellExecuteEx use STA #4362
Conversation
4610555 to
de5aae6
Compare
| @@ -106,6 +106,7 @@ Describe "Start-Process" -Tags @("CI","SLOW") { | |||
| } | |||
|
|
|||
| It "Should open the application that associates with extension '.txt'" -Skip:(!$isFullWin) { | |||
| "c:\windows\system32\notepad.exe" | Should Exist | |||
There was a problem hiding this comment.
Ignore this for now, trying to figure out why the test succeeds on my machine but fails in AppVeyor
There was a problem hiding this comment.
Fixed as part of #4353
| throw new Win32Exception(errorCode); | ||
| } | ||
| Thread thread = new Thread(InvokeShellExecuteEx); | ||
| thread.SetApartmentState(System.Threading.ApartmentState.STA); |
There was a problem hiding this comment.
We should check if the current thread is STA already. If so, invoke the method without creating a new thread.
| if (!NativeMethods.ShellExecuteEx(shellExecuteInfo)) | ||
| { | ||
| int errorCode = Marshal.GetLastWin32Error(); | ||
| if (errorCode == 0) |
There was a problem hiding this comment.
The error handling code needs to happen in the main thread, otherwise, the Win32Exception thrown from here will be ignored.
| Thread thread = new Thread(InvokeShellExecuteEx); | ||
| thread.SetApartmentState(System.Threading.ApartmentState.STA); | ||
| thread.Start(shellExecuteInfo); | ||
| thread.Join(); |
There was a problem hiding this comment.
The Win32Exception thrown from the new thread will probably be ignored. We should handle the error in the main thread.
Please take a look at the .NET implementation at https://referencesource.microsoft.com/#System/services/monitoring/system/diagnosticts/Process.cs,2239
The ShellExecuteOnSTAThread method is defined at https://referencesource.microsoft.com/#System/services/monitoring/system/diagnosticts/Process.cs,2856
de5aae6 to
405cc7f
Compare
| /// <summary> | ||
| /// Helper to start process using ShellExecuteEx. This is used only in PowerShell Core on Full Windows. | ||
| /// </summary> | ||
| internal class ShellExecuteHelper | ||
| { | ||
| private static bool _succeeded; | ||
| private static int _errorCode; |
There was a problem hiding this comment.
These 2 static fields will cause race condition when multiple Runsapces are running ShellExecuteHelper.Start at the same time. Using a helper class like in .NET implementation may be a better approach.
405cc7f to
d523ca0
Compare
| @@ -1719,6 +1591,56 @@ internal static Process Start(ProcessStartInfo startInfo, ProcessWindowStyle win | |||
| return processToReturn; | |||
| } | |||
|
|
|||
| private void ShellExecuteFunction() { | |||
There was a problem hiding this comment.
Please use the existing style -- put the open parenthesis on a new line.
| { | ||
| ShellExecuteFunction(); | ||
| } | ||
| return _succeeded; |
There was a problem hiding this comment.
_succeeded is never set to true. Maybe you can set its default value to true, or you can use similar way as in https://referencesource.microsoft.com/#System/services/monitoring/system/diagnosticts/Process.cs,2850
There was a problem hiding this comment.
missed that, will fix
| progFileDir = Path.Combine(appBase, "Modules"); | ||
| #else | ||
| progFileDir = Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.ProgramFiles), "WindowsPowerShell", "Modules"); |
There was a problem hiding this comment.
Don't we want load Windows PowerShell Modules from %ProgramFiles%\WindowsPowerShell? Currently we use PSModulePath as first step solution.
There was a problem hiding this comment.
We'll have a different solution for supporting Windows PowerShell PSModulePath in PSCore6 that replaces the current interim solution
fix shellexecuteex call to use STA as recommended by MSDN
also removed some FullCLR code (left the TODO as it's a bigger item)
Fix #2969