Avoid race condition in map for TupleElementIndex #58500

jcouv · 2021-12-26T18:33:40Z

The race condition is that PENamedTypeSymbol.LoadMembers calls MakeSynthesizedTupleMembers and then saves members in _lazyMembersInDeclarationOrder. But MakeSynthesizedTupleMembers is having a side-effect which saves some members in a map.
If two threads call LoadMembers concurrently, the members in _lazyMembersInDeclarationOrder and in _lazyFieldDefinitionsToIndexMap may not match (ie. they are coming from two different threads that are loading symbols).

Here are some of the existing tests that are relevant: TupleWithElementNamedWithDefaultName, TestValueTuple8Definition, IndexOfUnderlyingFieldsInTuple9, UnderlyingTypeMemberWithWrongSignature_1.

Fixes #52658 (flaky value of TupleElementIndex in TupleWithElementNamedWithDefaultName test)
Fixes #57560 (crash in TupleWithElementNamedWithDefaultName test now that AssertOrFailFast was added)

This PR maintains existing behavior, but I think it's not quite right. Filed #58499

CyrusNajmabadi · 2021-12-26T19:04:50Z

src/Compilers/CSharp/Portable/Symbols/FieldSymbol.cs

+
+                return found is not null
+                    ? tupleElementPosition - 1
+                    : -1;


is any of this expensive? would it be worth caching the reuslt of this? #Resolved

Considered this question and I lean towards not caching.
When using tuple syntax, we create TupleElementFieldSymbol for default elements (and TupleVirtualElementFieldSymbol for named elements) which stores location and name. So this path is only used when accessing a field directly from ValueTuple type (from source or PE) and that'd be less common.
I've filed an issue for discussion which proposes to remove this wrapping with TupleElementFieldSymbol. If we go down that route then caching would make more sense.

cool, that makes sense to me :)

CyrusNajmabadi · 2021-12-26T19:05:42Z

src/Compilers/CSharp/Portable/Symbols/Tuples/TupleTypeSymbol.cs

            {
-                if (name.StartsWith("Item", StringComparison.Ordinal))
+                string tail = name.Substring(4);


Suggested change

string tail = name.Substring(4);

string tail = name.Substring("Item".Length);

(will be the exact same jitted code, but this helps make it clearer what's going on (imo)). #Resolved

will be the exact same jitted code

It looks like get_Length() is called (see sharplab.io).

cston · 2022-01-04T19:52:29Z

src/Compilers/CSharp/Portable/Symbols/FieldSymbol.cs

+                WellKnownMember wellKnownMember = NamedTypeSymbol.GetTupleTypeMember(arity, tupleElementPosition);
+                MemberDescriptor descriptor = WellKnownMembers.GetDescriptor(wellKnownMember);
+                Symbol found = CSharpCompilation.GetRuntimeMember(ImmutableArray.Create<Symbol>(this), descriptor, CSharpCompilation.SpecialMembersSignatureComparer.Instance,
+                    accessWithinOpt: null); // force lookup of public members only


Can we call comparer.MatchFieldSignature(this, descriptor.Signature) directly instead? #ByDesign

GetRuntimeMember checks a few other things (static-ness, accessibility). I'd rather stay close to logic we used in MakeSynthesizedTupleMembers.

AlekseyTs · 2022-01-04T23:45:30Z

src/Compilers/CSharp/Portable/Symbols/Tuples/TupleTypeSymbol.cs

-            /// This supports <see cref="FieldSymbol.TupleElementIndex"/>.
-            /// For those fields, we map from their definition to an index.
-            /// </summary>
-            private SmallDictionary<FieldSymbol, int>? _lazyFieldDefinitionsToIndexMap;


_lazyFieldDefinitionsToIndexMap

Are we certain that nothing else in this class is prone to a similar race? Would it be simpler and more robust to ensure that members and tuple data are in sync by using an alternative approach? For example, by ensuring that they are stored together at once. #Resolved

I've reviewed the other parts of TupleExtraData and didn't spot a similar race condition.
That said, there is a race condition left somewhere (issue #53943) which seems different to the one fixed here. I haven't identified the root cause yet.

For the suggestion to have MakeSynthesizedTupleMembers return the map and have the caller store it, we'd discussed this with Chuck among a few options before the holiday break (but we leaned towards removing the map). I'm open to the approach if Chuck is okay with it.
If we do go that route, do you have an initialization pattern for the caller? Feels like we'd need to spin-wait the threads that are waiting for the winning thread to save the map.

AlekseyTs · 2022-01-04T23:59:26Z

I think that the more robust way to avoid this and similar races would be to change MakeSynthesizedTupleMembers to not mutate an instance of symbol, but instead return an instance of TupleExtraData that corresponds to members it looked at (perhaps it can take an instance of TupleExtraData to populate as an input as well), that should be stored together with the members by the consumer. Not only that would guarantee that we would address the current race, but that would address other races we overlooked and would prevent us from re-introducing a race in the future. That would also reduce the volume of required changes in a quite complicated logic.

src/Compilers/CSharp/Portable/Symbols/Tuples/TupleTypeSymbol.cs

This reverts commit 27d7a4b.

RikkiGibson · 2022-02-04T20:12:09Z

src/Compilers/CSharp/Portable/Symbols/FieldSymbol.cs

+                Symbol found = CSharpCompilation.GetRuntimeMember(ImmutableArray.Create<Symbol>(this), descriptor, CSharpCompilation.SpecialMembersSignatureComparer.Instance,
+                    accessWithinOpt: null); // force lookup of public members only


nit:

Suggested change

Symbol found = CSharpCompilation.GetRuntimeMember(ImmutableArray.Create<Symbol>(this), descriptor, CSharpCompilation.SpecialMembersSignatureComparer.Instance,

accessWithinOpt: null); // force lookup of public members only

Symbol found = CSharpCompilation.GetRuntimeMember(

ImmutableArray.Create<Symbol>(this),

descriptor,

CSharpCompilation.SpecialMembersSignatureComparer.Instance,

accessWithinOpt: null); // force lookup of public members only

RikkiGibson · 2022-02-04T20:14:19Z

src/Compilers/CSharp/Portable/Symbols/FieldSymbol.cs

+                    // ex: no "Item2" in 'ValueTuple<T1>'
+                    return -1;
+                }
+                Debug.Assert(tupleElementPosition < NamedTypeSymbol.ValueTupleRestPosition);


Could you please clarify why we can assume this?

Should this assert be moved to immediately following the if (!ContainingType.IsDefinition) { ... } block?

RikkiGibson · 2022-02-09T23:55:00Z

Is this PR ready to merge?

jcouv added Area-Compilers Feature - Tuples Tuples labels Dec 26, 2021

jcouv self-assigned this Dec 26, 2021

jcouv added 3 commits December 26, 2021 10:35

Remove tuple element index map

bb4050c

Add test for index of underlying field in long tuple

44387c7

Remove instrumentation

c63cb79

jcouv force-pushed the tuple-map branch from 81dc580 to c63cb79 Compare December 26, 2021 18:36

CyrusNajmabadi reviewed Dec 26, 2021

View reviewed changes

jcouv marked this pull request as ready for review January 3, 2022 17:01

jcouv requested a review from a team as a code owner January 3, 2022 17:01

cston reviewed Jan 4, 2022

View reviewed changes

AlekseyTs reviewed Jan 4, 2022

View reviewed changes

jcouv marked this pull request as draft January 10, 2022 19:48

Use CompareExchange

27d7a4b

AlekseyTs reviewed Jan 18, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Tuples/TupleTypeSymbol.cs Outdated Show resolved Hide resolved

Revert "Use CompareExchange"

e5cac13

This reverts commit 27d7a4b.

runfoapp bot mentioned this pull request Jan 18, 2022

[Flaky Test] Roslyn.VisualStudio.IntegrationTests.CSharp.CSharpCodeActions.GenerateMethodInClosedFile times out #57722

Closed

jcouv marked this pull request as ready for review February 2, 2022 18:19

jcouv added 2 commits February 2, 2022 10:20

Merge remote-tracking branch 'dotnet/main' into tuple-map

f6dd0d8

Address feedback

30a00e9

RikkiGibson self-assigned this Feb 4, 2022

RikkiGibson reviewed Feb 4, 2022

View reviewed changes

RikkiGibson approved these changes Feb 4, 2022

View reviewed changes

cston approved these changes Feb 4, 2022

View reviewed changes

jcouv enabled auto-merge (squash) February 11, 2022 00:59

jcouv merged commit 4d1d4b9 into dotnet:main Feb 11, 2022

ghost added this to the Next milestone Feb 11, 2022

jcouv deleted the tuple-map branch February 11, 2022 02:47

RikkiGibson modified the milestones: Next, 17.2.P2 Mar 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid race condition in map for TupleElementIndex #58500

Avoid race condition in map for TupleElementIndex #58500

jcouv commented Dec 26, 2021 •

edited

Loading

CyrusNajmabadi Dec 26, 2021 •

edited by jcouv

Loading

jcouv Dec 27, 2021

CyrusNajmabadi Dec 27, 2021

CyrusNajmabadi Dec 26, 2021 •

edited by jcouv

Loading

cston Feb 4, 2022 •

edited

Loading

cston Jan 4, 2022 •

edited by jcouv

Loading

jcouv Feb 2, 2022

AlekseyTs Jan 4, 2022 •

edited by jcouv

Loading

jcouv Jan 5, 2022

AlekseyTs commented Jan 4, 2022

RikkiGibson Feb 4, 2022

RikkiGibson Feb 4, 2022

RikkiGibson Feb 4, 2022

RikkiGibson commented Feb 9, 2022

	string tail = name.Substring(4);
	string tail = name.Substring("Item".Length);

		Symbol found = CSharpCompilation.GetRuntimeMember(ImmutableArray.Create<Symbol>(this), descriptor, CSharpCompilation.SpecialMembersSignatureComparer.Instance,
		accessWithinOpt: null); // force lookup of public members only

Avoid race condition in map for TupleElementIndex #58500

Avoid race condition in map for TupleElementIndex #58500

Conversation

jcouv commented Dec 26, 2021 • edited Loading

CyrusNajmabadi Dec 26, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Dec 27, 2021

Choose a reason for hiding this comment

CyrusNajmabadi Dec 27, 2021

Choose a reason for hiding this comment

CyrusNajmabadi Dec 26, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Feb 4, 2022 • edited Loading

Choose a reason for hiding this comment

cston Jan 4, 2022 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Feb 2, 2022

Choose a reason for hiding this comment

AlekseyTs Jan 4, 2022 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Jan 5, 2022

Choose a reason for hiding this comment

AlekseyTs commented Jan 4, 2022

RikkiGibson Feb 4, 2022

Choose a reason for hiding this comment

RikkiGibson Feb 4, 2022

Choose a reason for hiding this comment

RikkiGibson Feb 4, 2022

Choose a reason for hiding this comment

RikkiGibson commented Feb 9, 2022

jcouv commented Dec 26, 2021 •

edited

Loading

CyrusNajmabadi Dec 26, 2021 •

edited by jcouv

Loading

CyrusNajmabadi Dec 26, 2021 •

edited by jcouv

Loading

cston Feb 4, 2022 •

edited

Loading

cston Jan 4, 2022 •

edited by jcouv

Loading

AlekseyTs Jan 4, 2022 •

edited by jcouv

Loading