Make SourceTextData thread-safe with ConcurrentDictionary - #20113
Make SourceTextData thread-safe with ConcurrentDictionary#20113xperiandri wants to merge 2 commits into
SourceTextData thread-safe with ConcurrentDictionary#20113Conversation
Co-authored-by: xperiandri <2365592+xperiandri@users.noreply.github.com>
|
Fixes #20112 |
SourceTextData thread-safe with ConcurrentDictionary
T-Gro
left a comment
There was a problem hiding this comment.
🤖 This review was generated by AI (@expert-reviewer agent). Findings may contain inaccuracies — please verify independently.
Summary: Correct, well-scoped fix. Swapping the ResizeArray<SourceLineData option> for a ConcurrentDictionary<int, SourceLineData> removes the real memory-corruption hazard (concurrent extendTo/Add on a non-thread-safe list), and it also removes the unsafe side-effect where the getter previously mutated the backing store. ClearFrom and the setter stay behaviorally equivalent to the old code, since a missing key is indistinguishable from the previous None/uncached slot, and the clear loop still terminates at the first gap. Imports (System.Collections.Concurrent) and the (concurrencyLevel, capacity) constructor arguments are correct. No correctness, security, or performance regressions found.
| i <- i + 1 | ||
| while cont do | ||
| let removed, _ = data.TryRemove(i) | ||
| if removed then i <- i + 1 else cont <- false |
There was a problem hiding this comment.
Non-blocking observation: ConcurrentDictionary makes each individual entry access atomic, which fixes the memory-safety bug. However, the multi-line read-modify-write sequence in getFromRefreshedTokenCache (read [i] -> scanSourceLine -> set [i], chaining lexState, then ClearFrom(endLine+1)) is still not atomic as a whole. Two concurrent scans over overlapping line ranges on the same SourceTextData can interleave and briefly cache an inconsistent lex-state chain. This is a strict improvement over the old code (which could corrupt the list), and it self-heals on the next read via the IsValid/LexStateAtStartOfLine checks, so it is almost certainly acceptable. Worth noting only that the guarantee is container-level, not sequence-level, so the must be thread-safe comment does not imply a coherent snapshot across lines.
SourceTextDatais shared across concurrent editor operations (classification/tagging and symbol lookup), but itsResizeArraybacking store is not thread-safe.Changes
ResizeArray<SourceLineData option>withConcurrentDictionary<int, SourceLineData>.SourceTextData.[i]contract asSourceLineData optionusingTryGetValuein getter.Some vand removes onNonewithTryRemove.ClearFrom(n)now removes consecutive cached entries usingTryRemoveand stops on first missing index.This is the same fix prepared in my fork branch
copilot/fix-source-text-data-thread-safety.