Enable ApartmentState support #4772

SteveL-MSFT · 2017-09-07T19:00:36Z

WSManConfigProvider doesn't work once this is enabled if starting powershell with -sta, it does work if using -mta. The same/similar WSManConfigProvider code works fine under Windows PowerShell 5.1 with sta or mta. We shouldn't have to change the WSManConfigProvider code which relies on underlying non-thread safe COM objects, but need some help debugging this.

Fix #4350

add support for -sta/-mta in powershell.exe enable ApartmentState support overall some FullCLR code cleanup

fix WSMan provider

iSazonov · 2017-09-08T09:58:57Z

src/System.Management.Automation/engine/remoting/common/WireDataFormat/EncodeAndDecode.cs

                // So just return the CurrentTimeZone.

-#if !CORECLR // TimeZone Not In CoreCLR
+#if !CORECLR // TimeZone deprecated, need to change to TimeZoneInfo


Maybe add that this change affects serialization, remoting and backward compatibility. We can go back to that when we'll have tests for the areas.

Good suggestion, will add

adityapatwardhan · 2017-09-14T18:12:25Z

src/System.Management.Automation/engine/remoting/common/WireDataFormat/EncodeAndDecode.cs

                // So just return the CurrentTimeZone.

-#if !CORECLR // TimeZone Not In CoreCLR
+#if !CORECLR // TODO: TimeZone deprecated, need to change to TimeZoneInfo.  Affects serialization, remoting, and backwards compatibility.


According to documentation here: https://docs.microsoft.com/en-us/dotnet/api/system.timezone?view=netcore-2.0
TimeZone class can still be used to get CurrentTimeZone. A comment about deprecation might not be accurate.

At compilation time, dotnet reported it as deprecated

Confirm. https://referencesource.microsoft.com/#mscorlib/system/timezone.cs,33

@SteveL-MSFT
What do you mean when you say the WSManConfigProvider doesn't work? Do you have repro steps and errors?

@PaulHigin using sta, you would get this error:

dir : COM object that has been separated from its underlying RCW cannot be used. At line:1 char:1 + dir + ~~~ + CategoryInfo : NotSpecified: (:) [Get-ChildItem], InvalidComObjectException + FullyQualifiedErrorId : System.Runtime.InteropServices.InvalidComObjectException,Microsoft.PowerShell.Commands.GetChildItemCommand

daxian-dbw · 2017-09-19T18:04:24Z

src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHost.cs

-#if !CORECLR
                runspace.ApartmentState = ApartmentState.STA;
-#endif
-                runspace.ThreadOptions = PSThreadOptions.ReuseThread;


Is this line supposed to be removed?

daxian-dbw · 2017-09-19T19:20:42Z

src/Microsoft.WSMan.Management/CredSSP.cs

                if (Credential_Delegation_Key == null)
-                    Credential_Delegation_Key = rootKey.CreateSubKey(Registry_Path_Credentials_Delegation
-#if !CORECLR
-                        , RegistryKeyPermissionCheck.ReadWriteSubTree


Microsoft.Win32.RegistryKeyPermissionCheck is available in .NET Core. Maybe we should keep it?

daxian-dbw · 2017-09-19T19:21:13Z

src/Microsoft.WSMan.Management/CredSSP.cs

                if (Allow_Fresh_Credential_Key == null)
-                    Allow_Fresh_Credential_Key = rootKey.CreateSubKey(Registry_Path_Credentials_Delegation + @"\" + helper.Key_Allow_Fresh_Credentials
-#if !CORECLR
-                        , RegistryKeyPermissionCheck.ReadWriteSubTree


daxian-dbw · 2017-09-19T19:22:02Z

src/Microsoft.WSMan.Management/CredSSP.cs

-            if (Environment.OSVersion.Version.Major < 6)
-            {
-                //OS is XP/Win2k3. Throw error.
-                string message = helper.FormatResourceMsgFromResourcetext("CmdletNotAvailable");


Maybe the resource string CmdletNotAvailable can be removed?

daxian-dbw · 2017-09-19T19:42:33Z

src/Microsoft.WSMan.Management/Microsoft.WSMan.Management.csproj

  <ItemGroup>
    <ProjectReference Include="..\System.Management.Automation\System.Management.Automation.csproj" />
    <ProjectReference Include="..\Microsoft.WSMan.Runtime\Microsoft.WSMan.Runtime.csproj" />
+    <PackageReference Include="System.ServiceProcess.ServiceController" Version="4.4.0" />


Did you rebase? Some changes in this PR is also in #4756

daxian-dbw · 2017-09-19T19:43:21Z

src/Microsoft.WSMan.Management/WsManHelper.cs

                RegistryKey rGPOLocalMachineKey = Registry.LocalMachine.OpenSubKey(
                    Registry_Path_Credentials_Delegation + @"\CredentialsDelegation",
-#if !CORECLR
-                    RegistryKeyPermissionCheck.ReadWriteSubTree,


daxian-dbw · 2017-09-19T19:43:29Z

src/Microsoft.WSMan.Management/WsManHelper.cs

                {
                    rGPOLocalMachineKey = rGPOLocalMachineKey.OpenSubKey(Key_Allow_Fresh_Credentials,
-#if !CORECLR
-                        RegistryKeyPermissionCheck.ReadWriteSubTree,


daxian-dbw · 2017-09-19T19:44:24Z

src/Microsoft.WSMan.Management/WsManHelper.cs

-#if CORECLR
                    "0409" /* TODO: don't assume it is always English on CSS? */
-#else
-                    String.Concat("0", String.Format(CultureInfo.CurrentCulture, "{0:x2}", checked((uint)CultureInfo.CurrentUICulture.LCID)))


Shall we use the #else block instead?

daxian-dbw · 2017-09-19T19:53:31Z

src/System.Management.Automation/engine/hostifaces/LocalPipeline.cs

-                else if (i > 100)
-                    i = 100; // maximum 100MB
-                return i * 1000000;
+                // TODO: move this to startupconfig is needed.  This number is in bytes.


Can you add one more comment indicating we use 10MB by default?

daxian-dbw · 2017-09-19T19:55:34Z

src/System.Management.Automation/engine/hostifaces/LocalPipeline.cs

-            _closed = false;
-        }
-#else
        internal PipelineThread(ApartmentState apartmentState)


Is it possible that this method can be called on Unix plats? If so, _worker.SetApartmentState(apartmentState) might throw.

SteveL-MSFT · 2017-09-28T16:17:17Z

@daxian-dbw I've been thinking that I've been looking at this problem the wrong way. ApartmentState is only used for COM interop and dotnet will marshal between STA and MTA so the only loss is some perf impact. I'm thinking that the right thing to do here is to remove support for ApartmentState. Thoughts?

daxian-dbw · 2017-09-29T17:35:09Z

and dotnet will marshal between STA and MTA

@SteveL-MSFT I'm not very clear about the quoted sentence. Can you please elaborate a bit on it?

My understanding is that STA is for proper calls to COM object with the single-thread apartment (mainly about COM objects related to the user interface, for example, windows forms). Windows form may not be a problem as it's not supported in .NET Core. However, it may still be possible that user gets a STA COM object by New-Object and operate on it, so it might be affected when removing the Apartment support from powershell core.

A question, if we remove Apartment support, what would be the default thread apartment for powershell?

I want to point out that on Windows PowerShell, users are able to specify the ApartmentState in PSInvocationSettings and pass the setting to powershell.Invoke(...). Removing ApartmentState support in PS Core is technically a breaking change, so it probably needs to be reviewed by the Committee.

SteveL-MSFT · 2017-09-29T17:50:52Z

@daxian-dbw see Interop Marshaling

I'll close this for now and open a new issue regarding removing ApartmentState support and will discuss with Committee

SteveL-MSFT added 2 commits September 7, 2017 11:56

[Feature]

9cf32d5

add support for -sta/-mta in powershell.exe enable ApartmentState support overall some FullCLR code cleanup

removed difference between coreclr and fullclr code

5887a50

SteveL-MSFT assigned daxian-dbw Sep 7, 2017

SteveL-MSFT requested a review from daxian-dbw September 7, 2017 19:00

SteveL-MSFT requested review from BrucePay, JamesWTruher, PaulHigin, TravisEz13, adityapatwardhan, anmenaga, charub, dantraMSFT, lzybkr, mirichmo and vors as code owners September 7, 2017 19:00

msftclas added the cla-not-required label Sep 7, 2017

SteveL-MSFT force-pushed the apartmentstate-support branch 2 times, most recently from 19b0dbc to 4d1e4f8 Compare September 7, 2017 19:02

[feature]

9a7782d

fix WSMan provider

SteveL-MSFT force-pushed the apartmentstate-support branch from 4d1e4f8 to 9a7782d Compare September 7, 2017 22:51

iSazonov reviewed Sep 8, 2017

View reviewed changes

improved comment about deprecated TimeZone property

080f450

adityapatwardhan requested changes Sep 14, 2017

View reviewed changes

TravisEz13 removed their request for review September 14, 2017 19:34

daxian-dbw requested changes Sep 19, 2017

View reviewed changes

msftgits removed the cla-not-required label Sep 26, 2017

SteveL-MSFT closed this Sep 29, 2017

SteveL-MSFT deleted the apartmentstate-support branch October 16, 2017 21:54

iSazonov mentioned this pull request May 21, 2018

Discussion: Defaults for Thread.ApartmentState #6908

Closed

Enable ApartmentState support #4772

Enable ApartmentState support #4772

Uh oh!

Conversation

SteveL-MSFT commented Sep 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov Sep 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT commented Sep 28, 2017

Uh oh!

daxian-dbw commented Sep 29, 2017

Uh oh!

SteveL-MSFT commented Sep 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

iSazonov Sep 15, 2017 •

edited

Loading