Make SourceTextData thread-safe with ConcurrentDictionary - #20113
Make SourceTextData thread-safe with ConcurrentDictionary#20113xperiandri wants to merge 5 commits into
SourceTextData thread-safe with ConcurrentDictionary#20113Conversation
|
Fixes #20112 |
xperiandri
changed the title
SourceTextData thread-safe with ConcurrentDictionary
github-actions
Bot
added
the
AI-Tooling-Check-Scanned-Clean
| 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.
There was a problem hiding this comment.
Agreed — updated the comment above SourceTextData to clarify the guarantee is container-level (per-entry atomicity via ConcurrentDictionary), not a coherent snapshot across a range of lines. Concurrent overlapping scans can still interleave, but the cache self-heals on the next read via the IsValid/LexStateAtStartOfLine checks, as you noted. No functional change needed beyond the comment wording.
8bee41a to
bb0043a
Compare
❗ Release notes requiredYou can open this PR in browser to add release notes: open in github.dev
|
bb0043a to
84f9fe8
Compare
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
added
⚠️ Affects-Bootstrap
|
Pushed another round of improvements building on the thread-safety fix:
All affected projects ( |
4ff5e30 to
c20c864
Compare
c20c864 to
287c161
Compare
T-Gro
left a comment
T-Gro
left a comment
There was a problem hiding this comment.
One note on public API compatibility.
| type FSharpLineTokenizer = | ||
| /// Scan one token from the line | ||
| member ScanToken: lexState: FSharpTokenizerLexState -> FSharpTokenInfo option * FSharpTokenizerLexState | ||
| member ScanToken: lexState: FSharpTokenizerLexState -> struct (FSharpTokenInfo voption * FSharpTokenizerLexState) |
There was a problem hiding this comment.
🤖🕵️ Changing ScanToken to return struct (... voption * ...) breaks source and binary compatibility for every existing caller of this public member.
member ScanToken: lexState -> struct (FSharpTokenInfo voption * FSharpTokenizerLexState)Keep the existing member and add a separate allocation-free member for hot paths.
There was a problem hiding this comment.
What is our strategy on allocation-free code?
Ideally, all the code must return value tuples, unless a component of a tuple is a huge struct.
Would you like all such changes to be additions instead of replacements?
There was a problem hiding this comment.
@T-Gro could you direct me what to do with this?
287c161 to
4b3e2af
Compare
4b3e2af to
b5b7fcb
Compare
b5b7fcb to
4c499e3
Compare
Co-authored-by: xperiandri <2365592+xperiandri@users.noreply.github.com>
- Reuse already-materialized line contents in SourceTextData cache lookups instead of re-stringifying the line via IsValid - processToken now takes the token as a parameter instead of reading a mutable voption repeatedly; scanAndColorNextToken returns whether a token was scanned, removing the tokenInfoOption mutable - tokensUnderCursor uses List.filter instead of a list comprehension - Hoist forbiddenSymbolNameChars to a module-level private array so it is not recreated on every isValidNameForSymbol call
…ct return Change ScanToken's signature from 'FSharpTokenInfo option * FSharpTokenizerLexState' to 'struct (FSharpTokenInfo voption * FSharpTokenizerLexState)', eliminating a per-token option allocation on the hot tokenization path. Update all in-tree callers: FSharpChecker.TokenizeLine, the editor Tokenizer, and the deprecated FSharp.LanguageService colorizer/scanner (Colorize.fs, Intellisense.fs). Also clean up incidental trailing whitespace in Intellisense.fs picked up by formatting.
4c499e3 to
3d32352
Compare
This comment has been minimized.
This comment has been minimized.
… caught Address review comment: add a concurrent test against one shared SourceTextData, covering overlapping reads and invalidation. The test alternates classifySpans/getSymbolAtPosition calls across many threads on one document, interleaving two text versions of it (one with an unclosed block comment) so cache invalidation is exercised alongside concurrent reads. It reliably reproduced a real bug in getFromRefreshedTokenCache: after walking back to find the nearest valid cache entry at scanStartLine, the resume lex state was read from scanStartLine - 1 - one entry lower than the one just validated, and never checked itself. Under concurrent scans of different text on the same document, that neighboring entry can be stale, so classification would carry over a wrong lex state (typically "still inside a comment"), turning code after that point into one giant comment span, or occasionally throwing on a missing entry (silently swallowed by Assert.Exception, dropping the classification for that call). Fixed by reading LexStateAtStartOfLine off scanStartLine itself, the entry the preceding loop already confirmed valid. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
🔍 Tooling Safety Check — Affects-Bootstrap, Affects-Build-Infra
|
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.