Is ConstructorInfo.GetParameters Thread-Safe?

Question

This is a very strange problem that I have spent the day trying to track down. I am not sure if this is a bug, but it would be great to get some perspective and thoughts on why this is happening.

I am using xUnit (2.0) to run my unit tests. The beauty of xUnit is that it automatically runs tests in parallel for you. The problem that I have found, however, is that Constructor.GetParameters appears to not be thread-safe when the ConstructorInfo is marked as being a thread-safe type. That is, if two threads reach Constructor.GetParameters at the same time, two results are produced, and subsequent calls to this method returns the second result that was created (regardless of the thread that calls it).

I have created some code to demonstrate this unexpected behavior (I also have it hosted on GitHub if you would like to download and try the project locally).

Here is the code:

public class OneClass
{
    readonly ITestOutputHelper output;

    public OneClass( ITestOutputHelper output )
    {
        this.output = output;
    }

    [Fact]
    public void OutputHashCode()
    {
        Support.Add( typeof(SampleObject).GetTypeInfo() );
        output.WriteLine( "Initialized:" );
        Support.Output( output );

        Support.Add( typeof(SampleObject).GetTypeInfo() );
        output.WriteLine( "After Initialized:" );
        Support.Output( output );
    }
}

public class AnotherClass
{
    readonly ITestOutputHelper output;

    public AnotherClass( ITestOutputHelper output )
    {
        this.output = output;
    }

    [Fact]
    public void OutputHashCode()
    {
        Support.Add( typeof(SampleObject).GetTypeInfo() );
        output.WriteLine( "Initialized:" );
        Support.Output( output );

        Support.Add( typeof(SampleObject).GetTypeInfo() );
        output.WriteLine( "After Initialized:" );
        Support.Output( output );
    }
}

public static class Support
{
    readonly static ICollection<int> Numbers = new List<int>();

    public static void Add( TypeInfo info )
    {
        var code = info.DeclaredConstructors.Single().GetParameters().Single().GetHashCode();
        Numbers.Add( code );
    }

    public static void Output( ITestOutputHelper output )
    {
        foreach ( var number in Numbers.ToArray() )
        {
            output.WriteLine( number.ToString() );
        }
    }
}

public class SampleObject
{
    public SampleObject( object parameter ) {}
}

The two test classes ensure that two threads are created and run in parallel. Upon running these tests, you should get results that look like the following:

Initialized:
39053774 <---- Different!
45653674
After Initialized:
39053774 <---- Different!
45653674
45653674
45653674

(NOTE: I've added "<---- Different!" to denote the unexpected value. You will not see this in the test results.)

As you can see, the result from the very first call to the GetParameters returns a different value than all subsequent calls.

I have had my nose in .NET for quite a while now, but have never seen anything quite like this. Is this expected behavior? Is there a preferred/known way of initializing the .NET type system so that this does not happen?

Finally, if anyone is interested, I ran into this problem while using xUnit with MEF 2, where a ParameterInfo being used as a key in a dictionary is not returning as equal to the ParameterInfo being passed in from a previously saved value. This, of course, results in unexpected behavior and resulting in failed tests when run concurrently.

EDIT: After some good feedback from the answers, I have (hopefully) clarified this question and scenario. The core of the issue is "Thread-Safety" of a "Thead-Safe" type, and acquiring better knowledge of what exactly this means.

ANSWER: This issue ended up being being due to several factors, one of which is due to me never-ending ignorance to multi-threaded scenarios, which it seems I am forever learning with no end for the foreseeable future. I am again appreciative of xUnit for being designed in such a way to learn this territory in such an effective manner.

The other issue does appear to be inconsistencies with how the .NET type system initializes. With the TypeInfo/Type, you get the same type/reference/hashcode no matter which thread accesses it however many times. For MemberInfo/MethodInfo/ParameterInfo, this is not the case. Thread access beware.

Finally, it seems I am not the only person with this confusion and this has indeed been recognized as an invalid assumption on a submitted issue to .NET Core's GitHub repository.

So, problem solved, mostly. I would like to send my thanks and appreciation to all involved in dealing with my ignorance in this matter, and helping me learn (what I am finding is) this very complex problem space.

Ortolan

And that is a problem? That you have two different instances of a class with the same values? – Roll

Correct. It is one instance on the first call, and then another instance on every subsequent call. So, one thread will get one version on the first call, and then each thread will get another (unchanging instance on each subsequent call. If I am using that first call to store a key (as in the example above with MEF2), then yes, that is a problem. :) – Ortolan

Answer 1

It is one instance on the first call, and then another instance on every subsequent call.

OK, that's fine. A bit weird, but the method is not documented as always returning the same instance every time.

So, one thread will get one version on the first call, and then each thread will get another (unchanging instance on each subsequent call.

Again, weird, but perfectly legal.

Is this expected behavior?

Well, I would not have expected it before your experiment. But after your experiment, yes, I expect that behaviour to continue.

Is there a preferred/known way of initializing the .NET type system so that this does not happen?

Not to my knowledge.

If I am using that first call to store a key then yes, that is a problem.

Then you have evidence that you should stop doing that. If it hurts when you do that, don't do it.

A ParameterInfo reference should always represent the same ParameterInfo reference regardless of the thread it is on or how many times accessed.

That's a moral statement about how the feature should have been designed. It's not how it was designed, and its clearly not how it was implemented. You can certainly make the argument that the design is bad.

Mr. Lippert is also right in that the documentation does not guarantee/specify this, but this has always been my expectation of and experience with this behavior until this point.

Past performance is no guarantee of future results; your experience was not sufficiently varied until now. Multithreading has a way of confounding people's expectations! A world where memory is constantly changing unless something is keeping it still is contrary to our normal mode of things being the same until something changes them.

Chiliarch

Two things: First, it's not the array that's producing a HashCode: It's a ParameterInfo, which I don't think can be mutated. Second, and more importantly, I think it's reasonable to suppose that Equals() would either consistently return true or consistently return false for two ParameterInfo objects that represent the same parameter. Am I missing something? – Docent

Thank you @Eric-Lippert for your reply. Please note that this is not me who is using this per se, but MEF 2 (System.Composition) as I noted above and why I have spent the day tracking this down. It is indeed my (and MEF 2's) expectation/understanding that each and every call to any System.Reflection element return the same reference for the queried object. In the example above, this expectation is indeed met with each and every call with the exception of the first call. In addition to adding to my knowledge of System.Reflection, System.Composition is about to get a new issue in GitHub. :P – Ortolan

@Mike-EEE: Sounds like MEF2 has a bug then! Nice find. – Chiliarch

Hm, how can you compare ParameterInfos if not through object identity? No Equals on ParameterInfo or on RuntimeParameterInfo. It might not be documented to be the case but I believe it has been the intention to absolutely ensure object uniqueness. Seems like an important use case. – Macro

Agreed with both @Macro (and StriplingWarrior). A ParameterInfo reference should always represent the same ParameterInfo reference regardless of the thread it is on or how many times accessed. Mr. Lippert is also right in that the documentation does not guarantee/specify this, but this has always been my expectation of and experience with this behavior until this point. So, it is either a bug in MEF2's usage, or (I hate to even suggest!) a latent bug/unexpected design in the framework code. I have opened a GitHub issue here for further discussion: github.com/dotnet/corefx/issues/6857 – Ortolan

@Eric-Lippert upon my own investigation I will have to clarify/quality your statement here that while the documentation does indeed not explicitly specify that the GetParameters method returns the same items, both ConstructorInfo and ParameterInfo classes are denoted in documentation as being Thread-Safe. Which, from my understanding, means it and all of its properties/methods/members are protected from race condition scenarios exactly such as this. It would be great to hear from you if I have this misunderstood and/or some further context on what this means. – Ortolan

@Mike-EEE: This has nothing to do with thread safety. The implementation can hand out a new object every time it wants even on a single thread. That it chooses not to in some circumstances does not make it unsafe. A class that meets the same contract when called on multiple threads as one thread is thread safe; you just don't like the contract. – Chiliarch

@EricLippert (SO is saying that is how to tag you? Apologies if that is not the case!) I am 100% in agreement with your analysis/commentary on multi-threading. It has completely rocked my world and I have learned so much (and am still learning -- obviously!), and I am grateful that xUnit is designed in such a way. However, I still see this as a thread-safety issue. Can you expound on what you mean by "A class that meets the same contract when called on"? Also, I feel this is getting chatty so please feel free to open a chat over this, too. :) Thank you! – Ortolan

The contract is what the object promises to do in exchange for you calling it properly. The contract is you ask for a parameter info, you get an object that has that information in it, you can ask from any thread, and that's the extent of the contract. If sometimes you get more than was promised -- referential integrity was not promised -- then that is a bonus, not a part of the contract. – Chiliarch

@Mike-EEE: Thread safety contracts deal with things like memory corruption. Suppose you write the long 0x00000000DEADBEEF to a field on one thread and write 0xBAADF00D00000000 to the same long on another thread. The contract here is that when you read the long, you'll get one of those two values out, provided that you did appropriate interlocked operations on the long. If you did not do those operations then your side of the contract is not met, so the runtime does not have to hold up its side of the contract. It can give you back 0xBAADF00DDEADBEEF if it likes, and it sometimes will! – Chiliarch

@EricLippert That's on a 32 bit build, no? On a 64 bit build writes to a long would be atomic, and wouldn't get shredded like that. (Not that I'd recommend that practice.) – Convulsive

@Servy: the 64 bit CLR only guarantees atomic access to 64 bit quantities when they are aligned. Are fields always aligned? I don't recall. As one of my former managers used to say "care to bet your car on whether that code is threadsafe?" I'm not a betting man. – Chiliarch

@EricLippert Sure; like I said, I wouldn't do that either. And of course the 32 bit version isn't a safe write on a 64 bit system, if memory serves, so unless you're prepared to re-write all of your logic any time you build the program for a different platform, not depending on any of the build-specific behaviors is going to be a good idea, at least if you can afford to. – Convulsive

Well it is safe to say I have been taken to school here. Thank you again for your patience and time @EricLippert in helping explain some fundamentals here as I endure some growing (learning) pains with this topic. I have updated my question with additional information and context on my own personal answer as well. – Ortolan

Answer 2

As an answer, I am looking at the .NET sources and the ConstructorInfo class has this in its bowels:

private ParameterInfo[] m_parameters = null; // Created lazily when GetParameters() is called.

That's their comment, not mine. Let's see GetParameters:

[System.Security.SecuritySafeCritical]  // auto-generated
internal override ParameterInfo[] GetParametersNoCopy()
{
    if (m_parameters == null)
        m_parameters = RuntimeParameterInfo.GetParameters(this, this, Signature);

    return m_parameters;
}

[Pure]
public override ParameterInfo[] GetParameters()
{
    ParameterInfo[] parameters = GetParametersNoCopy();

    if (parameters.Length == 0)
        return parameters;

    ParameterInfo[] ret = new ParameterInfo[parameters.Length];
    Array.Copy(parameters, ret, parameters.Length);
    return ret;
}

So no locking, nothing that would prevent the m_parameters being overridden by a racing thread.

Update: Here is the relevant code inside GetParameters: args[position] = new RuntimeParameterInfo(sig, scope, tkParamDef, position, attr, member); It is clear that in this case RuntimeParameterInfo is just a container for the parameters given in its constructor. There was never even the intention to get the same instance.

That is unlike TypeInfo, which is inheriting from Type and also implementing IReflectableType and which for its GetTypeInfo method just returns itself as an IReflectableType, so maintaining the same instance of the type.

Roll

Right, but it is not the parameters array that is being hashed here -- I too was momentarily confused by this. It is the contents of the parameters array that are being hashed. But I'll bet you that the parameter info generation code similarly has lazy logic in it that is not race proof. And there's no reason for it to be race-proof; the worst that happens is that you get two instances that have the same content and one is orphaned. – Chiliarch

Regardless of the exact mechanism of the behaviour though, clearly the Reflection engine does not make any promises about the referential identity of the objects it hands back, so depending on that identity is a bad idea. – Chiliarch

The issue here from my (and apparently MEF2's) view is that in a single-threaded scenario, you can depend on the referential integrity of reflection objects 100%. To me, this does seem like an oversight in the framework, but what do I know. That is why I am here and asking questions to find out for sure. :) – Ortolan

Additionally, in the documentation ConstructorInfo is marked as thread safe, so it and all of its members should be protecting its resources and ensuring thread safety, correct? – Ortolan

@Mike-EEE No, you can't depend on it, because it's not a documented behavior. The runtime is free to change that behavior at any time. The fact that a given implementation would happen to only return a single instance in a single-threaded context doesn't mean that that's something you can rely on. – Convulsive

OK I am clearly being taken to school here. Looks like I am battling my own ignorance of this matter and also inconsistent behavior that Siderite is highlighting in this answer. That is, it appears that Type/TypeInfo maintain thread-safe referential integrity (is that the term I am looking for?) while ParameterInfo does not (even though it does after the first completed call -- to make it all the more confusing!). – Ortolan