Skip to content

Add check if STA is supported before using on console startup #15106

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.

Sign up for GitHub

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

Merged
Rob Holt (rjmholt) merged 11 commits into from
Apr 22, 2021
Merged

Add check if STA is supported before using on console startup #15106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
125f3aa
Add check if STA is supported before using on console startup
SteveL-MSFT Mar 26, 2021
a0a0446
fix codefactor
SteveL-MSFT Mar 26, 2021
15a499c
fix UNIX build issue
SteveL-MSFT Mar 26, 2021
28a9fe0
update unit test
SteveL-MSFT Mar 26, 2021
f9e2d4b
fix unit tests
SteveL-MSFT Mar 26, 2021
c59ec94
address Paul's feedback by adding a comment
SteveL-MSFT Mar 26, 2021
3b7882b
Change detection of STA to using COM
SteveL-MSFT Apr 15, 2021
12b82c4
remove unnecessary System.Threading
SteveL-MSFT Apr 15, 2021
a4c1fd2
fix CodeFactor
SteveL-MSFT Apr 15, 2021
bc1cd52
change to lazy init addressing Rob's feedback
SteveL-MSFT Apr 15, 2021
f7c5eaf
address codefactor
SteveL-MSFT Apr 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,7 +477,7 @@ internal bool StaMode
}
else
{
return true;
return Platform.IsStaSupported;
}
}
}
Expand Down Expand Up @@ -928,7 +928,7 @@ private void ParseHelper(string[] args)
}
else if (MatchSwitch(switchKey, "sta", "sta"))
{
if (!Platform.IsWindowsDesktop)
if (!Platform.IsWindowsDesktop || !Platform.IsStaSupported)
{
SetCommandLineError(
CommandLineParameterParserStrings.STANotImplemented);
Expand Down
50 changes: 48 additions & 2 deletions src/System.Management.Automation/CoreCLR/CorePsPlatform.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@ namespace System.Management.Automation
/// </summary>
public static class Platform
{
private static string _tempDirectory = null;

/// <summary>
/// True if the current platform is Linux.
/// </summary>
Expand Down Expand Up @@ -140,6 +138,21 @@ public static bool IsWindowsDesktop
}
}

/// <summary>
/// Gets a value indicating whether the underlying system supports single-threaded apartment.
/// </summary>
public static bool IsStaSupported
{
get
{
#if UNIX
return false;
#else
return _isStaSupported.Value;
#endif
}
}

#if UNIX
// Gets the location for cache and config folders.
internal static readonly string CacheDirectory = Platform.SelectProductNameForDirectory(Platform.XDG_Type.CACHE);
Expand All @@ -148,6 +161,22 @@ public static bool IsWindowsDesktop
// Gets the location for cache and config folders.
internal static readonly string CacheDirectory = Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData) + @"\Microsoft\PowerShell";
internal static readonly string ConfigDirectory = Environment.GetFolderPath(Environment.SpecialFolder.Personal) + @"\PowerShell";
private static readonly Lazy<bool> _isStaSupported = new Lazy<bool>(() =>
Comment thread
SteveL-MSFT marked this conversation as resolved.
{
// See objbase.h
const int COINIT_APARTMENTTHREADED = 0x2;
const int E_NOTIMPL = unchecked((int)0X80004001);
int result = Windows.NativeMethods.CoInitializeEx(IntPtr.Zero, COINIT_APARTMENTTHREADED);

@PaulHigin Paul Higinbotham (PaulHigin) Apr 19, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this PInvoke is anymore efficient than spinning up a thread before. Has any perf testing been done?

@PaulHigin Paul Higinbotham (PaulHigin) Apr 19, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can an exception be thrown here?

@SteveL-MSFT Steve Lee (SteveL-MSFT) Apr 22, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CoInitializeEx only returns an HRESULT, so there should never be an exception here unless .NET throws.


// If 0 is returned the thread has been initialized for the first time
// as an STA and thus supported and needs to be uninitialized.
if (result > 0)
{
Windows.NativeMethods.CoUninitialize();
}
Comment on lines +171 to +176

@iSazonov Ilya (iSazonov) Apr 22, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why we need the check if it is used in startup scenario.
If we want to have universal code the condition looks wrong:
S_Ok = 0
S_False = 1
RPC_E_CHANGED_MODE = -2147417850

@SteveL-MSFT Steve Lee (SteveL-MSFT) Apr 22, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's needed in the startup scenario as the main console runspace needs to start as either STA or MTA. So we need to know if STA is supported before trying STA otherwise pwsh ends up in an unusable state. S_False will never apply here as CoInitialize() won't be called before this.


return result != E_NOTIMPL;
});

private static bool? _isNanoServer = null;
private static bool? _isIoT = null;
Expand All @@ -170,6 +199,8 @@ public static bool IsWindowsDesktop
"WSMan.format.ps1xml"
};

private static string _tempDirectory = null;

/// <summary>
/// Some common environment variables used in PS have different
/// names in different OS platforms.
Expand Down Expand Up @@ -557,6 +588,21 @@ internal static int NonWindowsGetProcessParentPid(int pid)
return IsMacOS ? Unix.NativeMethods.GetPPid(pid) : Unix.GetProcFSParentPid(pid);
}

internal static class Windows
{
/// <summary>The native methods class.</summary>
internal static class NativeMethods
{
private const string ole32Lib = "api-ms-win-core-com-l1-1-0.dll";

[DllImport(ole32Lib)]
internal static extern int CoInitializeEx(IntPtr reserve, int coinit);

[DllImport(ole32Lib)]
internal static extern void CoUninitialize();
}
}

// Please note that `Win32Exception(Marshal.GetLastWin32Error())`
// works *correctly* on Linux in that it creates an exception with
// the string perror would give you for the last set value of errno.
Expand Down
4 changes: 2 additions & 2 deletions src/System.Management.Automation/engine/hostifaces/LocalPipeline.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ protected override void StartPipelineExecution()
}

#if !UNIX
if (apartmentState != ApartmentState.Unknown && !Platform.IsNanoServer && !Platform.IsIoT)
if (apartmentState != ApartmentState.Unknown && Platform.IsStaSupported)
{
invokeThread.SetApartmentState(apartmentState);
}
Expand Down Expand Up @@ -1165,7 +1165,7 @@ internal PipelineThread(ApartmentState apartmentState)
_closed = false;

#if !UNIX
if (apartmentState != ApartmentState.Unknown && !Platform.IsNanoServer && !Platform.IsIoT)
if (apartmentState != ApartmentState.Unknown && Platform.IsStaSupported)
{
_worker.SetApartmentState(apartmentState);
}
Expand Down
20 changes: 18 additions & 2 deletions test/xUnit/csharp/test_CommandLineParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,15 @@ public static void TestDefaults()
Assert.False(cpp.SkipProfiles);
Assert.False(cpp.SocketServerMode);
Assert.False(cpp.SSHServerMode);

@rjmholt Rob Holt (rjmholt) Apr 15, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Assert.False(cpp.SSHServerMode);
Assert.False(cpp.SSHServerMode);

Assert.True(cpp.StaMode);
if (Platform.IsWindows)
{
Assert.True(cpp.StaMode);
}
else
{
Assert.False(cpp.StaMode);
}

Assert.False(cpp.ThrowOnReadAndPrompt);
Assert.False(cpp.WasInitialCommandEncoded);
Assert.Null(cpp.WorkingDirectory);
Expand Down Expand Up @@ -1185,7 +1193,15 @@ public static void TestParameter_LastParameterIsFileName_Exist(params string[] c
Assert.False(cpp.NoExit);
Assert.False(cpp.ShowShortHelp);
Assert.False(cpp.ShowBanner);
Assert.True(cpp.StaMode);
if (Platform.IsWindows)

@rjmholt Rob Holt (rjmholt) Apr 15, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (Platform.IsWindows)
if (Platform.IsWindows)

{
Assert.True(cpp.StaMode);
}
else
{
Assert.False(cpp.StaMode);
}

Assert.Equal(CommandLineParameterParser.NormalizeFilePath(commandLine[commandLine.Length - 1]), cpp.File);
Assert.Null(cpp.ErrorMessage);
}
Expand Down