Skip to content

Set Runspaces to use STA when running in Windows PowerShell #769

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
merged 5 commits into from
Oct 12, 2018
Merged

Set Runspaces to use STA when running in Windows PowerShell #769

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
22791c6
Set STA ApartmentState in Windows PowerShell runspaces
Oct 10, 2018
454bd6d
Use correct overload of CreateDelegate
Oct 10, 2018
46afe90
Use explicit Delegate type on variable
Oct 10, 2018
55427d4
Use string.Equals instead of ==
Oct 11, 2018
2ed6294
Add comment about runspace sta setting
Oct 11, 2018
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
25 changes: 25 additions & 0 deletions src/PowerShellEditorServices/Session/PowerShellContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,12 @@
using System.Collections.ObjectModel;
using System.Globalization;
using System.IO;
using System.Runtime.InteropServices;
using System.Linq;
using System.Management.Automation.Host;
using System.Management.Automation.Remoting;
using System.Management.Automation.Runspaces;
using System.Reflection;
using System.Text;
using System.Text.RegularExpressions;
using System.Threading;
Expand All @@ -31,6 +33,21 @@ namespace Microsoft.PowerShell.EditorServices
/// </summary>
public class PowerShellContext : IDisposable, IHostSupportsInteractiveSession
{
private const string DotNetFrameworkDescription = ".NET Framework";

private static readonly Action<Runspace, ApartmentState> s_runspaceApartmentStateSetter;

static PowerShellContext()
{
// PowerShell ApartmentState APIs aren't available in PSStandard, so we need to use reflection
Copy link
Member

Choose a reason for hiding this comment

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

Should this be in PowerShellStandard?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no such property in PSCore, and PSStandard is PSCore ∩ Windows PowerShell. So it makes sense it's not in PSStandard.

Copy link
Member

Choose a reason for hiding this comment

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

I think it could possibly be in PSStandard but then throw Not Implemented Exception for PSCore.

We shouldn't need to use reflection.

This is an edge case for sure but we should talk about it.

Cc @SteveL-MSFT @JamesWTruher

Copy link
Collaborator

Choose a reason for hiding this comment

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

@tylerl0706 If the member has been removed completely, wouldn't that throw a missing member exception at runtime?

Copy link
Member

Choose a reason for hiding this comment

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

PSCore6 currently doesn't support setting apartmentstate. There's currently no plan to add it back. One of the things I dislike about .NET Std is APIs that exist but throw PlatformNotSupportedException at runtime meaning I can't rely on build time to find api problems for cross platform.

Copy link
Member

Choose a reason for hiding this comment

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

PSCore6 currently doesn't support setting apartmentstate. There's currently no plan to add it back.

Makes sense and I'm definitely not asking for it. More concerned about:

One of the things I dislike about .NET Std is APIs that exist but throw PlatformNotSupportedException at runtime meaning I can't rely on build time to find api problems for cross platform

I can understand your dislike for that but the alternative is to use reflection which isn't really elegant, right?

I don't know the .NET Std team's reasoning behind going down that route but I wouldn't be surprised if this was part of that reasoning:

"not having to rely on reflection for scenarios when you have to do extra work on one platform to get the expected behavior on all"

Copy link
Member

Choose a reason for hiding this comment

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

In this case, it's the lesser of two evils. I don't think reflection in itself is necessary a bad thing as long as you use a publicly documented API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm personally in favour of compile time checking helping to identify what would otherwise be an unforeseeable runtime crash. But either way, the reflection done here is (1) done once, (2) efficiently compiled to a delegate and (3) called as readably as I could make it -- so hopefully that smooths over the clumsiness of reflection a little bit.

Copy link
Member

Choose a reason for hiding this comment

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

That's totally fine. Just wanted to bring it up. If we want to go down this route we should at least have a doc on best practices for these sorts of things.

if (RuntimeInformation.FrameworkDescription.Equals(DotNetFrameworkDescription))
{
MethodInfo setterInfo = typeof(Runspace).GetProperty("ApartmentState").GetSetMethod();
Delegate setter = Delegate.CreateDelegate(typeof(Action<Runspace, ApartmentState>), firstArgument: null, method: setterInfo);
s_runspaceApartmentStateSetter = (Action<Runspace, ApartmentState>)setter;
}
}

#region Fields

private readonly SemaphoreSlim resumeRequestHandle = AsyncUtils.CreateSimpleLockingSemaphore();
Expand Down Expand Up @@ -174,6 +191,14 @@ public static Runspace CreateRunspace(PSHost psHost)
}

Runspace runspace = RunspaceFactory.CreateRunspace(psHost, initialSessionState);

// Windows PowerShell must be hosted in STA mode
// This must be set on the runspace *before* it is opened
if (RuntimeInformation.FrameworkDescription.Equals(DotNetFrameworkDescription))
{
s_runspaceApartmentStateSetter(runspace, ApartmentState.STA);
}

runspace.ThreadOptions = PSThreadOptions.ReuseThread;
runspace.Open();

Expand Down