Is the locking in ReflectionComposablePart correct?

[omajid]

runtime/src/libraries/System.ComponentModel.Composition/src/System/ComponentModel/Composition/ReflectionModel/ReflectionComposablePart.cs

Lines 85 to 97 in 93eacbd

	var value = _importsCache;
	if (value == null)
	{
	lock (_lock)
	{
	if (value == null)
	{
	value = new Dictionary<ImportDefinition, ImportingItem>();
	_importsCache = value;
	}
	}
	}
	return value;

https://github.com/dotnet/runtime/blob/main/docs/design/specs/Memory-model.md describes how Object assignment to a location potentially accessible by other threads is a release with respect to accesses to the instance’s fields/elements and metadata. But this method only uses the local variable value for locking. Any updates to value will not be seen by other threads. It seems like this might be possible:

1. Thread 1 sees _importsCache == null and value == null and enters the first conditional
2. Thread 2 sees _importsCache == null and value == null and enters the first conditional too
3. Thread 1 takes the lock, sees value == null and initializes _importsCache to a new dictionary
4. Thread 2 takes the lock, see the local variable value is still null, and then re-initializes _importsCache to another value.

Is this correct/possible?

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-componentmodel-composition
See info in area-owners.md if you want to be subscribed.

[stephentoub]

This is broken.

runtime/src/libraries/System.ComponentModel.Composition/src/System/ComponentModel/Composition/ReflectionModel/ReflectionComposablePart.cs

Line 90 in 93eacbd

if (value == null)

should be checking the field not the local.

[omajid]

Same issue in

runtime/src/libraries/System.ComponentModel.Composition/src/System/ComponentModel/Composition/ReflectionModel/ReflectionComposablePart.cs

Lines 64 to 77 in 93eacbd

	var value = _importValues;
	if (value == null)
	{
	lock (_lock)
	{
	value = _importValues;
	if (value == null)
	{
	value = new Dictionary<ImportDefinition, object?>();
	_importValues = value;
	}
	}
	}
	return value;

[stephentoub]

Same issue in

That one's ok. Note L69.