Skip to content

Commit

Permalink
Fix a couple of apartment state issues
Browse files Browse the repository at this point in the history
Fix for https://github.com/dotnet/coreclr/issues/17822
- The apartment state now defaults to MTA for the main thread along with a CoInitialize
- Calling `Thread.SetApartmentState` with STA now fails as expected (different behavior from previous netcore, same behavior as netfx)

Fix for https://github.com/dotnet/coreclr/issues/17787
- `WaitHandle.WaitAll` for multiple handles is not supported on an STA thread due to issues described in https://github.com/dotnet/coreclr/issues/17787#issuecomment-385117537
- It now throws `NotSupportedException` as expected (different behavior from previous netcore, same behavior as netfx)

Fix for https://github.com/dotnet/coreclr/issues/19225
  • Loading branch information
kouvel committed Aug 9, 2018
1 parent d7191b5 commit 0f5369e
Show file tree
Hide file tree
Showing 13 changed files with 309 additions and 12 deletions.
3 changes: 3 additions & 0 deletions src/System.Private.CoreLib/Resources/Strings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -3049,6 +3049,9 @@
<data name="NotSupported_UnknownTypeCode" xml:space="preserve">
<value>TypeCode '{0}' was not valid.</value>
</data>
<data name="NotSupported_WaitAllSTAThread" xml:space="preserve">
<value>WaitAll for multiple handles on a STA thread is not supported.</value>
</data>
<data name="NotSupported_UnreadableStream" xml:space="preserve">
<value>Stream does not support reading.</value>
</data>
Expand Down
3 changes: 2 additions & 1 deletion src/vm/appdomain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3074,8 +3074,9 @@ void SystemDomain::SetThreadAptState (Thread::ApartmentState state)
Thread::ApartmentState pState = pThread->SetApartment(Thread::AS_InSTA, TRUE);
_ASSERTE(pState == Thread::AS_InSTA);
}
else if (state == Thread::AS_InMTA)
else
{
// If an apartment state was not explicitly requested, default to MTA
Thread::ApartmentState pState = pThread->SetApartment(Thread::AS_InMTA, TRUE);
_ASSERTE(pState == Thread::AS_InMTA);
}
Expand Down
6 changes: 1 addition & 5 deletions src/vm/assembly.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1798,11 +1798,7 @@ INT32 Assembly::ExecuteMainMethod(PTRARRAYREF *stringArgs, BOOL waitForOtherThre

Thread::ApartmentState state = Thread::AS_Unknown;
state = SystemDomain::GetEntryPointThreadAptState(pMeth->GetMDImport(), pMeth->GetMemberDef());

// If the entry point has an explicit thread apartment state, set it
// before running the AppDomainManager initialization code.
if (state == Thread::AS_InSTA || state == Thread::AS_InMTA)
SystemDomain::SetThreadAptState(state);
SystemDomain::SetThreadAptState(state);
#endif // FEATURE_COMINTEROP
}

Expand Down
14 changes: 8 additions & 6 deletions src/vm/comwaithandle.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -227,12 +227,14 @@ FCIMPL4(INT32, WaitHandleNative::CorWaitMultipleNative, Object* waitObjectsUNSAF
PTRARRAYREF pWaitObjects = (PTRARRAYREF)waitObjects; // array of objects on which to wait
int numWaiters = pWaitObjects->GetNumComponents();

// Note: this should really be FEATURE_COMINTEROP_APARTMENT_SUPPORT.
// Because it's not, CoreCLR will allow WaitAll on STA threads.
// But fixing this would be a breaking change at this point, since we already shipped
// SL 2 and 3 this way.
// Perhaps in a future release we can fix this, if we aren't quite so concerned about
// compatibility....
#ifdef FEATURE_COMINTEROP_APARTMENT_SUPPORT
// There are some issues with wait-all from an STA thread
// - https://github.com/dotnet/coreclr/issues/17787#issuecomment-385117537
if (waitForAll && numWaiters > 1 && pThread->GetApartment() == Thread::AS_InSTA)
{
COMPlusThrow(kNotSupportedException, W("NotSupported_WaitAllSTAThread"));
}
#endif // FEATURE_COMINTEROP && !FEATURE_CORECLR

WaitHandleArrayHolder arrayHolder;
arrayHolder.Initialize(numWaiters, (PTRARRAYREF*) &waitObjects);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Threading;

public static class SetApartmentStateDefaultMtaTests
{
public static int Main()
{
if (CannotChangeApartmentStateOfMainThreadTest())
{
return 100; // pass
}
return 101; // fail
}

private static bool CannotChangeApartmentStateOfMainThreadTest()
{
var thread = Thread.CurrentThread;
try
{
thread.SetApartmentState(ApartmentState.STA);

Console.WriteLine("CannotChangeApartmentStateOfMainThreadTest: Unexpected success (no exception): 'SetApartmentState(ApartmentState.STA)'");
return false;
}
catch (InvalidOperationException)
{
}
catch (Exception ex)
{
Console.WriteLine($"CannotChangeApartmentStateOfMainThreadTest: Unexpected exception during 'SetApartmentState(ApartmentState.STA)': {ex}");
return false;
}

try
{
thread.SetApartmentState(ApartmentState.MTA);
}
catch (Exception ex)
{
Console.WriteLine($"CannotChangeApartmentStateOfMainThreadTest: Unexpected exception during 'SetApartmentState(ApartmentState.MTA)': {ex}");
return false;
}

return true;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<?xml version="1.0" encoding="utf-8"?>
<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
<PropertyGroup>
<Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
<Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
<ProjectGuid>{0F4DD32F-EC7D-4AEF-9C6F-0C40E7CC7F2C}</ProjectGuid>
<OutputType>Exe</OutputType>
</PropertyGroup>
<!-- Default configurations to help VS understand the configurations -->
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'Debug|x64'">
</PropertyGroup>
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'Release|x64'">
</PropertyGroup>
<ItemGroup>
<Compile Include="SetApartmentStateDefaultMta.cs" />
</ItemGroup>
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
</Project>
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Threading;

public static class SetApartmentStateMtaTests
{
[MTAThread]
public static int Main()
{
if (CannotChangeApartmentStateOfMainThreadTest())
{
return 100; // pass
}
return 101; // fail
}

private static bool CannotChangeApartmentStateOfMainThreadTest()
{
var thread = Thread.CurrentThread;
try
{
thread.SetApartmentState(ApartmentState.STA);

Console.WriteLine("CannotChangeApartmentStateOfMainThreadTest: Unexpected success (no exception): 'SetApartmentState(ApartmentState.STA)'");
return false;
}
catch (InvalidOperationException)
{
}
catch (Exception ex)
{
Console.WriteLine($"CannotChangeApartmentStateOfMainThreadTest: Unexpected exception during 'SetApartmentState(ApartmentState.STA)': {ex}");
return false;
}

try
{
thread.SetApartmentState(ApartmentState.MTA);
}
catch (Exception ex)
{
Console.WriteLine($"CannotChangeApartmentStateOfMainThreadTest: Unexpected exception during 'SetApartmentState(ApartmentState.MTA)': {ex}");
return false;
}

return true;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<?xml version="1.0" encoding="utf-8"?>
<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
<PropertyGroup>
<Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
<Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
<ProjectGuid>{D5F2231F-6DEA-4DB2-AF79-120CABAED49B}</ProjectGuid>
<OutputType>Exe</OutputType>
</PropertyGroup>
<!-- Default configurations to help VS understand the configurations -->
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'Debug|x64'">
</PropertyGroup>
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'Release|x64'">
</PropertyGroup>
<ItemGroup>
<Compile Include="SetApartmentStateMta.cs" />
</ItemGroup>
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
</Project>
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Threading;

public static class SetApartmentStateStaTests
{
[STAThread]
public static int Main()
{
if (CannotChangeApartmentStateOfMainThreadTest())
{
return 100; // pass
}
return 101; // fail
}

private static bool CannotChangeApartmentStateOfMainThreadTest()
{
var thread = Thread.CurrentThread;
try
{
thread.SetApartmentState(ApartmentState.MTA);

Console.WriteLine("CannotChangeApartmentStateOfMainThreadTest: Unexpected success (no exception): 'SetApartmentState(ApartmentState.MTA)'");
return false;
}
catch (InvalidOperationException)
{
}
catch (Exception ex)
{
Console.WriteLine($"CannotChangeApartmentStateOfMainThreadTest: Unexpected exception during 'SetApartmentState(ApartmentState.MTA)': {ex}");
return false;
}

try
{
thread.SetApartmentState(ApartmentState.STA);
}
catch (Exception ex)
{
Console.WriteLine($"CannotChangeApartmentStateOfMainThreadTest: Unexpected exception during 'SetApartmentState(ApartmentState.STA)': {ex}");
return false;
}

return true;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<?xml version="1.0" encoding="utf-8"?>
<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
<PropertyGroup>
<Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
<Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
<ProjectGuid>{6EE5C24D-CB8C-42AD-9D44-B49D584FB173}</ProjectGuid>
<OutputType>Exe</OutputType>
</PropertyGroup>
<!-- Default configurations to help VS understand the configurations -->
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'Debug|x64'">
</PropertyGroup>
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'Release|x64'">
</PropertyGroup>
<ItemGroup>
<Compile Include="SetApartmentStateSta.cs" />
</ItemGroup>
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
</Project>
66 changes: 66 additions & 0 deletions tests/src/baseservices/threading/ApartmentState/WaitAllSta.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Threading;

public static class WaitAllStaTests
{
[STAThread]
public static int Main()
{
if (WaitAllNotSupportedOnSta_Test0() &&
WaitAllNotSupportedOnSta_Test1())
{
return 100; // pass
}
return 101; // fail
}

private static bool WaitAllNotSupportedOnSta_Test0()
{
var wh = new ManualResetEvent[2];
wh[0] = new ManualResetEvent(true);
wh[1] = new ManualResetEvent(true);
try
{
bool result = WaitHandle.WaitAll(wh, 0);

Console.WriteLine($"WaitAllNotSupportedOnSta_Test0: WaitAll did not throw but returned {result}");
}
catch (NotSupportedException)
{
return true;
}
catch (Exception ex)
{
Console.WriteLine($"WaitAllNotSupportedOnSta_Test0: WaitAll threw unexpected exception: {ex}");
}

return false;
}

private static bool WaitAllNotSupportedOnSta_Test1()
{
var wh = new ManualResetEvent[2];
wh[0] = new ManualResetEvent(true);
wh[1] = wh[0];
try
{
bool result = WaitHandle.WaitAll(wh, 0);

Console.WriteLine($"WaitAllNotSupportedOnSta_Test1: WaitAll did not throw but returned {result}");
}
catch (NotSupportedException)
{
return true;
}
catch (Exception ex)
{
Console.WriteLine($"WaitAllNotSupportedOnSta_Test1: WaitAll threw unexpected exception: {ex}");
}

return false;
}
}
19 changes: 19 additions & 0 deletions tests/src/baseservices/threading/ApartmentState/WaitAllSta.csproj
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<?xml version="1.0" encoding="utf-8"?>
<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
<PropertyGroup>
<Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
<Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
<ProjectGuid>{CEC647A3-9A72-42C6-8023-22670F63E7B2}</ProjectGuid>
<OutputType>Exe</OutputType>
</PropertyGroup>
<!-- Default configurations to help VS understand the configurations -->
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'Debug|x64'">
</PropertyGroup>
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'Release|x64'">
</PropertyGroup>
<ItemGroup>
<Compile Include="WaitAllSta.cs" />
</ItemGroup>
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
</Project>
1 change: 1 addition & 0 deletions tests/testsUnsupportedOutsideWindows.txt
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
baseservices/exceptions/regressions/Dev11/147911/test147911/test147911.sh
baseservices/exceptions/regressions/V1/SEH/VJ/UnmanagedToManaged/UnmanagedToManaged.sh
baseservices/threading/ApartmentState/WaitAllSta.sh
baseservices/threading/commitstackonlyasneeded/DefaultStackCommit/DefaultStackCommit.sh
baseservices/threading/interlocked/compareexchange/compareexchangetneg/compareexchangetneg.sh
baseservices/threading/monitor/pulse/monitorpulse02/monitorpulse02.sh
Expand Down

0 comments on commit 0f5369e

Please sign in to comment.