Fix flaky NullReferenceException in MultiThreadedGetExportsWorkWithImportingConstructor on Mono/ARM#125804
Fix flaky NullReferenceException in MultiThreadedGetExportsWorkWithImportingConstructor on Mono/ARM#125804
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-collections |
…gConstructor by caching GetParameters() result On Mono/ARM, concurrent calls to ConstructorInfo.GetParameters() on the same ConstructorInfo can return different ParameterInfo instances due to lazy initialization. When GetActivator called GetParameters() a second time, the resulting ParameterInfo objects could differ from those stored in dependencies (from the first GetParameters() call in GetPartActivatorDependencies), causing ParameterInfoComparer.GetHashCode to fail (e.g., null Member on ARM) or the dictionary lookup to fail silently, leading to wrong/null constructor arguments. Fix: cache the GetParameters() result in _constructorParameters after the first call in GetPartActivatorDependencies, and reuse it in GetActivator to ensure the exact same ParameterInfo instances are used for both building the dependency dictionary and iterating over constructor parameters. Fixes: https://github.com/dotnet/runtime/issues/MultiThreadedGetExportsWorkWithImportingConstructor Co-authored-by: danmoseley <6385855+danmoseley@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes a flaky NullReferenceException in System.Composition.TypedParts triggered by concurrent ConstructorInfo.GetParameters() calls (notably on Mono/ARM) producing inconsistent/partially-initialized ParameterInfo instances.
Changes:
- Added a
_constructorParametersfield to cache constructor parameters. - Cached
_constructor.GetParameters()during dependency discovery and reused it during activator construction to keepParameterInfoidentity stable.
| // Cache GetParameters() result to ensure the same ParameterInfo instances | ||
| // are used in both GetPartActivatorDependencies and GetActivator. | ||
| // On some runtimes (e.g. Mono on ARM), concurrent calls to GetParameters() | ||
| // on the same ConstructorInfo can return different ParameterInfo instances | ||
| // due to lazy initialization, causing lookup failures. | ||
| _constructorParameters = _constructor.GetParameters(); | ||
| var cps = _constructorParameters; |
There was a problem hiding this comment.
_constructorParameters is assigned on every GetDependencies() call. Because ExportDescriptorPromise uses Lazy(..., isThreadSafe: false) (System.Composition.Hosting/Core/ExportDescriptorPromise.cs:46-47), GetDependencies/GetActivator can run concurrently on the same DiscoveredPart, so this can (1) invoke ConstructorInfo.GetParameters() concurrently (re-triggering the Mono/ARM lazy-init race) and (2) overwrite _constructorParameters between dependency creation and activator creation. Consider initializing _constructorParameters exactly once (and with a memory barrier) using LazyInitializer.EnsureInitialized or Interlocked.CompareExchange, and then always reuse the cached array.
| var cps = _constructorParameters ?? _constructor.GetParameters(); | ||
| Expression[] paramActivatorCalls = new Expression[cps.Length]; |
There was a problem hiding this comment.
GetActivator() falls back to calling _constructor.GetParameters() when _constructorParameters is null. Under concurrent initialization this can still result in multiple GetParameters() calls on the same ConstructorInfo, undermining the fix. Prefer using the same single-initialization helper here as well (e.g., EnsureInitialized/CompareExchange + Volatile.Read) so GetActivator never triggers another GetParameters() call once construction has begun.
|
@copilot address feedback |
Co-authored-by: danmoseley <6385855+danmoseley@users.noreply.github.com>
|
@copilot address feedback here |
Co-authored-by: danmoseley <6385855+danmoseley@users.noreply.github.com>
Addressed in b60ceee. Both |
| // On some runtimes (e.g. Mono on ARM), concurrent calls to GetParameters() | ||
| // on the same ConstructorInfo can return different ParameterInfo instances | ||
| // due to lazy initialization, causing lookup failures in GetActivator. | ||
| var cps = LazyInitializer.EnsureInitialized(ref _constructorParameters, () => _constructor.GetParameters()); |
There was a problem hiding this comment.
This is going to allocate a new delegate on every call, regardless of whether _constructorParameters is initialized or not.
Same for the similar change below.
Sounds like there's a lower-level root cause to be addressed? |
|
I thought that, but these scenarios will be coreclr soon, right? |
If this is only a mono on arm issue, and that's going away such that we don't want to invest in fixes there, then we shouldn't be making unnecessary compensating source changes higher in the stack as this PR is doing. |
|
True. @akoeplinger I can just leave this test disabled for mono? |
On Mono/ARM,
ConstructorInfo.GetParameters()lazily initializes its internal cache, and concurrent calls from different threads sharing the sameConstructorInfocan return distinctParameterInfoinstances with partially-initialized fields (e.g.,Member == null).DiscoveredPartcalledGetParameters()twice — once inGetPartActivatorDependenciesto buildParameterImportSitekeys, and again inGetActivatorto iterate parameters for dictionary lookup — meaning the two calls could return different instances. This causedParameterInfoComparer.GetHashCodeto dereference a nullMember, throwingNullReferenceException.Description
DiscoveredPart(src/libraries/System.Composition.TypedParts/src/System/Composition/TypedParts/Discovery/DiscoveredPart.cs):private ParameterInfo[] _constructorParametersfieldGetPartActivatorDependenciesandGetActivator, replaced direct_constructor.GetParameters()calls withLazyInitializer.EnsureInitialized(ref _constructorParameters, ...)to ensure the result is initialized exactly once perDiscoveredPartinstance with a proper memory barrierLazyInitializer.EnsureInitializedusesVolatile.Read+Interlocked.CompareExchangeunder the hood: multiple threads may race to call the factory, but only one result is stored and all subsequent callers — includingGetActivator— read back that sameParameterInfo[]instance. This guarantees both methods always operate on the sameParameterInfoinstances, soReferenceEqualssucceeds in theParameterInfoComparerlookup rather than falling through to position+member comparison against potentially-different instances.Security
No security impact.
Original prompt
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.