System.Composition: Bug in Resolving Activators Due to the .NET Type System

[Mike-E-angelo]

I have been tracking down a pretty nasty bug today in regards to System.Composition. After a discussion about this on StackOverflow, I have decided to open an issue here in hopes to get additional conversation/context/insight around this.

Essentially, there appears to be a race condition when accessing System.Reflection elements for the first time from different threads. In the case of System.Composition, it relies on a ParameterInfo object when determining an activator to use for a dependency.

Due to the issue as explained above in StackOverflow, the ParameterInfo object may or may not be the expected key at this point, since the stored ParameterInfo in the CompositionDependency.Site.Parameter may or may not be the same object that gets returned after the value has been initialized and cached in the System.Reflection system.

[dsplaisted]

It looks like a fix may involve using the Position property of the ParameterInfo as the lookup key instead of the ParameterInfo itself in DiscoveredPart.GetActivator.

It would also be good to see if there are other places in the System.Composition code that are making similarly invalid assumptions.

FYI @nblumhardt

[GSPP]

Is ParameterInfo not intended to be a unique object like Type instances are supposed to be?

[Mike-E-angelo]

Excellent question @GSPP and exactly what caused me to ask this question on StackOverflow that ultimately got this issue created.

The thing is... ParameterInfo and MemberInfo objects are unique in single-threaded scenarios, and even in multi-threaded scenarios, but only after they have properly initialized. If they have not finished initializing and another thread accesses them, then that is when you get funky/inconsistent behavior and references.

To me, it seems as if the framework would protect against this scenario (being that is what I would interpret "thread-safe" as meaning), but I am a total newb w/ the multithreading realm so there must be something here that I do not understand as of yet.

It would be great it hear someone with great knowledge of this area expound upon this. I wanted to have a conversation around this w/ StackOverflow, but they are how shall we say, economic with the chatter there. 😄

[nblumhardt]

Nice catch! I'd assumed value equality would be handled in the memberinfo types, but obviously not :-)

It's been a while since I touched the code so no immediate recollection of where else this might occur. Let me know if any help's needed with the fix.

[Mike-E-angelo]

Thanks @nblumhardt. You assumed as such (as did I -- and I am sure a whole lot of others!) because it is indeed equal for all single-threaded scenarios and even multi-threaded scenarios granted the MemberInfo items you are accessing have been properly thread-initialized ala above (I probably didn't explain it well enough).

While this is a bug in System.Composition, I am still haunted/bothered by the fact that this might be a general bug in the framework type system. Again, it would be great to get someone who has better knowledge around this to explain/confirm in greater detail.

To recap:

TypeInfo/Type: Same reference, all the time, no matter the thread (works perfectly).
MemberInfo/ParameterInfo Depends on thread race.

This seems inconsistent at best, buggy at worst. From a newb's perspective, at least. 😄

[dsplaisted]

@pallavit Is the behavior encountered here expected, where in multithreaded scenarios the ParameterInfos you get from different calls to GetParameters() may not compare as equal or have the same hash code?

[pallavit]

Adding @nguerrera and @atsushikan who might know a little bit more about it.

[GSPP]

ParameterInfo does not have any Equals or GetHashCode implementation. It is not clear how to even check for equality. This suggests reference equality semantics. Therefore I think the best solution is to ensure uniqueness at the .NET BCL level.

[ghost]

It appears the original issue has been fixed in System.Composition by removing the assumption that ParameterInfo is a valid key. The caching of reflection objects such as ParameterInfo are driven by performance and memory consumption desires, not as a promise that it can be used to establish semantic identity.

The question of whether Reflection should offer a more complete "comparison" story is an interesting one but it's not an RTM issue.

[Mike-E-angelo]

Great... thank you for for the explanation, @atsushikan. I might be missing something, but the code file/line that I referenced still appears to be using the ParameterInfo still...

[ghost]

Oh yes, my mind skipped a step there. The bug in Composition has been acknowledged, but not yet fixed. Too early to close the issue.

[Mike-E-angelo]

Whew... sanity restored.... for now. 😄 Are you thinking this System.Composition fix will make it into RTM? If neither issues will be addressed for RTM, it would be great to somehow see if the deeper issue/conversation could be resolved for a future release, and what that might look like.

[ghost]

The target for the System.Composition fix is RTM for now.

The bigger issue about comparing Reflection objects - I'm pretty sure that won't make RTM as there a lot of tradeoffs and conflicting interests there.

It'd be a good Issue for a Future milestone.

[Mike-E-angelo]

OK... sounds good. How does one create an issue for a future milestone? :)

[ghost]

There’s already a “Future” selection under the “Milestone” dropdown box ☺

[Mike-E-angelo]

Hmmm... I am missing something, then. This is what I see under milestone:

I am also very much a GitHub newb. :P Is it possible to simply clone this issue under the Future milestone?

[ghost]

I can open the issue when I get a free moment.

[ghost]

Ok, I’ve opened https://github.com/dotnet/corefx/issues/8130. Maybe the “Future” milestone is only visible to MS people – I’m not sure.

[ghost]

@dsplaisted - do you own System.Composition? This is no longer a Reflection issue and I know nothing about Composition.

MemberInfos and ParameterInfos are not guaranteed to be unique instances. They cannot be used as Dictionary keys.

[karelz]

@AlexGhiondea can you please help as area expert/owner? I have never seen the code base before, so I can't comment.

In general: I would recommend to split it into 2 parts/phases or even 2 bugs - one specific and one "find similar instances".

[AlexGhiondea]

@jswolf19 I think your proposed solution should work. Please submit a PR :).

As @karelz mentioned, let's split this into 2 work items - fix the issue we know of (and add tests) and then look for other places that might be impacted.

[jswolf19]

@AlexGhiondea Ok, I'll get to work on that. I'll go the IComparer route because it's more robust, unless you think changing the key is better.

[jswolf19]

@AlexGhiondea As far as the testing, ideally, I want to (consistently) cause the two calls to GetParameterInfo() to return different references.

The first obstacle I have is that the class I really want to test is internal. I can write a test that goes through the place I want to change, but, the only ways I can see to check if I successfully caused the race condition to occur are to have access to the class itself or via debugging (some sort of profiler).

The second obstacle I have is with the consistency of the test. To achieve consistency, I could use reflection to reset the RuntimeConstructorInfo.m_parameters to null between the GetParameterInfo() calls which does not seem ideal except from a consistency standpoint. Without reflection, I think I would need to try to cause the race condition to occur (perhaps multiple times, to increase the chances of success).

Is there a way I'm missing, or a preferred route to take in these situations?

[AlexGhiondea]

@jswolf19 sorry for the delayed response.

I think using reflection in the tests to clean-up internal state between calls should be fine. I don't think that is the best way to do it in general, but it should be ok in this case.

Thanks for working on this!

[ghost]

There isn't always a good way to write a regression test for every bug (non-deterministic bugs especially so) and I think we can live without a test for this one rather than a test that invades the internals like that. (Note that this won't work at all if PlatformDetection.IsNetNative is true since that platform uses a completely different implementation of Reflection.)

[jswolf19]

@AlexGhiondea No worries. I can finally build again without dotnet crashing ^_^ I imagine things are busy.

Taking into account @atsushikan's advice, I'd still like to add a non-deterministic test, unless no test at all is preferred. I seem to be able to trigger the race condition fairly consistently even with just two tasks on my machine if I'm running only the Composition tests, so I'll see if it'll fail from build-tests. If so, I'll create Environment.ProcessorCount tasks to increase the odds of failure on a multi-core machine and then add in the fix.

[ghost]

I'd still like to add a non-deterministic test

Sounds good. If it's too time-consuming, it might have to be marked [Outerloop] but other than that, testing by stressing the code paths sounds reasonable to me.

[jswolf19]

The test I have failed only 2/10 times during build-tests on my machine (8 logical processors). If I add the CollectionBehavior attribute to have xunit not run in parallel, then I can get about a 5/10 failure rate. Any thoughts?

[GSPP]

@jswolf19 it has been my experience that threading issues are easier to produce with more threads than cores because that effectively pauses threads randomly every few dozen milliseconds and adds races.

[jswolf19]

I tried with two threads per core, which seemed to help a little (although it could have been statistical noise). I found that changing to threads from tasks and then having them wait on an event seems to get things lined up better to reproduce the issue. I got the test to fail 10/10 times during build-tests and it also seems to fail consistently when run alone (while allowing xunit to use parallel execution).

The test tends to take a little less than .5s on my computer, although it sometimes hits 1s.