Fix torn write of struct causing crash in symbol finder. #69927
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
| @@ -8,146 +8,145 @@ | |||
| using Roslyn.Utilities; | |||
There was a problem hiding this comment.
view with whitespace off.
| Contract.ThrowIfNull(value, nameof(value)); | ||
| Contract.ThrowIfNull(_concatenatedLowerCaseWords, nameof(_concatenatedLowerCaseWords)); | ||
| Contract.ThrowIfTrue(_nodes.IsDefault, $"{nameof(_nodes)}.{nameof(_nodes.IsDefault)}"); | ||
| Contract.ThrowIfTrue(_edges.IsDefault, $"{nameof(_edges)}.{nameof(_edges.IsDefault)}"); |
There was a problem hiding this comment.
this is the important part. ideally so we get a dump that tells us which of these is the problem.
There was a problem hiding this comment.
Is this going to stay in the PR since you think you've figured out the problem?
There was a problem hiding this comment.
yeah, i don't mind it :)
CyrusNajmabadi
changed the title
| { | ||
| get | ||
| { | ||
| _spellChecker ??= CreateSpellChecker(Checksum, _nodes); |
There was a problem hiding this comment.
SpellChecker? was a Nullable<SpellChecker>. This is just a pure struct which can have torn writes. By moving to a StrongBox<SpellChecker> we have teh runtime guarantee that if _spllChecker is not null then _spellChecker.Value is 'completely written'.
| { | ||
| Logger.Log(FunctionId.SymbolTreeInfo_ExceptionInCacheRead); | ||
| var rawSpellChecker = SpellChecker.TryReadFrom(reader); | ||
| spellChecker = rawSpellChecker is null ? null : new(rawSpellChecker.Value); |
There was a problem hiding this comment.
yes. if we can't read in teh spell checker (should never happen, but we're resilient to it anyways), we can still proceed as the new spell checker can be lazily created.
| /// <summary> | ||
| /// Explicitly boxed so that we can safely initialize/read this across threads without the need for a lock. | ||
| /// </summary> | ||
| private StrongBox<SpellChecker>? _spellChecker; |
There was a problem hiding this comment.
❗ Since _spellChecker.Value is never written after construction, and there are no other instantiations of the SpellChecker type, this change is functionally equivalent to (but much more complicated than) just making SpellChecker a reference type. You could further simplify things by using the helpers from #69017 to initialize the field on first use.
There was a problem hiding this comment.
The other option if you want to keep this a value type is to use this helper:
The implementation of EnsureInitialized contains a lock-free fast path for the case where initialized is true (set at the end of initialization). You may need to use PooledDelegates to avoid a delegate allocation on the fast path as well.
There was a problem hiding this comment.
Thanks. Moved to just be a refernece type. That fits best here for me :)
| SpellChecker.FindSimilarWords(ref similarNames.AsRef(), name, substringsAreSimilar: false); | ||
| // Ensure the spell checker is initialized. This is concurrency safe. Technically multiple threads may end | ||
| // up overwriting the field, but even if that happens, we are sure to see a fully written spell checker as | ||
| // the runtime guarantees that the initialize of the SpellChecker instnace completely written when we read |
There was a problem hiding this comment.
| // the runtime guarantees that the initialize of the SpellChecker instnace completely written when we read | |
| // the runtime guarantees that the initialize of the SpellChecker instance completely written when we read |
ghost
added this to the 
Fixes #69919