[Repo Assist] Fix thread-safety races in member-wrapper caches (issue #481)#482
Merged
sergey-tihon merged 5 commits intomasterfrom Mar 22, 2026
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 21, 2026
There was a problem hiding this comment.
Pull request overview
This PR addresses thread-safety races in the SDK’s metadata-backed reflection wrappers that can be accessed concurrently by the F# compiler during parallel compilation, preventing intermittent InvalidOperationException/NullReferenceException failures (issue #481).
Changes:
- Adds synchronization to lazy dictionary materialization in
ILMethodDefs,ILTypeDefs, andILExportedTypesAndForwarders. - Makes metadata row caches (
mkCacheInt32/mkCacheGeneric) safe under concurrent access by guarding cache reads/writes. - Reworks
TxTable<'T>to useConcurrentDictionary<int, Lazy<'T>>and adds a regression test exercising concurrent member enumeration on a generated target type.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/ProvidedTypes.fs |
Adds locking / concurrent structures to prevent concurrent mutations of internal caches used during metadata-backed reflection. |
tests/BasicGenerativeProvisionTests.fs |
Adds a regression test that concurrently calls GetConstructors/GetMethods/etc. on the same generated TargetTypeDefinition. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Member
|
@copilot i don't like this implementation, it is too complicated. rework it using ConcurrentDictionary |
Contributor
|
@sergey-tihon I've opened a new pull request, #486, to work on those changes. Once the pull request is ready, I'll request review from you. |
`mkCacheInt32` and `mkCacheGeneric` were using `Dictionary` + `lock
syncObj`, serializing all cache access including expensive metadata
decode work.
## Changes
- **`mkCacheInt32` / `mkCacheGeneric`**: replaced `Dictionary` + `lock`
with `ConcurrentDictionary` using `TryGetValue` / `TryAdd`. No locks, no
`syncObj`.
```fsharp
let mkCacheInt32 lowMem _infile _nm _sz =
if lowMem then (fun f x -> f x) else
let cache = ConcurrentDictionary<int32, _>()
fun f (idx:int32) ->
match cache.TryGetValue idx with
| true, v -> v
| false, _ ->
let v = f idx
cache.TryAdd(idx, v) |> ignore
cache.[idx]
```
`GetOrAdd(key, factory)` was not usable here — F# type inference cannot
disambiguate it from `GetOrAdd(key, value)` when the value type is an
unconstrained generic. `TryAdd` has no overloads and sidesteps the
issue. On a concurrent cache miss, `f idx` may be computed twice (both
`GetOrAdd` with factory and this pattern share that behaviour per the
.NET docs), which is acceptable since these factories are pure metadata
reads.
<!-- START COPILOT CODING AGENT TIPS -->
---
⌨️ Start Copilot coding agent tasks without leaving your editor —
available in [VS Code](https://gh.io/cca-vs-code-docs), [Visual
Studio](https://gh.io/cca-visual-studio-docs), [JetBrains
IDEs](https://gh.io/cca-jetbrains-docs) and
[Eclipse](https://gh.io/cca-eclipse-docs).
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: sergey-tihon <1197905+sergey-tihon@users.noreply.github.com>
…ches The ILMethodDefs, ILTypeDefs and ILExportedTypesAndForwarders caches are build-once-read-many, so a simple lazy value provides thread-safe init without introducing mutable fields, lock objects or ConcurrentDictionary overhead.
sergey-tihon
approved these changes
Mar 22, 2026
sergey-tihon
deleted the
repo-assist/fix-thread-safety-lazy-caches-481-a00dbff9b73cb3e8
branch
Member
|
I hope it ok that i've merged this fix, because master was broken. |
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
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 is an automated pull request from Repo Assist.
Fixes #481 —
InvalidOperationException/NullReferenceExceptionwhen the F# compiler accesses type-provider types from multiple parallel compilation threads.Root cause
PR #471 introduced lazy caches (
ctorDefs/methDefs/fieldDefs/eventDefs/propDefs/nestedDefs) inTargetTypeDefinitionso member-wrapper objects are allocated once and reused. When the compiler invokesGetConstructors/GetMethods/etc. from multiple threads on the same type, several underlying caches that were never designed for concurrent access are hit simultaneously:ILMethodDefs.getmap()/ILTypeDefs.getmap()/ILExportedTypesAndForwarders.getmap()mutable lmap = nullchecked without a lock. Thread A setslmapto a newDictionaryand starts filling it; Thread B sees the non-null value and reads from it while Thread A is writing →InvalidOperationException.mkCacheInt32/mkCacheGeneric(binary-reader row caches)ref null/Dictionarypattern across all 8 per-reader caches.TxTable(T).GetDictionary(int,T)written without any lock; concurrent type-resolution calls (via cachedMethodInfo/ConstructorInfo→txILTypeRef→txTable.Get) from two threads can collide.Fix
ILMethodDefs/ILTypeDefs/ILExportedTypesAndForwarders– add asyncObjper instance; buildlmapinsidelock syncObjso the dictionary is fully populated before any reader sees it. Subsequent calls acquire the lock, see the already-set field, and return immediately.mkCacheInt32/mkCacheGeneric– each closure now owns asyncObjand wraps everyTryGetValue+ write pair inlock.TxTable(T)– backed byConcurrentDictionary(int, Lazy<T)>.GetOrAddraces safely; theLazy(T)wrapper (using F#'s defaultExecutionAndPublicationmode) ensures the factory is called at most once per token, preserving the identity-equality guarantee thatTxTableprovides.Test status
New regression test added:
TargetTypeDefinition member-wrapper caches are thread-safe under parallel access— 8 threads × 50 iterations, each calling all sixGetXxxmethods on the sameTargetTypeDefinitionconcurrently.All pre-existing tests continue to pass.