Make ShellExecuteEx use STA #4362
Conversation
SteveL-MSFT
force-pushed
the
startprocess
branch
2 times, most recently
from
4610555 to
de5aae6
Compare
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.
We should check if the current thread is STA already. If so, invoke the method without creating a new thread.
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.
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
SteveL-MSFT
force-pushed
the
startprocess
branch
from
de5aae6 to
405cc7f
Compare
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.
SteveL-MSFT
force-pushed
the
startprocess
branch
from
405cc7f to
d523ca0
Compare
| 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
| #if CORECLR | ||
| 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
4 participants
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