[Repo Assist] Fix thread-safety races in TargetTypeDefinition member-wrapper caches#483
Closed
github-actions[bot] wants to merge 2 commits intomasterfrom
Closed
Conversation
Closes #481 Root cause: PR #471 added lazy caches (ctorDefs/methDefs/fieldDefs/etc.) in TargetTypeDefinition so wrapper objects are allocated once and shared across all GetConstructors/GetMethods/etc. calls. When the F# compiler invokes these from multiple threads concurrently, the lazies can be forced concurrently, and the underlying non-thread-safe caches race: * ILMethodDefs.getmap() / ILTypeDefs.getmap() / ILExportedTypesAndForwarders.getmap() used a mutable null-check pattern without synchronisation. One thread sets lmap to a new Dictionary and starts filling it; a second thread sees the non-null lmap and reads it while the first is still writing -> InvalidOperationException. * mkCacheInt32 / mkCacheGeneric (binary-reader caches) had the same unsynchronised ref-null pattern. * TxTable<T>.Get wrote to Dictionary<int,T> without a lock; concurrent type-resolution calls (txILTypeRef -> txTable.Get) from shared cached MethodInfo/ConstructorInfo objects could collide. Fixes: * ILMethodDefs / ILTypeDefs / ILExportedTypesAndForwarders: build lmap inside lock syncObj so the dictionary is fully populated before any reader can see it. Subsequent calls acquire the lock, check the already-set field and return immediately (single branch). * mkCacheInt32 / mkCacheGeneric: each cache now holds its own lock object and protects every TryGetValue/set_Item pair. * TxTable<T>: backed by ConcurrentDictionary<int, Lazy<T>> so that concurrent GetOrAdd calls for the same token race safely, with Lazy<T> guaranteeing the factory runs exactly once per token. Adds a thread-safety regression test: 8 threads × 50 iterations each calling GetConstructors/GetMethods/GetFields/GetProperties/GetEvents/ GetNestedTypes on the same TargetTypeDefinition simultaneously. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Mar 20, 2026
Member
|
Duplicate of #482 |
sergey-tihon
deleted the
repo-assist/fix-thread-safety-lazy-caches-481-9ed88c14244a34af
branch
dsyme
pushed a commit
that referenced
this pull request
Mar 23, 2026
…getTypeDefinition (#485) 🤖 *This is an automated PR from Repo Assist, an AI assistant for this repository.* ## Summary `TargetTypeDefinition.FullName`, `BaseType`, and `GetInterfaces()` each compute their result from immutable input data (`inp.Namespace`/`inp.Name`, `inp.Extends`, `inp.Implements`) but were recomputed on every call — allocating new strings/arrays and re-running type resolution each time. For large type providers with many types (e.g. SwaggerProvider), where the F# compiler queries these properties many times per type during type-checking, this saves repeated allocations and type-resolution work. ### Changes | Property | Before | After | |---|---|---| | `FullName` | String concatenation every call | Cached `lazy` — computed once, same `string` returned thereafter | | `BaseType` | `txILType` (type-resolution) every call | Cached `lazy` — resolved once | | `GetInterfaces()` | `Array.map txILType` (allocates new `Type[]`) every call | Cached `lazy` — resolved and allocated once | All three caches use F# `lazy` which defaults to `LazyThreadSafetyMode.ExecutionAndPublication`, so concurrent first-access from multiple F# compiler threads is safe. This is complementary to PR #471 (which cached member-wrapper arrays) and does not touch the thread-safety areas being addressed by PRs #482/#483. ## Test Status ``` Passed! - Failed: 0, Passed: 117, Skipped: 0, Total: 117 (net8.0) ``` All 117 pre-existing tests pass. The `netstandard2.0` build target ran OOM on the CI machine (infrastructure issue, not caused by this change — the same issue affects master); the `net8.0` build and tests both pass cleanly. > Generated by [Repo Assist](https://github.com/fsprojects/FSharp.TypeProviders.SDK/actions/runs/23367946628) · [◷](https://github.com/search?q=repo%3Afsprojects%2FFSharp.TypeProviders.SDK+%22gh-aw-workflow-id%3A+repo-assist%22&type=pullrequests) > > To install this [agentic workflow](https://github.com/githubnext/agentics/tree/d1d884596e62351dd652ae78465885dd32f0dd7d/workflows/repo-assist.md), run > ``` > gh aw add githubnext/agentics@d1d8845 > ``` <!-- gh-aw-agentic-workflow: Repo Assist, engine: copilot, id: 23367946628, workflow_id: repo-assist, run: https://github.com/fsprojects/FSharp.TypeProviders.SDK/actions/runs/23367946628 --> <!-- gh-aw-workflow-id: repo-assist --> --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 This PR was created by Repo Assist, an automated AI assistant.
Closes #481
Root Cause
PR #471 introduced
lazycaches (ctorDefs,methDefs, etc.) inTargetTypeDefinitionso that wrapper objects (ConstructorInfo,MethodInfo, …) are created once and shared across callers. This exposed pre-existing but latent race conditions in the downstream data structures, because the same wrapper objects are now accessed from multiple F# compiler threads simultaneously.There were three independent races:
ILMethodDefs.getmap()/ILTypeDefs.getmap()/ILExportedTypesAndForwarders.getmap()lmap <- Dictionary()and starts filling it; Thread B sees non-nulllmapand callsTryGetValuewhile A is still inserting →InvalidOperationException: Operations that change non-concurrent collections must have exclusive accessmkCacheInt32/mkCacheGeneric(binary-reader caches)ref null+ unsynchronisedDictionarypattern; two threads both see null, both allocate a newDictionary, one overwrites the other → later readers get a stale/empty cache or a partially-filled oneTxTable(T).GetDictionary(int,'T2)with no locking; concurrenttxILTypecalls sharing the sameTargetTypeDefinitionwrapper race ondict[inp] <- result→InvalidOperationExceptionorNullReferenceExceptionThe reported
InvalidOperationException: Operations that change non-concurrent collections must have exclusive access(Ubuntu) andNullReferenceException(Windows) are both manifestations of these races.Fix
ILMethodDefs.getmap()/ILTypeDefs.getmap()/ILExportedTypesAndForwarders.getmap(): Added asyncObj = obj()per instance;getmap()builds the map into a local value underlock syncObjand assignslmap <- monly after construction is complete.mkCacheInt32/mkCacheGeneric: Replaced theref null+ unsynchronisedDictionarypattern with an eagerly-allocatedDictionary+syncObj; all lookups and writes are wrapped inlock syncObj.TxTable(T).Get: ReplacedDictionary(int,'T2)with aConcurrentDictionary(int, Lazy<'T2)>;GetOrAddis race-safe, and wrapping the value inLazy(defaultExecutionAndPublicationmode) ensures the factory runs exactly once per token even under concurrent access. This preserves the identity guarantee: same type token → sameTargetTypeDefinitionobject.Test Status
Added regression test:
TargetTypeDefinition member-wrapper caches are thread-safe under parallel access— 8 threads × 50 iterations, all sixGetXxxmethods called concurrently.All 118 tests pass (117 pre-existing + 1 new).