Parallel compilation fix for TP SDK - #20102
Conversation
… parallel compilation issues
|
As far as I understand this goes under "tests" so there is nothing to release, for "release notes". |
github-actions
Bot
added
the
AI-Tooling-Check-Scanned-Clean
T-Gro
left a comment
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.
This is a faithful port of the parallel-compilation fixes from FSharp.TypeProviders.SDK into the test-data type provider. Overall it looks correct. I traced the new two-lock design and believe it is deadlock-free: typeTablesLock is only ever held for table lookups and target-type shell creation (never while realizing members), while member realization takes realizationLock first and only then reaches into typeTablesLock via convMemberDefToTgt. So the acquisition order is consistently realizationLock → typeTablesLock. Two low-severity observations below — neither is blocking.
| type ILPropertyDefs(larr: Lazy<ILPropertyDef[]>) = | ||
| let lmap = lazy ( | ||
| let d = Dictionary<string, ILPropertyDef>() | ||
| for p in larr.Force() do d.[p.Name] <- p |
There was a problem hiding this comment.
Behavioral change worth confirming: the new dictionary-based TryFindByName uses last-wins (d.[p.Name] <- p), whereas the previous Entries |> Array.tryFind (fun md -> md.Name = name) returned the first matching entry. For IL properties this differs when a type has name-overloaded members (e.g. an indexer Item with multiple index-parameter signatures): GetPropertyImpl(name, ...) will now resolve to the last such property instead of the first. The same first→last shift applies to the analogous field/event/nested-type dict lookups. In practice fields/events/nested types can't share a name, so only overloaded properties are affected; likely harmless for the test providers here, but flagging in case any test relies on the previously-returned member.
| // same type's realization on the same thread is fine. The queue writers (AddMember et al.) stay | ||
| // unlocked by design: they run either during provider construction, before the compiler observes | ||
| // the type, or from a delayed factory that already holds this lock via realization. | ||
| let realizationLock = obj() |
There was a problem hiding this comment.
The correctness of this two-lock scheme depends on a strict acquisition order: realizationLock (this per-type lock) must always be taken before the context-wide typeTablesLock, never the reverse. That invariant holds today because typeTablesLock is only held for table lookups and target-type shell creation, and the getFreshMethods/GetMembersFromCursor factories acquire the source type's realizationLock before calling convMemberDefToTgt (which reaches typeTablesLock). Worth a short comment recording this ordering invariant, since a future edit that realizes members (e.g. calls getMembers/GetMethods) while holding typeTablesLock would introduce an AB–BA deadlock under ParallelCompilation.
T-Gro
left a comment
T-Gro
left a comment
There was a problem hiding this comment.
Approving as this is just porting reality to our copy in tests.
github-project-automation
Bot
moved this from New
to In Progress
in F# Compiler and Tooling
2 participants
Description
This is a ported from https://github.com/fsprojects/FSharp.TypeProviders.SDK
Fixes #20060