Threading fixes #76230
Threading fixes #76230
Conversation
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
jaredpar
removed
the
untriaged
| @@ -613,9 +613,9 @@ private static void GetExportedTypes(NamespaceOrTypeSymbol symbol, int parentInd | |||
|
|
|||
| if (_lazyExportedTypes.IsDefault) | |||
| { | |||
| _lazyExportedTypes = CalculateExportedTypes(); | |||
| var initialized = ImmutableInterlocked.InterlockedInitialize(ref _lazyExportedTypes, CalculateExportedTypes()); | |||
There was a problem hiding this comment.
This is not a case of immutable arrays being used as a gate for other logic fields. Rather, it is a simple threading issue that I discovered while auditing all of our lazy immutable array fields. It could result in diagnostics being double reported depending on calling patterns, based on simple thread timing.
| 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; |
There was a problem hiding this comment.
For these types of flags enums, where we read a bit from packed flags, then look at a separate structure for the actual value, nothing was preventing us from looking at the separate structure first due to speculation or reordering. I did not use Volatile.Read everywhere because, for the other flags, both the "is populated" and "result" sections are both bits on the same integer, and we do not have to worry about out-of-order reads of the same field.
| { | ||
| System.Diagnostics.Debug.Assert(typesByNS != null); |
There was a problem hiding this comment.
This was a simple thread-timing issue waiting to happen. Rather than using lazyTypes and lazyNamespaces as proxies to the real sentinel, we should just use _typesByNS, which is the actual sentinel that work was completed.
| @@ -221,7 +222,7 @@ private void ComputeParameters() | |||
| _declarationDependencies.AddAll(diagnostics.DependenciesBag); | |||
| diagnostics.Free(); | |||
| _lazyIsVarArg = isVararg; | |||
| _lazyParameters = parameters; | |||
| RoslynImmutableInterlocked.VolatileWrite(ref _lazyParameters, parameters); | |||
There was a problem hiding this comment.
Nothing prevents writes within a lock from being observed in alternate orders by other threads, when those other threads don't use a lock to check themselves. Since _lazyParameters is checked for Default outside of a lock as the sentinel, we therefore need to make sure that it's the last write inside this lock or we could still race.
| @@ -369,11 +370,11 @@ internal int GetDisplayLineNumber(TextSpan span) | |||
| /// </summary> | |||
| internal Cci.DebugSourceInfo GetDebugSourceInfo() | |||
| { | |||
| if (_lazyChecksum.IsDefault) | |||
| if (RoslynImmutableInterlocked.VolatileRead(ref _lazyChecksum).IsDefault) | |||
There was a problem hiding this comment.
Stumbling on this was frankly a bit shocking. My only conclusion is that GetDebugSourceInfo can never be called from threaded code, because no speculation or reordering is required to race here. It just sets the fields in the wrong order!
|
@dotnet/roslyn-compiler for a second review. |
1 similar comment
|
@dotnet/roslyn-compiler for a second review. |
src/Compilers/VisualBasic/Portable/Symbols/Attributes/PEAttributeData.vb
Show resolved
Hide resolved
src/Compilers/VisualBasic/Portable/Symbols/Metadata/PE/PENestedNamespaceSymbol.vb
Outdated
Show resolved
Hide resolved
We have a few theoretical races in some of our threading code, as detailed in #75759. I've audited our uses of
ImmutableArrayas 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 namedlazyin our codebase.Closes #75759