Is the double-checked locking pattern in BuildManager correct?

[omajid]

msbuild/src/Build/BackEnd/BuildManager/BuildManager.cs

Lines 3177 to 3221 in 4a45d56

	if (!_disposed)
	{
	if (disposing)
	{
	lock (_syncLock)
	{
	// We should always have finished cleaning up before calling Dispose.
	RequireState(BuildManagerState.Idle, "ShouldNotDisposeWhenBuildManagerActive");
	_componentFactories?.ShutdownComponents();
	if (_workQueue != null)
	{
	_workQueue.Complete();
	_workQueue = null;
	}
	if (_executionCancellationTokenSource != null)
	{
	_executionCancellationTokenSource.Cancel();
	_executionCancellationTokenSource = null;
	}
	if (_noActiveSubmissionsEvent != null)
	{
	_noActiveSubmissionsEvent.Dispose();
	_noActiveSubmissionsEvent = null;
	}
	if (_noNodesActiveEvent != null)
	{
	_noNodesActiveEvent.Dispose();
	_noNodesActiveEvent = null;
	}
	if (ReferenceEquals(this, s_singletonInstance))
	{
	s_singletonInstance = null;
	}
	_disposed = true;
	}
	}
	}
	}

This doesn't quite match the double-checked locking pattern described at:

https://github.com/dotnet/runtime/blob/main/docs/design/specs/Memory-model.md#examples-and-common-patterns

It seems it might be possible to see a stale value of _disposed and then enter the lock and do the cleanup operations again. Is this safe to do because the cleanup operations are idempotent?

[JanKrivanek]

Team is currently overloaded - so postponing this.
Contributions are very welcome for this one.

[JanKrivanek]

Thank you @omajid for looking into our code and taking time to report this!

I quickly peeked into this - all involved code paths have the pattern 'If not null - clean and nullify' - which makes it effectively idempotent.

However - I needed to navigate through the code a bit to confirm this - so the intent is not clear, plus it's very easy to accidentaly break it (e.g. by changing the behavior of BuildComponentFactoryCollection.ShutdownComponent).

So - since I already spent couple minutes, I'll spend 5 more to make the intent of the code clear.