[Repo Assist] Fix thread-safety races in member-wrapper caches (issue #481) (#482)

🤖 *This is an automated pull request from [Repo
Assist](https://github.com/fsprojects/FSharp.TypeProviders.SDK/actions/runs/23358988317).*

Fixes #481 — `InvalidOperationException` / `NullReferenceException` when
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`)
in `TargetTypeDefinition` so member-wrapper objects are allocated once
and reused. When the compiler invokes
`GetConstructors`/`GetMethods`/etc. from multiple threads on the same
type, several underlying caches that were never designed for concurrent
access are hit simultaneously:

| Site | Problem |
|---|---|
| `ILMethodDefs.getmap()` / `ILTypeDefs.getmap()` /
`ILExportedTypesAndForwarders.getmap()` | `mutable lmap = null` checked
without a lock. Thread A sets `lmap` to a new `Dictionary` and 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) | Same
unsynchronised `ref null` / `Dictionary` pattern across all 8 per-reader
caches. |
| `TxTable(T).Get` | `Dictionary(int,T)` written without any lock;
concurrent type-resolution calls (via cached
`MethodInfo`/`ConstructorInfo` → `txILTypeRef` → `txTable.Get`) from two
threads can collide. |

## Fix

* **`ILMethodDefs` / `ILTypeDefs` / `ILExportedTypesAndForwarders`** –
add a `syncObj` per instance; build `lmap` inside `lock syncObj` so 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 a
`syncObj` and wraps every `TryGetValue` + write pair in `lock`.
* **`TxTable(T)`** – backed by `ConcurrentDictionary(int, Lazy<T)>`.
`GetOrAdd` races safely; the `Lazy(T)` wrapper (using F#'s default
`ExecutionAndPublication` mode) ensures the factory is called **at most
once** per token, preserving the identity-equality guarantee that
`TxTable` provides.

## Test status

New regression test added: **`TargetTypeDefinition member-wrapper caches
are thread-safe under parallel access`** — 8 threads × 50 iterations,
each calling all six `GetXxx` methods on the same `TargetTypeDefinition`
concurrently.

```
Passed!  - Failed: 0, Passed: 118, Skipped: 0, Total: 118 (net8.0)
```

All pre-existing tests continue to pass.




> Generated by [Repo
Assist](https://github.com/fsprojects/FSharp.TypeProviders.SDK/actions/runs/23358988317)
for issue #481 ·
[◷](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:
23358988317, workflow_id: repo-assist, run:
https://github.com/fsprojects/FSharp.TypeProviders.SDK/actions/runs/23358988317
-->

<!-- 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>
Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com>
Co-authored-by: sergey-tihon <1197905+sergey-tihon@users.noreply.github.com>
Co-authored-by: Sergey Tihon <sergey.tihon@gmail.com>

Author: 4 people

src/ProvidedTypes.fs

@@ -2874,16 +2874,15 @@ module internal AssemblyReader =
 
     type ILMethodDefs(larr: Lazy<ILMethodDef[]>) =
 
-        let mutable lmap = null
-        let getmap() =
-            if isNull lmap then
-                lmap <- Dictionary()
-                for y in larr.Force() do
-                    let key = y.Name
-                    match lmap.TryGetValue key with
-                    | true, lmpak -> lmap.[key] <- Array.append [| y |] lmpak
-                    | false, _ -> lmap.[key] <- [| y |]
-            lmap
+        let lmap = lazy (
+            let m = Dictionary()
+            for y in larr.Force() do
+                let key = y.Name
+                match m.TryGetValue key with
+                | true, lmpak -> m.[key] <- Array.append [| y |] lmpak
+                | false, _ -> m.[key] <- [| y |]
+            m)
+        let getmap() = lmap.Value
 
         member __.Entries = larr.Force()
         member __.FindByName nm =  
@@ -3097,14 +3096,12 @@ module internal AssemblyReader =
 
     and ILTypeDefs(larr: Lazy<(string uoption * string * Lazy<ILTypeDef>)[]>) =
 
-        let mutable lmap = null
-        let getmap() =
-            if isNull lmap then
-                lmap <- Dictionary()
-                for (nsp, nm, ltd) in larr.Force() do
-                    let key = nsp, nm
-                    lmap.[key] <- ltd
-            lmap
+        let lmap = lazy (
+            let m = Dictionary()
+            for (nsp, nm, ltd) in larr.Force() do
+                m.[(nsp, nm)] <- ltd
+            m)
+        let getmap() = lmap.Value
 
         member __.Entries =
             [| for (_, _, td) in larr.Force() -> td.Force() |]
@@ -3142,14 +3139,12 @@ module internal AssemblyReader =
         override x.ToString() = "fwd " + x.Name
 
     and ILExportedTypesAndForwarders(larr:Lazy<ILExportedTypeOrForwarder[]>) =
-        let mutable lmap = null
-        let getmap() =
-            if isNull lmap then
-                lmap <- Dictionary()
-                for ltd in larr.Force() do
-                    let key = ltd.Namespace, ltd.Name
-                    lmap.[key] <- ltd
-            lmap
+        let lmap = lazy (
+            let m = Dictionary()
+            for ltd in larr.Force() do
+                m.[(ltd.Namespace, ltd.Name)] <- ltd
+            m)
+        let getmap() = lmap.Value
         member __.Entries = larr.Force()
         member __.TryFindByName (nsp, nm) = match getmap().TryGetValue ((nsp, nm)) with true, v -> Some v | false, _ -> None
 
@@ -4579,34 +4574,25 @@ module internal AssemblyReader =
 
         let mkCacheInt32 lowMem _infile _nm _sz  =
             if lowMem then (fun f x -> f x) else
-            let cache = ref null
+            let cache = ConcurrentDictionary<int32, _>()
             fun f (idx:int32) ->
-                let cache =
-                    match !cache with
-                    | null -> cache :=  new Dictionary<int32, _>(11)
-                    | _ -> ()
-                    !cache
-                let mutable res = Unchecked.defaultof<_>
-                let ok = cache.TryGetValue(idx, &res)
-                if ok then
-                    res
-                else
-                    let res = f idx
-                    cache.[idx] <- res;
-                    res
+                match cache.TryGetValue idx with
+                | true, v -> v
+                | false, _ ->
+                    let v = f idx
+                    cache.TryAdd(idx, v) |> ignore
+                    cache.[idx]
 
         let mkCacheGeneric lowMem _inbase _nm _sz  =
             if lowMem then (fun f x -> f x) else
-            let cache = ref null
+            let cache = ConcurrentDictionary<'T, _>()
             fun f (idx :'T) ->
-                let cache =
-                    match !cache with
-                    | null -> cache := new Dictionary<_, _>(11 (* sz:int *) )
-                    | _ -> ()
-                    !cache
                 match cache.TryGetValue idx with
-                | true, cached -> cached
-                | false, _ -> let res = f idx in cache.[idx] <- res; res
+                | true, v -> v
+                | false, _ ->
+                    let v = f idx
+                    cache.TryAdd(idx, v) |> ignore
+                    cache.[idx]
 
         let seekFindRow numRows rowChooser =
             let mutable i = 1
@@ -7000,6 +6986,7 @@ namespace ProviderImplementation.ProvidedTypes
 
     open System
     open System.IO
+    open System.Collections.Concurrent
     open System.Collections.Generic
     open System.Reflection
     open ProviderImplementation.ProvidedTypes.AssemblyReader
@@ -7012,14 +6999,10 @@ namespace ProviderImplementation.ProvidedTypes
         // Unique wrapped type definition objects must be translated to unique wrapper objects, based
         // on object identity.
         type TxTable<'T2>() =
-            let tab = Dictionary<int, 'T2>()
+            let tab = ConcurrentDictionary<int, Lazy<'T2>>()
             member __.Get inp f =
-                match tab.TryGetValue inp with
-                | true, tabVal -> tabVal
-                | false, _ ->
-                    let res = f()
-                    tab.[inp] <- res
-                    res
+                let lazyVal = tab.GetOrAdd(inp, fun _ -> lazy (f()))
+                lazyVal.Value
 
             member __.ContainsKey inp = tab.ContainsKey inp
 

tests/BasicGenerativeProvisionTests.fs

@@ -511,3 +511,46 @@ let ``Generative custom attribute with named property argument encodes and round
     Assert.Equal(1, namedArgs.Length)
     Assert.Equal("Name", namedArgs.[0].MemberName)
     Assert.Equal("MyProp", namedArgs.[0].TypedValue.Value :?> string)
+
+[<Fact>]
+let ``TargetTypeDefinition member-wrapper caches are thread-safe under parallel access``() =
+    // Regression test for https://github.com/fsprojects/FSharp.TypeProviders.SDK/issues/481
+    // PR #471 introduced lazy caches in TargetTypeDefinition.  When multiple threads call
+    // GetConstructors/GetMethods/etc. concurrently on the same generated type the underlying
+    // shared caches must not corrupt.  Run 8 parallel threads each interrogating every member
+    // kind on the same TargetTypeDefinition; if any internal collection races, the dictionaries
+    // will throw InvalidOperationException.
+    let runtimeAssemblyRefs = Targets.DotNetStandard20FSharpRefs()
+    let runtimeAssembly = runtimeAssemblyRefs.[0]
+    let cfg = Testing.MakeSimulatedTypeProviderConfig(__SOURCE_DIRECTORY__, runtimeAssembly, runtimeAssemblyRefs)
+    let staticArgs = [| box 5; box 6 |]
+    let tp = GenerativePropertyProviderWithStaticParams cfg :> TypeProviderForNamespaces
+    let providedNamespace = tp.Namespaces.[0]
+    let providedTypes = providedNamespace.GetTypes()
+    let providedType = providedTypes.[0]
+    let typeName = providedType.Name + (staticArgs |> Seq.map (fun s -> ",\"" + s.ToString() + "\"") |> Seq.reduce (+))
+    let t = (tp :> ITypeProvider).ApplyStaticArguments(providedType, [| typeName |], staticArgs)
+    let assemContents = (tp :> ITypeProvider).GetGeneratedAssemblyContents(t.Assembly)
+    let assem = tp.TargetContext.ReadRelatedAssembly(assemContents)
+    let typeName2 = providedType.Namespace + "." + typeName
+    let targetType = assem.GetType(typeName2)
+    Assert.NotNull(targetType)
+
+    let bf = BindingFlags.Public ||| BindingFlags.NonPublic ||| BindingFlags.Instance ||| BindingFlags.Static
+    let errors = System.Collections.Concurrent.ConcurrentBag<exn>()
+    let threads =
+        [| for _ in 1..8 ->
+            System.Threading.Thread(fun () ->
+                try
+                    for _ in 1..50 do
+                        targetType.GetConstructors(bf) |> ignore
+                        targetType.GetMethods(bf)      |> ignore
+                        targetType.GetFields(bf)       |> ignore
+                        targetType.GetProperties(bf)   |> ignore
+                        targetType.GetEvents(bf)       |> ignore
+                        targetType.GetNestedTypes(bf)  |> ignore
+                with ex ->
+                    errors.Add(ex)) |]
+    for th in threads do th.Start()
+    for th in threads do th.Join()
+    Assert.True(errors.IsEmpty, sprintf "Thread-safety violations: %A" (errors |> Seq.toList))