From 0f5369e21bbb43214c14260abfb11a211d8e6fd4 Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Thu, 9 Aug 2018 02:08:23 -0700 Subject: [PATCH] Fix a couple of apartment state issues 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 --- .../Resources/Strings.resx | 3 + src/vm/appdomain.cpp | 3 +- src/vm/assembly.cpp | 6 +- src/vm/comwaithandle.cpp | 14 ++-- .../SetApartmentStateDefaultMta.cs | 50 ++++++++++++++ .../SetApartmentStateDefaultMta.csproj | 19 ++++++ .../ApartmentState/SetApartmentStateMta.cs | 51 ++++++++++++++ .../SetApartmentStateMta.csproj | 19 ++++++ .../ApartmentState/SetApartmentStateSta.cs | 51 ++++++++++++++ .../SetApartmentStateSta.csproj | 19 ++++++ .../threading/ApartmentState/WaitAllSta.cs | 66 +++++++++++++++++++ .../ApartmentState/WaitAllSta.csproj | 19 ++++++ tests/testsUnsupportedOutsideWindows.txt | 1 + 13 files changed, 309 insertions(+), 12 deletions(-) create mode 100644 tests/src/baseservices/threading/ApartmentState/SetApartmentStateDefaultMta.cs create mode 100644 tests/src/baseservices/threading/ApartmentState/SetApartmentStateDefaultMta.csproj create mode 100644 tests/src/baseservices/threading/ApartmentState/SetApartmentStateMta.cs create mode 100644 tests/src/baseservices/threading/ApartmentState/SetApartmentStateMta.csproj create mode 100644 tests/src/baseservices/threading/ApartmentState/SetApartmentStateSta.cs create mode 100644 tests/src/baseservices/threading/ApartmentState/SetApartmentStateSta.csproj create mode 100644 tests/src/baseservices/threading/ApartmentState/WaitAllSta.cs create mode 100644 tests/src/baseservices/threading/ApartmentState/WaitAllSta.csproj diff --git a/src/System.Private.CoreLib/Resources/Strings.resx b/src/System.Private.CoreLib/Resources/Strings.resx index b1cba1672d4a..366169c69244 100644 --- a/src/System.Private.CoreLib/Resources/Strings.resx +++ b/src/System.Private.CoreLib/Resources/Strings.resx @@ -3049,6 +3049,9 @@ TypeCode '{0}' was not valid. + + WaitAll for multiple handles on a STA thread is not supported. + Stream does not support reading. diff --git a/src/vm/appdomain.cpp b/src/vm/appdomain.cpp index 3ca7ec7e24a2..ef1e173141cf 100644 --- a/src/vm/appdomain.cpp +++ b/src/vm/appdomain.cpp @@ -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); } diff --git a/src/vm/assembly.cpp b/src/vm/assembly.cpp index 6b2c3b8a151d..ec3d882fa5b7 100644 --- a/src/vm/assembly.cpp +++ b/src/vm/assembly.cpp @@ -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 } diff --git a/src/vm/comwaithandle.cpp b/src/vm/comwaithandle.cpp index 7d0b7b738dbe..4586cb7886df 100644 --- a/src/vm/comwaithandle.cpp +++ b/src/vm/comwaithandle.cpp @@ -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); diff --git a/tests/src/baseservices/threading/ApartmentState/SetApartmentStateDefaultMta.cs b/tests/src/baseservices/threading/ApartmentState/SetApartmentStateDefaultMta.cs new file mode 100644 index 000000000000..67216d192e84 --- /dev/null +++ b/tests/src/baseservices/threading/ApartmentState/SetApartmentStateDefaultMta.cs @@ -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; + } +} diff --git a/tests/src/baseservices/threading/ApartmentState/SetApartmentStateDefaultMta.csproj b/tests/src/baseservices/threading/ApartmentState/SetApartmentStateDefaultMta.csproj new file mode 100644 index 000000000000..c194bc083ac8 --- /dev/null +++ b/tests/src/baseservices/threading/ApartmentState/SetApartmentStateDefaultMta.csproj @@ -0,0 +1,19 @@ + + + + + Debug + AnyCPU + {0F4DD32F-EC7D-4AEF-9C6F-0C40E7CC7F2C} + Exe + + + + + + + + + + + diff --git a/tests/src/baseservices/threading/ApartmentState/SetApartmentStateMta.cs b/tests/src/baseservices/threading/ApartmentState/SetApartmentStateMta.cs new file mode 100644 index 000000000000..3a5df09ecfaa --- /dev/null +++ b/tests/src/baseservices/threading/ApartmentState/SetApartmentStateMta.cs @@ -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; + } +} diff --git a/tests/src/baseservices/threading/ApartmentState/SetApartmentStateMta.csproj b/tests/src/baseservices/threading/ApartmentState/SetApartmentStateMta.csproj new file mode 100644 index 000000000000..64b742c50ed5 --- /dev/null +++ b/tests/src/baseservices/threading/ApartmentState/SetApartmentStateMta.csproj @@ -0,0 +1,19 @@ + + + + + Debug + AnyCPU + {D5F2231F-6DEA-4DB2-AF79-120CABAED49B} + Exe + + + + + + + + + + + diff --git a/tests/src/baseservices/threading/ApartmentState/SetApartmentStateSta.cs b/tests/src/baseservices/threading/ApartmentState/SetApartmentStateSta.cs new file mode 100644 index 000000000000..d89c7f52360e --- /dev/null +++ b/tests/src/baseservices/threading/ApartmentState/SetApartmentStateSta.cs @@ -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; + } +} diff --git a/tests/src/baseservices/threading/ApartmentState/SetApartmentStateSta.csproj b/tests/src/baseservices/threading/ApartmentState/SetApartmentStateSta.csproj new file mode 100644 index 000000000000..d49f369f66b1 --- /dev/null +++ b/tests/src/baseservices/threading/ApartmentState/SetApartmentStateSta.csproj @@ -0,0 +1,19 @@ + + + + + Debug + AnyCPU + {6EE5C24D-CB8C-42AD-9D44-B49D584FB173} + Exe + + + + + + + + + + + diff --git a/tests/src/baseservices/threading/ApartmentState/WaitAllSta.cs b/tests/src/baseservices/threading/ApartmentState/WaitAllSta.cs new file mode 100644 index 000000000000..ccb327798484 --- /dev/null +++ b/tests/src/baseservices/threading/ApartmentState/WaitAllSta.cs @@ -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; + } +} diff --git a/tests/src/baseservices/threading/ApartmentState/WaitAllSta.csproj b/tests/src/baseservices/threading/ApartmentState/WaitAllSta.csproj new file mode 100644 index 000000000000..80a63ff13674 --- /dev/null +++ b/tests/src/baseservices/threading/ApartmentState/WaitAllSta.csproj @@ -0,0 +1,19 @@ + + + + + Debug + AnyCPU + {CEC647A3-9A72-42C6-8023-22670F63E7B2} + Exe + + + + + + + + + + + diff --git a/tests/testsUnsupportedOutsideWindows.txt b/tests/testsUnsupportedOutsideWindows.txt index d4928b20aae3..2aa488112654 100644 --- a/tests/testsUnsupportedOutsideWindows.txt +++ b/tests/testsUnsupportedOutsideWindows.txt @@ -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