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.

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...

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.