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

STAThread attribute is not respected in netcoreapp #13688

Closed
tannergooding opened this Issue Aug 30, 2017 · 23 comments

Comments

Projects
None yet
4 participants
@tannergooding
Member

tannergooding commented Aug 30, 2017

Create a new netcoreapp2.0 project and set the Program.cs file to:

using System;
using System.Threading;

namespace ConsoleApp4
{
    class Program
    {
        [STAThread]
        static void Main(string[] args)
        {
            Console.WriteLine(Thread.CurrentThread.GetApartmentState());
            Console.ReadLine();
        }
    }
}

Examine that the output from the application is:

MTA

Not respecting STAThread means that users who depend on it (even if it really only matters on Windows), have to manually spawn a thread to run their code.

In some cases, certain libraries actually check and enforce that the calling thread is STA, and will throw an exception if it is not.

@tannergooding

This comment has been minimized.

Show comment
Hide comment
@tannergooding

tannergooding Aug 30, 2017

Member

Actually, this might be a fault in the dotnet host for @dotnet/dotnet-cli

Member

tannergooding commented Aug 30, 2017

Actually, this might be a fault in the dotnet host for @dotnet/dotnet-cli

@tannergooding

This comment has been minimized.

Show comment
Hide comment
@tannergooding

tannergooding Aug 30, 2017

Member

Closing. This is an issue with the dotnet host: dotnet/core-setup#3127

Member

tannergooding commented Aug 30, 2017

Closing. This is an issue with the dotnet host: dotnet/core-setup#3127

@tannergooding

This comment has been minimized.

Show comment
Hide comment
@tannergooding

tannergooding Aug 30, 2017

Member

Actually, reopening. This is in CoreCLR after all.

The code to get the threading model for the entry point still exists, it just isn't called anywhere: GetEntryPointThreadAptState

Member

tannergooding commented Aug 30, 2017

Actually, reopening. This is in CoreCLR after all.

The code to get the threading model for the entry point still exists, it just isn't called anywhere: GetEntryPointThreadAptState

@tannergooding tannergooding reopened this Aug 30, 2017

@danmosemsft danmosemsft added this to the 2.0.x milestone Sep 1, 2017

@danmosemsft danmosemsft added the area-VM label Sep 1, 2017

@danmosemsft

This comment has been minimized.

Show comment
Hide comment
@danmosemsft

danmosemsft Sep 1, 2017

Member

@jkotas is there any technical reason for this? (If so we can remove the code). Right now I can run any .NET Core thread as an STA except the main thread.

Marking 2.0.x for now as a possible servicing candidate. This is a discrepancy with Desktop that could cause compatibility issues as some library code does require running on STA.

Member

danmosemsft commented Sep 1, 2017

@jkotas is there any technical reason for this? (If so we can remove the code). Right now I can run any .NET Core thread as an STA except the main thread.

Marking 2.0.x for now as a possible servicing candidate. This is a discrepancy with Desktop that could cause compatibility issues as some library code does require running on STA.

@tannergooding

This comment has been minimized.

Show comment
Hide comment
@tannergooding

tannergooding Sep 1, 2017

Member

There are several cases, at least on Windows, where you may require STA thread and additionally require that CoInitializeEx(null, COINIT_APARTMENTTHREADED) was run.

On Linux/Mac this is likely not important/relevant, except for corner cases, but it greatly simplifies existing managed code that throws if the calling thread is not STA, due to any requirements set forth by Windows.

Member

tannergooding commented Sep 1, 2017

There are several cases, at least on Windows, where you may require STA thread and additionally require that CoInitializeEx(null, COINIT_APARTMENTTHREADED) was run.

On Linux/Mac this is likely not important/relevant, except for corner cases, but it greatly simplifies existing managed code that throws if the calling thread is not STA, due to any requirements set forth by Windows.

@jkotas jkotas added area-Interop and removed area-VM labels Sep 1, 2017

@jkotas

This comment has been minimized.

Show comment
Hide comment
@jkotas

jkotas Sep 1, 2017

Member

Note that you can trivially work around this by just setting the apartment mode programmatically:

using System;
using System.Threading;

class Program
{
    static void Main(string[] args)
    {
        Thread.CurrentThread.SetApartmentState(ApartmentState.STA);
        Console.WriteLine(Thread.CurrentThread.GetApartmentState());
    }
}

Adding back the code to check for the attribute and making the call for you should be fine. It is pretty trivial logic.

However, I do not think it is a good candidate for 2.0.x servicing because of it is potentially breaking. There may be apps that have STAThread on the main method by accident (the attribute on Main method was part of templates in the past), they are perfectly happy running with main MTA thread, and switching the main thread to STA would break them or regress their performance significantly.

Member

jkotas commented Sep 1, 2017

Note that you can trivially work around this by just setting the apartment mode programmatically:

using System;
using System.Threading;

class Program
{
    static void Main(string[] args)
    {
        Thread.CurrentThread.SetApartmentState(ApartmentState.STA);
        Console.WriteLine(Thread.CurrentThread.GetApartmentState());
    }
}

Adding back the code to check for the attribute and making the call for you should be fine. It is pretty trivial logic.

However, I do not think it is a good candidate for 2.0.x servicing because of it is potentially breaking. There may be apps that have STAThread on the main method by accident (the attribute on Main method was part of templates in the past), they are perfectly happy running with main MTA thread, and switching the main thread to STA would break them or regress their performance significantly.

@jkotas

This comment has been minimized.

Show comment
Hide comment
@jkotas
Member

jkotas commented Sep 1, 2017

@tannergooding

This comment has been minimized.

Show comment
Hide comment
@tannergooding

tannergooding Sep 1, 2017

Member

@jkotas was something changed in Thread.SetApartmentState? It is currently documented as (and as far as I am aware, has always behaved as) throwing a ThreadStateException if the thread has already started.

Member

tannergooding commented Sep 1, 2017

@jkotas was something changed in Thread.SetApartmentState? It is currently documented as (and as far as I am aware, has always behaved as) throwing a ThreadStateException if the thread has already started.

@tannergooding

This comment has been minimized.

Show comment
Hide comment
@tannergooding

tannergooding Sep 2, 2017

Member

Okay, it appears as though you can do that code, but only in .netcore.

I would think that is a secondary bug that is an extension of this bug. Because we are not respecting STAThread or MTAThread (or lack-there-of), we are also not properly initializing the apartment state of the entry thread at all.

This is not only breaking any assumptions users may have around the STAThread and the MTAThread attributes, which have their own contracts, it is also breaking the contract of Thread.SetApartmentState and Thread.GetApartmentState

Member

tannergooding commented Sep 2, 2017

Okay, it appears as though you can do that code, but only in .netcore.

I would think that is a secondary bug that is an extension of this bug. Because we are not respecting STAThread or MTAThread (or lack-there-of), we are also not properly initializing the apartment state of the entry thread at all.

This is not only breaking any assumptions users may have around the STAThread and the MTAThread attributes, which have their own contracts, it is also breaking the contract of Thread.SetApartmentState and Thread.GetApartmentState

@tannergooding

This comment has been minimized.

Show comment
Hide comment
@tannergooding

tannergooding Sep 2, 2017

Member

On Windows, this can also have other consequences.

If the user is doing any kind of COM interop (and this includes things like DirectX), a call that would normally synchronize to a specific thread will no longer do so.

It also means that messages may not continue being pumped since COM does not do this automatically for multi-threaded apartments.

Member

tannergooding commented Sep 2, 2017

On Windows, this can also have other consequences.

If the user is doing any kind of COM interop (and this includes things like DirectX), a call that would normally synchronize to a specific thread will no longer do so.

It also means that messages may not continue being pumped since COM does not do this automatically for multi-threaded apartments.

@jkotas

This comment has been minimized.

Show comment
Hide comment
@jkotas

jkotas Sep 2, 2017

Member

throwing a ThreadStateException if the thread has already started

This is not quite correct. SetThreadApartmentState will throw exception only if COM appartment of the underlying OS thread was initialized to a different value than what is getting requested. E.g. you can create thread by PInvoking Win32 CreateThread API and then call SetAppartmentState with either STA or MTA on it just fine.

As I have said, fixing the bug with the attribute not being respected is fine with me (not in servicing release because of it is potentially breaking).

But I would not change .NET Core to force initilialize the main thread to MTA by default like it is done in .NET Framework. It is even more potentially breaking, will have a good workaround (use the attribute), and the full .NET Framework behavior is really just a side-effect of the complex startup needing to initialize COM to do what it needs to do. .NET Core startup is much simpler and it does not need COM initialized to do what it needs to do.

Member

jkotas commented Sep 2, 2017

throwing a ThreadStateException if the thread has already started

This is not quite correct. SetThreadApartmentState will throw exception only if COM appartment of the underlying OS thread was initialized to a different value than what is getting requested. E.g. you can create thread by PInvoking Win32 CreateThread API and then call SetAppartmentState with either STA or MTA on it just fine.

As I have said, fixing the bug with the attribute not being respected is fine with me (not in servicing release because of it is potentially breaking).

But I would not change .NET Core to force initilialize the main thread to MTA by default like it is done in .NET Framework. It is even more potentially breaking, will have a good workaround (use the attribute), and the full .NET Framework behavior is really just a side-effect of the complex startup needing to initialize COM to do what it needs to do. .NET Core startup is much simpler and it does not need COM initialized to do what it needs to do.

@tannergooding

This comment has been minimized.

Show comment
Hide comment
@tannergooding

tannergooding Sep 2, 2017

Member

I would think the following should happen then:

  1. Fix all the documentation to indicate the behavior for .NETCore
  2. If STAThread or MTAThread are explicitly specified, configure the threading model for the main thread; otherwise, do nothing (however, see comment below)
  3. For the main thread, if uninitialized, we should return ApartmentState.Unknown. Today, we are returning ApartmentState.MTA (we should likely do this for any call to GetApartmentState() if it is not otherwise set).

It seems as though all threads that are not the Main thread are explicitly configured to be MTA, if the apartment state was not set before-hand. This means that the Main thread now has "magic" behavior for apartment state configuration and is the only thread (outside of using P/Invoke, etc) that will not have the apartment state set by the time it is started.

I think we should have all threads treated the same. That is, either all should be initialized to MTA by default or they should all be initialized to 'Unknown' by default and the first SetApartmentState call configures the threading model for that thread

Member

tannergooding commented Sep 2, 2017

I would think the following should happen then:

  1. Fix all the documentation to indicate the behavior for .NETCore
  2. If STAThread or MTAThread are explicitly specified, configure the threading model for the main thread; otherwise, do nothing (however, see comment below)
  3. For the main thread, if uninitialized, we should return ApartmentState.Unknown. Today, we are returning ApartmentState.MTA (we should likely do this for any call to GetApartmentState() if it is not otherwise set).

It seems as though all threads that are not the Main thread are explicitly configured to be MTA, if the apartment state was not set before-hand. This means that the Main thread now has "magic" behavior for apartment state configuration and is the only thread (outside of using P/Invoke, etc) that will not have the apartment state set by the time it is started.

I think we should have all threads treated the same. That is, either all should be initialized to MTA by default or they should all be initialized to 'Unknown' by default and the first SetApartmentState call configures the threading model for that thread

@danmosemsft

This comment has been minimized.

Show comment
Hide comment
@danmosemsft

danmosemsft Nov 30, 2017

Member

@tannergooding this should be marked 2.1 .. as Jan pointed out we would not take it in servicing. LMK if you are expecting in servicing.

Member

danmosemsft commented Nov 30, 2017

@tannergooding this should be marked 2.1 .. as Jan pointed out we would not take it in servicing. LMK if you are expecting in servicing.

@danmosemsft danmosemsft modified the milestones: 2.0.x, 2.1.0 Nov 30, 2017

@danmosemsft danmosemsft assigned Anipik and unassigned Anipik Nov 30, 2017

@danmosemsft

This comment has been minimized.

Show comment
Hide comment
@danmosemsft

danmosemsft Nov 30, 2017

Member

Re respecting the [STAThread] attribute -- the relevant code seems to be this (in full FX) appdomain.cpp in SystemDomain::ExecuteMainMethod

        Thread::ApartmentState state = Thread::AS_Unknown;        

        if((!IsNilToken(tkEntryPoint)) && (TypeFromToken(tkEntryPoint) == mdtMethodDef)) {
            if (scope->IsValidToken(tkEntryPoint))
                state = SystemDomain::GetEntryPointThreadAptState(scope, tkEntryPoint);
            else
                ThrowHR(COR_E_BADIMAGEFORMAT);
        }

        // 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(scope, state);

In CoreCLR this method does not exist and the correct place to put this code looks like Assembly::ExecuteMainMethod.

Member

danmosemsft commented Nov 30, 2017

Re respecting the [STAThread] attribute -- the relevant code seems to be this (in full FX) appdomain.cpp in SystemDomain::ExecuteMainMethod

        Thread::ApartmentState state = Thread::AS_Unknown;        

        if((!IsNilToken(tkEntryPoint)) && (TypeFromToken(tkEntryPoint) == mdtMethodDef)) {
            if (scope->IsValidToken(tkEntryPoint))
                state = SystemDomain::GetEntryPointThreadAptState(scope, tkEntryPoint);
            else
                ThrowHR(COR_E_BADIMAGEFORMAT);
        }

        // 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(scope, state);

In CoreCLR this method does not exist and the correct place to put this code looks like Assembly::ExecuteMainMethod.

@danmosemsft

This comment has been minimized.

Show comment
Hide comment
@danmosemsft

danmosemsft Nov 30, 2017

Member

Currently:

Main thread:

  • On Core, starts off uninitialized, but GetApartmentState returns MTA (should be Unknown). Because it's uninitialized, you can successfully SetApartmentState to STA or MTA and that initializes it.
  • On Full FX, starts off as specified in attribute, else MTA. GAS returns the value accurately and because it is already initialized you can't SAS to a different value.

Secondary threads:

  • On both platforms, GAS accurately reports Unknown. You can set to MTA or STA to intialize it, until you have started the thread. [update: after the thread starts, it returns MTA unless you requested STA]

It seems as though all threads that are not the Main thread are explicitly configured to be MTA, if the apartment state was not set before-hand.

@tannergooding I don't reproduce the above on either platform. Code I used:

            Thread t = new Thread(new ThreadStart(() => { }));
            Console.WriteLine(t.GetApartmentState()); // Returns Unknown
            t.SetApartmentState(ApartmentState.STA); // Succeeds - same if you do MTA
            Console.WriteLine(t.GetApartmentState()); // Returns what was set above

So I think there are these

  1. Fix Core to respect the attribute if present.
  2. Fix Core to accurately return ApartmentState.Unknown on the main thread if there is no attribute.
  3. Fix docs to indicate that Core defaults to uninitialized on main thread

@jkotas @tannergooding do you agree with this?

Member

danmosemsft commented Nov 30, 2017

Currently:

Main thread:

  • On Core, starts off uninitialized, but GetApartmentState returns MTA (should be Unknown). Because it's uninitialized, you can successfully SetApartmentState to STA or MTA and that initializes it.
  • On Full FX, starts off as specified in attribute, else MTA. GAS returns the value accurately and because it is already initialized you can't SAS to a different value.

Secondary threads:

  • On both platforms, GAS accurately reports Unknown. You can set to MTA or STA to intialize it, until you have started the thread. [update: after the thread starts, it returns MTA unless you requested STA]

It seems as though all threads that are not the Main thread are explicitly configured to be MTA, if the apartment state was not set before-hand.

@tannergooding I don't reproduce the above on either platform. Code I used:

            Thread t = new Thread(new ThreadStart(() => { }));
            Console.WriteLine(t.GetApartmentState()); // Returns Unknown
            t.SetApartmentState(ApartmentState.STA); // Succeeds - same if you do MTA
            Console.WriteLine(t.GetApartmentState()); // Returns what was set above

So I think there are these

  1. Fix Core to respect the attribute if present.
  2. Fix Core to accurately return ApartmentState.Unknown on the main thread if there is no attribute.
  3. Fix docs to indicate that Core defaults to uninitialized on main thread

@jkotas @tannergooding do you agree with this?

@tannergooding

This comment has been minimized.

Show comment
Hide comment
@tannergooding

tannergooding Nov 30, 2017

Member

I don't reproduce the above on either platform.

You aren't actually starting the thread.

Try:

Thread t = new Thread(new ThreadStart(() => { while (true) ; }));
Console.WriteLine(t.GetApartmentState());

t.Start();

Console.WriteLine(t.GetApartmentState());
t.SetApartmentState(ApartmentState.STA);

It will print:

Unknown
MTA
<System.Threading.ThreadStateException>
Member

tannergooding commented Nov 30, 2017

I don't reproduce the above on either platform.

You aren't actually starting the thread.

Try:

Thread t = new Thread(new ThreadStart(() => { while (true) ; }));
Console.WriteLine(t.GetApartmentState());

t.Start();

Console.WriteLine(t.GetApartmentState());
t.SetApartmentState(ApartmentState.STA);

It will print:

Unknown
MTA
<System.Threading.ThreadStateException>
@tannergooding

This comment has been minimized.

Show comment
Hide comment
@tannergooding

tannergooding Nov 30, 2017

Member

The approach to fixing it sounds reasonable to me. I am guessing making it mirror Desktop would be too breaking at this point?

We should ensure the documents are accurately updated as well.

Member

tannergooding commented Nov 30, 2017

The approach to fixing it sounds reasonable to me. I am guessing making it mirror Desktop would be too breaking at this point?

We should ensure the documents are accurately updated as well.

@danmosemsft

This comment has been minimized.

Show comment
Hide comment
@danmosemsft

danmosemsft Nov 30, 2017

Member

@tannergooding ah I see. So Unknown should be interpreted as "not CoInitialized" rather than "thread will remain not CoInitialized unless told otherwise". Can't change that now.

Re mirroring desktop, which part -- we should make the attributes work, but per @jkotas above, we don't want to automatically initialize the main thread to MTA in the absence of any attributes. There are no other discrepancies right?

Member

danmosemsft commented Nov 30, 2017

@tannergooding ah I see. So Unknown should be interpreted as "not CoInitialized" rather than "thread will remain not CoInitialized unless told otherwise". Can't change that now.

Re mirroring desktop, which part -- we should make the attributes work, but per @jkotas above, we don't want to automatically initialize the main thread to MTA in the absence of any attributes. There are no other discrepancies right?

@tannergooding

This comment has been minimized.

Show comment
Hide comment
@tannergooding

tannergooding Nov 30, 2017

Member

The Main thread, on Desktop, is initialized to MTA if there are no attributes. This matches the behavior of additional threads the user creates on both Desktop and Core (when they don't manually call SetApartmentState before calling Start).

As such, I think having Core mimic that makes the most sense (not only does it match Desktop, but it also matches any other thread they explicitly create). However, I understand if there are back-compat concerns that might prevent that.

Member

tannergooding commented Nov 30, 2017

The Main thread, on Desktop, is initialized to MTA if there are no attributes. This matches the behavior of additional threads the user creates on both Desktop and Core (when they don't manually call SetApartmentState before calling Start).

As such, I think having Core mimic that makes the most sense (not only does it match Desktop, but it also matches any other thread they explicitly create). However, I understand if there are back-compat concerns that might prevent that.

@danmosemsft

This comment has been minimized.

Show comment
Hide comment
@danmosemsft

danmosemsft Nov 30, 2017

Member

Right.

Member

danmosemsft commented Nov 30, 2017

Right.

@danmosemsft

This comment has been minimized.

Show comment
Hide comment
@danmosemsft

danmosemsft Nov 30, 2017

Member

@russellhadley I think you own vm* - mind if @Anipik does this one as a learning exercise?

Member

danmosemsft commented Nov 30, 2017

@russellhadley I think you own vm* - mind if @Anipik does this one as a learning exercise?

@jkotas

This comment has been minimized.

Show comment
Hide comment
@jkotas

jkotas Dec 21, 2017

Member

The fix was causing intermittent test failures - reverted in #15586 .

@Anipik Let me know if you need help investigating the intermittent test failures. Since the failures are intermittent, you will probably need to run the affected tests in a loop to reproduce them.

Member

jkotas commented Dec 21, 2017

The fix was causing intermittent test failures - reverted in #15586 .

@Anipik Let me know if you need help investigating the intermittent test failures. Since the failures are intermittent, you will probably need to run the affected tests in a loop to reproduce them.

@Anipik

This comment has been minimized.

Show comment
Hide comment
@Anipik

Anipik Dec 21, 2017

Member

I will try to reproduce it on my machine by running them in a loop to get more info about the cause of the failure, then after that we can discuss how to resolve it.

Member

Anipik commented Dec 21, 2017

I will try to reproduce it on my machine by running them in a loop to get more info about the cause of the failure, then after that we can discuss how to resolve it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment