Skip to content
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.

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

[Tiering] System.Threading.Thread CoreFX test ThreadTests.ApartmentState_NoAttributePresent_DefaultState_Windows fails with tiering #10810

Closed
kouvel opened this issue Jul 31, 2018 · 11 comments
Assignees
Milestone

Comments

@kouvel
Copy link
Member

kouvel commented Jul 31, 2018

The test fails with an xunit assertion failure with tiering in CoreFX repro with release build of CoreFX and test project, which I think runs against a release CoreCLR.

@dotnet/jit-contrib

@kouvel
Copy link
Member Author

kouvel commented Jul 31, 2018

Small repro used for debug coreclr + debug corefx test:

        var apartmentState = Thread.CurrentThread.GetApartmentState();
        if (apartmentState != ApartmentState.MTA)
        {
            Console.WriteLine($"Unexpected result (expected ApartmentState.MTA): {apartmentState}");
            return;
        }

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

        try
        {
            Thread.CurrentThread.SetApartmentState(ApartmentState.MTA);
        }
        catch (Exception ex)
        {
            Console.WriteLine($"Unexpected exception during 'Thread.CurrentThread.SetApartmentState(ApartmentState.MTA)': {ex}");
        }

kouvel referenced this issue in kouvel/corefx Jul 31, 2018
See https://github.com/dotnet/coreclr/issues/19225. This would fail in every CoreFX CI job in CoreCLR repo (which seem to be PR-triggered) after tiering is enabled by default.
kouvel referenced this issue in kouvel/corefx Jul 31, 2018
See https://github.com/dotnet/coreclr/issues/19225. This would fail in every CoreFX CI job in CoreCLR and CoreFX repos after tiering is enabled by default.
@kouvel
Copy link
Member Author

kouvel commented Jul 31, 2018

PR to temporarily disable test: dotnet/corefx#31523

@kouvel
Copy link
Member Author

kouvel commented Jul 31, 2018

Also, the test (small repro above and the System.Threading.Thread CoreFX tests) does not fail with only COMPlus_JITMinOpts=1, but it fails with COMPlus_TieredCompilation=1.

@RussKeldorph
Copy link
Contributor

@dotnet/jit-contrib

@AndyAyersMS
Copy link
Member

I can repro with the small example (say tiering vs minopts), and have done a bit of instrumentation in the runtime to see when it tries to get or set apartment state, but no real insights to offer yet.

I wonder if this is somehow tied into the observation I made the other day that tier0 is bypassed for some methods from System.Private.CoreLib, an this perhaps impacts the order of initialization or something -- that is with tiering we now run a mixture of prejitted, tier1 and tier0 code that doesn't match what we see with minopts, which only runs prejitted and tier0 code, or full opts, which only runs prejitted and tier1 code.

So perhaps there is some side effect the jit can cause via the jit interface that impacts the thread apartment state and this plays out differently in the mixed tier0/tier1 case than in the pure tier0 or pure tier1 cases. Or perhaps some initialization is done in a different order.

Will keep looking.

@AndyAyersMS
Copy link
Member

Here's a guess:

  • when tiering is enabled: tiering manager starts up a timer thread early, and this is set to be an MTA thread, and that implicitly makes the other threads (like the main thread) in the process MTA. Some in-between checks then think the main thread is already MTA and don't bother to try and set it to explicit MTA. Subsequently when the program sets the apartment state to STA this succeeds because the implicit MTA can be overridden.

  • when tiering is not enabled: the main thread is unknown state and eventually gets explicitly set to MTA. Then when the program tries to set it to STA this fails and we get an (expected) exception.

So some in-between bit of logic is possibly not being sufficiently careful to distinguish between implict MTA and explicit MTA.

At any rate this doesn't look like a jit issue so will retag as runtime-ish.

Could also be a test bug I suppose.

@kouvel @noahfalk hope this helps.

@kouvel
Copy link
Member Author

kouvel commented Aug 1, 2018

Ah ok, thanks @AndyAyersMS, I'll take a closer look and see how the apartment state of the main thread is being affected.

There was also an assertion failure in the JIT when using debug coreclr, it may be unrelated to the apartment state.

@AndyAyersMS
Copy link
Member

Ah I overlooked that part -- I'll try and repro the assertion failure.

@jkotas
Copy link
Member

jkotas commented Aug 1, 2018

when tiering is enabled: tiering manager starts up a timer thread early, and this is set to be an MTA thread, and that implicitly makes the other threads (like the main thread) in the process MTA. Some in-between checks then think the main thread is already MTA and don't bother to try and set it to explicit MTA

We may want to fix this by explicitly setting the main thread to MTA. It will also improve the compatibility with desktop. We have issue on it already: https://github.com/dotnet/coreclr/issues/17822

@AndyAyersMS
Copy link
Member

I can't repro the jit assert -- let me know if you still see this.

I tried running the System.Threading.Thread tests on a debug build of CoreFx at 724ffde with tiering enabled, using a checked jit from CoreCLR at c62976e.

@kouvel kouvel self-assigned this Aug 2, 2018
@kouvel
Copy link
Member Author

kouvel commented Aug 2, 2018

I'm also not able to repro the assertion failure after syncing to latest

kouvel referenced this issue in kouvel/coreclr Aug 9, 2018
Fixes 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)

Fixes https://github.com/dotnet/coreclr/issues/17787
- `WaitHandle.WaitAll` 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)

Fixes https://github.com/dotnet/coreclr/issues/19225
kouvel referenced this issue in kouvel/coreclr Aug 9, 2018
Fixes 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)

Fixes 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)

Fixes https://github.com/dotnet/coreclr/issues/19225
kouvel referenced this issue in kouvel/coreclr Aug 9, 2018
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
kouvel referenced this issue in kouvel/coreclr Aug 12, 2018
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
jkotas referenced this issue in dotnet/coreclr Aug 13, 2018
* 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

* Temporarily exclude invalid CoreFX test
kouvel referenced this issue in kouvel/coreclr Aug 16, 2018
Partial port of dotnet#19384 to 2.2

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/19225
kouvel referenced this issue in kouvel/coreclr Aug 16, 2018
Partial port of dotnet#19384 to 2.2

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/19225
kouvel referenced this issue in kouvel/coreclr Aug 20, 2018
Partial port of dotnet#19384 to 2.2

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/19225
kouvel referenced this issue in kouvel/coreclr Aug 22, 2018
Partial port of dotnet#19384 to 2.2

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/19225
kouvel referenced this issue in kouvel/coreclr Aug 22, 2018
Partial port of dotnet#19384 to 2.2

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/19225
kouvel referenced this issue in kouvel/coreclr Aug 24, 2018
Partial port of dotnet#19384 to 2.2

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/19225
kouvel referenced this issue in kouvel/coreclr Aug 27, 2018
Partial port of dotnet#19384 to 2.2

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/19225
@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the 3.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants