Set Runspaces to use STA when running in Windows PowerShell #769
Conversation
rjmholt
requested review from
TylerLeonhardt,
rkeithhill and
SeeminglyScience
|
|
||
| static PowerShellContext() | ||
| { | ||
| // PowerShell ApartmentState APIs aren't available in PSStandard, so we need to use reflection |
There was a problem hiding this comment.
Should this be in PowerShellStandard?
There was a problem hiding this comment.
There's no such property in PSCore, and PSStandard is PSCore ∩ Windows PowerShell. So it makes sense it's not in PSStandard.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@tylerl0706 If the member has been removed completely, wouldn't that throw a missing member exception at runtime?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -174,6 +191,13 @@ public static Runspace CreateRunspace(PSHost psHost) | |||
| } | |||
|
|
|||
| Runspace runspace = RunspaceFactory.CreateRunspace(psHost, initialSessionState); | |||
|
|
|||
| // Windows PowerShell must be hosted in STA mode | |||
| if (RuntimeInformation.FrameworkDescription == DotNetFrameworkDescription) | |||
There was a problem hiding this comment.
Ends up being the same thing I believe, but I usually write this as
if (RuntimeInformation.FrameworkDescription.Equals(DotNetFrameworkDescription, StringComparison.Ordinal))If we ever start using code factor or some other static analyzer that often gets flagged.
…ll#769) Update PSStandard dependency (PowerShell#771)
Fixes PowerShell/vscode-powershell#1571.
I carelessly took out the
runspace.ApartmentState = ApartmentState.STAline of PSES v1.x without thinking and forgot to reinstate it.This PR puts it back in.