Skip to content

Commit

Permalink
Threading fixes (#76230)
Browse files Browse the repository at this point in the history
We have a few theoretical races in some of our threading code, as detailed in #75759. I've audited our uses of ImmutableArray as backing fields for this pattern, fixing it up where I find issues, as well as a couple of other threading issues that I found while looking at things named lazy in our codebase.

Closes #75759
  • Loading branch information
333fred authored Dec 16, 2024
1 parent 192c9cc commit 1715a86
Show file tree
Hide file tree
Showing 21 changed files with 88 additions and 45 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -613,9 +613,9 @@ private static void GetExportedTypes(NamespaceOrTypeSymbol symbol, int parentInd

if (_lazyExportedTypes.IsDefault)
{
_lazyExportedTypes = CalculateExportedTypes();
var initialized = ImmutableInterlocked.InterlockedInitialize(ref _lazyExportedTypes, CalculateExportedTypes());

if (_lazyExportedTypes.Length > 0)
if (initialized && _lazyExportedTypes.Length > 0)
{
ReportExportedTypeNameCollisions(_lazyExportedTypes, diagnostics);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.Diagnostics.CodeAnalysis;
using System.Reflection.Metadata;
using System.Threading;
using Microsoft.CodeAnalysis.Collections;

namespace Microsoft.CodeAnalysis.CSharp.Symbols.Metadata.PE
{
Expand Down Expand Up @@ -97,7 +98,7 @@ private void EnsureClassAndConstructorSymbolsAreLoaded()

private void EnsureAttributeArgumentsAreLoaded()
{
if (_lazyConstructorArguments.IsDefault || _lazyNamedArguments.IsDefault)
if (RoslynImmutableInterlocked.VolatileRead(in _lazyConstructorArguments).IsDefault || RoslynImmutableInterlocked.VolatileRead(in _lazyNamedArguments).IsDefault)
{
TypedConstant[]? lazyConstructorArguments = null;
KeyValuePair<string, TypedConstant>[]? lazyNamedArguments = null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
using System.Linq;
using System.Reflection.Metadata;
using System;
using System.Threading;

namespace Microsoft.CodeAnalysis.CSharp.Symbols.Metadata.PE
{
Expand Down Expand Up @@ -81,7 +82,7 @@ internal override ModuleSymbol ContainingModule

protected override void EnsureAllMembersLoaded()
{
if (lazyTypes == null || lazyNamespaces == null)
if (Volatile.Read(ref lazyTypes) == null || Volatile.Read(ref lazyNamespaces) == null)
{
IEnumerable<IGrouping<string, TypeDefinitionHandle>> groups;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,24 +131,24 @@ public MethodKind MethodKind
public bool IsExplicitFinalizerOverride => (_bits & IsExplicitFinalizerOverrideBit) != 0;
public bool IsExplicitClassOverride => (_bits & IsExplicitClassOverrideBit) != 0;
public bool IsExplicitOverrideIsPopulated => (_bits & IsExplicitOverrideIsPopulatedBit) != 0;
public bool IsObsoleteAttributePopulated => (_bits & IsObsoleteAttributePopulatedBit) != 0;
public bool IsCustomAttributesPopulated => (_bits & IsCustomAttributesPopulatedBit) != 0;
public bool IsUseSiteDiagnosticPopulated => (_bits & IsUseSiteDiagnosticPopulatedBit) != 0;
public bool IsConditionalPopulated => (_bits & IsConditionalPopulatedBit) != 0;
public bool IsOverriddenOrHiddenMembersPopulated => (_bits & IsOverriddenOrHiddenMembersPopulatedBit) != 0;
public bool IsObsoleteAttributePopulated => (Volatile.Read(ref _bits) & IsObsoleteAttributePopulatedBit) != 0;
public bool IsCustomAttributesPopulated => (Volatile.Read(ref _bits) & IsCustomAttributesPopulatedBit) != 0;
public bool IsUseSiteDiagnosticPopulated => (Volatile.Read(ref _bits) & IsUseSiteDiagnosticPopulatedBit) != 0;
public bool IsConditionalPopulated => (Volatile.Read(ref _bits) & IsConditionalPopulatedBit) != 0;
public bool IsOverriddenOrHiddenMembersPopulated => (Volatile.Read(ref _bits) & IsOverriddenOrHiddenMembersPopulatedBit) != 0;
public bool IsReadOnly => (_bits & IsReadOnlyBit) != 0;
public bool IsReadOnlyPopulated => (_bits & IsReadOnlyPopulatedBit) != 0;
public bool DoesNotReturn => (_bits & DoesNotReturnBit) != 0;
public bool IsDoesNotReturnPopulated => (_bits & IsDoesNotReturnPopulatedBit) != 0;
public bool IsMemberNotNullPopulated => (_bits & IsMemberNotNullPopulatedBit) != 0;
public bool IsMemberNotNullPopulated => (Volatile.Read(ref _bits) & IsMemberNotNullPopulatedBit) != 0;
public bool IsInitOnly => (_bits & IsInitOnlyBit) != 0;
public bool IsInitOnlyPopulated => (_bits & IsInitOnlyPopulatedBit) != 0;
public bool IsUnmanagedCallersOnlyAttributePopulated => (_bits & IsUnmanagedCallersOnlyAttributePopulatedBit) != 0;
public bool IsUnmanagedCallersOnlyAttributePopulated => (Volatile.Read(ref _bits) & IsUnmanagedCallersOnlyAttributePopulatedBit) != 0;
public bool HasSetsRequiredMembers => (_bits & HasSetsRequiredMembersBit) != 0;
public bool HasSetsRequiredMembersPopulated => (_bits & HasSetsRequiredMembersPopulatedBit) != 0;
public bool IsUnscopedRef => (_bits & IsUnscopedRefBit) != 0;
public bool IsUnscopedRefPopulated => (_bits & IsUnscopedRefPopulatedBit) != 0;
public bool IsOverloadResolutionPriorityPopulated => (_bits & OverloadResolutionPriorityPopulatedBit) != 0;
public bool IsOverloadResolutionPriorityPopulated => (Volatile.Read(ref _bits) & OverloadResolutionPriorityPopulatedBit) != 0;

#if DEBUG
static PackedFlags()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
using System.Reflection.Metadata.Ecma335;
using System.Runtime.InteropServices;
using System.Threading;
using Microsoft.CodeAnalysis.Collections;
using Microsoft.CodeAnalysis.CSharp.DocumentationComments;
using Microsoft.CodeAnalysis.CSharp.Emit;
using Microsoft.CodeAnalysis.PooledObjects;
Expand Down Expand Up @@ -1260,7 +1261,7 @@ private void EnsureEnumUnderlyingTypeIsLoaded(UncommonProperties uncommon)

private void EnsureAllMembersAreLoaded()
{
if (_lazyMembersByName == null)
if (Volatile.Read(ref _lazyMembersByName) == null)
{
LoadMembers();
}
Expand All @@ -1270,7 +1271,7 @@ private void LoadMembers()
{
ArrayBuilder<Symbol> members = null;

if (_lazyMembersInDeclarationOrder.IsDefault)
if (RoslynImmutableInterlocked.VolatileRead(ref _lazyMembersInDeclarationOrder).IsDefault)
{
EnsureNestedTypesAreLoaded();

Expand Down Expand Up @@ -1440,7 +1441,7 @@ private void LoadMembers()
}
}

if (_lazyMembersByName == null)
if (Volatile.Read(ref _lazyMembersByName) == null)
{
if (members == null)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,11 +125,14 @@ internal override ModuleSymbol ContainingModule

protected override void EnsureAllMembersLoaded()
{
var typesByNS = _typesByNS;
var typesByNS = Volatile.Read(ref _typesByNS);

if (lazyTypes == null || lazyNamespaces == null)
if (typesByNS == null)
{
Debug.Assert(lazyNamespaces != null && lazyTypes != null);
}
else
{
System.Diagnostics.Debug.Assert(typesByNS != null);
LoadAllMembers(typesByNS);
Interlocked.Exchange(ref _typesByNS, null);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,28 +128,28 @@ public void SetUseSiteDiagnosticPopulated()
ThreadSafeFlagOperations.Set(ref _bits, IsUseSiteDiagnosticPopulatedBit);
}

public readonly bool IsUseSiteDiagnosticPopulated => (_bits & IsUseSiteDiagnosticPopulatedBit) != 0;
public bool IsUseSiteDiagnosticPopulated => (Volatile.Read(ref _bits) & IsUseSiteDiagnosticPopulatedBit) != 0;

public void SetObsoleteAttributePopulated()
{
ThreadSafeFlagOperations.Set(ref _bits, IsObsoleteAttributePopulatedBit);
}

public readonly bool IsObsoleteAttributePopulated => (_bits & IsObsoleteAttributePopulatedBit) != 0;
public bool IsObsoleteAttributePopulated => (Volatile.Read(ref _bits) & IsObsoleteAttributePopulatedBit) != 0;

public void SetCustomAttributesPopulated()
{
ThreadSafeFlagOperations.Set(ref _bits, IsCustomAttributesPopulatedBit);
}

public readonly bool IsCustomAttributesPopulated => (_bits & IsCustomAttributesPopulatedBit) != 0;
public bool IsCustomAttributesPopulated => (Volatile.Read(ref _bits) & IsCustomAttributesPopulatedBit) != 0;

public void SetOverloadResolutionPriorityPopulated()
{
ThreadSafeFlagOperations.Set(ref _bits, IsOverloadResolutionPriorityPopulatedBit);
}

public readonly bool IsOverloadResolutionPriorityPopulated => (_bits & IsOverloadResolutionPriorityPopulatedBit) != 0;
public bool IsOverloadResolutionPriorityPopulated => (Volatile.Read(ref _bits) & IsOverloadResolutionPriorityPopulatedBit) != 0;
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
using System.Reflection.Metadata;
using System.Reflection.Metadata.Ecma335;
using System.Threading;
using Microsoft.CodeAnalysis.Collections;
using Microsoft.CodeAnalysis.PooledObjects;
using Roslyn.Utilities;

Expand Down Expand Up @@ -152,7 +153,7 @@ private ImmutableArray<TypeWithAnnotations> GetDeclaredConstraintTypes(ConsList<
Debug.Assert(!inProgress.ContainsReference(this));
Debug.Assert(!inProgress.Any() || ReferenceEquals(inProgress.Head.ContainingSymbol, this.ContainingSymbol));

if (_lazyDeclaredConstraintTypes.IsDefault)
if (RoslynImmutableInterlocked.VolatileRead(ref _lazyDeclaredConstraintTypes).IsDefault)
{
ImmutableArray<TypeWithAnnotations> declaredConstraintTypes;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.Diagnostics;
using System.Threading;
using Microsoft.Cci;
using Microsoft.CodeAnalysis.Collections;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.PooledObjects;
using Roslyn.Utilities;
Expand Down Expand Up @@ -181,7 +182,7 @@ public override ImmutableArray<ParameterSymbol> Parameters

private void ComputeParameters()
{
if (_lazyParameters != null)
if (!RoslynImmutableInterlocked.VolatileRead(in _lazyParameters).IsDefault)
{
return;
}
Expand Down Expand Up @@ -211,7 +212,7 @@ private void ComputeParameters()

lock (_declarationDiagnostics)
{
if (_lazyParameters != null)
if (!_lazyParameters.IsDefault)
{
diagnostics.Free();
return;
Expand All @@ -221,7 +222,7 @@ private void ComputeParameters()
_declarationDependencies.AddAll(diagnostics.DependenciesBag);
diagnostics.Free();
_lazyIsVarArg = isVararg;
_lazyParameters = parameters;
RoslynImmutableInterlocked.VolatileWrite(ref _lazyParameters, parameters);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.Collections.Immutable;
using System.Diagnostics;
using System.Threading;
using Microsoft.CodeAnalysis.Collections;
using Microsoft.CodeAnalysis.PooledObjects;
using Roslyn.Utilities;

Expand Down Expand Up @@ -99,7 +100,7 @@ public sealed override ImmutableArray<TypeParameterSymbol> TypeParameters

private void EnsureMapAndTypeParameters()
{
if (!_lazyTypeParameters.IsDefault)
if (!RoslynImmutableInterlocked.VolatileRead(ref _lazyTypeParameters).IsDefault)
{
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using System.Diagnostics;
using System.Linq;
using System.Threading;
using Microsoft.CodeAnalysis.Collections;
using Microsoft.CodeAnalysis.CSharp.Emit;
using Microsoft.CodeAnalysis.PooledObjects;
using Roslyn.Utilities;
Expand Down Expand Up @@ -96,7 +97,7 @@ public sealed override ImmutableArray<TypeParameterSymbol> TypeParameters

private void EnsureMapAndTypeParameters()
{
if (!_lazyTypeParameters.IsDefault)
if (!RoslynImmutableInterlocked.VolatileRead(ref _lazyTypeParameters).IsDefault)
{
return;
}
Expand Down
5 changes: 3 additions & 2 deletions src/Compilers/Core/Portable/Syntax/SyntaxTree.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using System.Text;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.Collections;
using Microsoft.CodeAnalysis.Text;
using Roslyn.Utilities;

Expand Down Expand Up @@ -369,11 +370,11 @@ internal int GetDisplayLineNumber(TextSpan span)
/// </summary>
internal Cci.DebugSourceInfo GetDebugSourceInfo()
{
if (_lazyChecksum.IsDefault)
if (RoslynImmutableInterlocked.VolatileRead(ref _lazyChecksum).IsDefault)
{
var text = this.GetText();
_lazyChecksum = text.GetChecksum();
_lazyHashAlgorithm = text.ChecksumAlgorithm;
ImmutableInterlocked.InterlockedInitialize(ref _lazyChecksum, text.GetChecksum());
}

Debug.Assert(!_lazyChecksum.IsDefault);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ Imports Microsoft.CodeAnalysis.VisualBasic.Symbols
Imports Microsoft.CodeAnalysis.VisualBasic.Syntax
Imports System.Runtime.InteropServices
Imports System.Threading
Imports Microsoft.CodeAnalysis.Collections

Namespace Microsoft.CodeAnalysis.VisualBasic

Expand Down Expand Up @@ -55,7 +56,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
Debug.Assert(group.PendingExtensionMethodsOpt Is Me)
Debug.Assert(Not useSiteInfo.AccumulatesDependencies OrElse _withDependencies)

If _lazyMethods.IsDefault Then
If RoslynImmutableInterlocked.VolatileRead(_lazyMethods).IsDefault Then
Dim receiverOpt As BoundExpression = group.ReceiverOpt
Dim methods As ImmutableArray(Of MethodSymbol) = ImmutableArray(Of MethodSymbol).Empty
Dim localUseSiteInfo = If(_withDependencies, New CompoundUseSiteInfo(Of AssemblySymbol)(_lookupBinder.Compilation.Assembly), CompoundUseSiteInfo(Of AssemblySymbol).DiscardedDependencies)
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/VisualBasic/Portable/Emit/PEModuleBuilder.vb
Original file line number Diff line number Diff line change
Expand Up @@ -386,9 +386,9 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Emit
Debug.Assert(HaveDeterminedTopLevelTypes)

If _lazyExportedTypes.IsDefault Then
_lazyExportedTypes = CalculateExportedTypes()
Dim initialized = ImmutableInterlocked.InterlockedInitialize(_lazyExportedTypes, CalculateExportedTypes())

If _lazyExportedTypes.Length > 0 Then
If initialized AndAlso _lazyExportedTypes.Length > 0 Then
ReportExportedTypeNameCollisions(_lazyExportedTypes, diagnostics)
End If
End If
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Symbols.Metadata.PE

Private Sub EnsureLazyMembersAreLoaded()

If _lazyConstructorArguments Is Nothing Then
If Volatile.Read(_lazyConstructorArguments) Is Nothing Then

Dim constructorArgs As TypedConstant() = Nothing
Dim namedArgs As KeyValuePair(Of String, TypedConstant)() = Nothing
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ Imports Microsoft.CodeAnalysis.Text
Imports Microsoft.CodeAnalysis.VisualBasic.Symbols
Imports Microsoft.CodeAnalysis.VisualBasic.Syntax
Imports System.Reflection.Metadata
Imports System.Threading

Namespace Microsoft.CodeAnalysis.VisualBasic.Symbols.Metadata.PE

Expand Down Expand Up @@ -61,7 +62,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Symbols.Metadata.PE
End Property

Protected Overrides Sub EnsureAllMembersLoaded()
If m_lazyTypes Is Nothing OrElse m_lazyMembers Is Nothing Then
If Volatile.Read(m_lazyTypes) Is Nothing OrElse Volatile.Read(m_lazyMembers) Is Nothing Then
Dim groups As IEnumerable(Of IGrouping(Of String, TypeDefinitionHandle))

Try
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,25 +113,25 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Symbols.Metadata.PE

Public ReadOnly Property IsObsoleteAttributePopulated As Boolean
Get
Return (_bits And s_isObsoleteAttributePopulatedBit) <> 0
Return (Volatile.Read(_bits) And s_isObsoleteAttributePopulatedBit) <> 0
End Get
End Property

Public ReadOnly Property IsCustomAttributesPopulated As Boolean
Get
Return (_bits And s_isCustomAttributesPopulatedBit) <> 0
Return (Volatile.Read(_bits) And s_isCustomAttributesPopulatedBit) <> 0
End Get
End Property

Public ReadOnly Property IsUseSiteDiagnosticPopulated As Boolean
Get
Return (_bits And s_isUseSiteDiagnosticPopulatedBit) <> 0
Return (Volatile.Read(_bits) And s_isUseSiteDiagnosticPopulatedBit) <> 0
End Get
End Property

Public ReadOnly Property IsConditionalPopulated As Boolean
Get
Return (_bits And s_isConditionalAttributePopulatedBit) <> 0
Return (Volatile.Read(_bits) And s_isConditionalAttributePopulatedBit) <> 0
End Get
End Property

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -649,7 +649,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Symbols.Metadata.PE

Private Sub EnsureNonTypeMembersAreLoaded()

If _lazyMembers Is Nothing Then
If Volatile.Read(_lazyMembers) Is Nothing Then
' A method may be referenced as an accessor by one or more properties. And,
' any of those properties may be "bogus" if one of the property accessors
' does not match the property signature. If the method is referenced by at
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,11 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Symbols.Metadata.PE
End Property

Protected Overrides Sub EnsureAllMembersLoaded()
Dim typesByNS = _typesByNS
Dim typesByNS = Volatile.Read(_typesByNS)

If m_lazyTypes Is Nothing OrElse m_lazyMembers Is Nothing Then
Debug.Assert(typesByNS IsNot Nothing)
If typesByNS Is Nothing Then
Debug.Assert(m_lazyTypes IsNot Nothing AndAlso m_lazyMembers IsNot Nothing)
Else
LoadAllMembers(typesByNS)
Interlocked.Exchange(_typesByNS, Nothing)
End If
Expand Down
Loading

0 comments on commit 1715a86

Please sign in to comment.