From 70653c143d13fc94110b00fa347cce0ae00f88c0 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Fri, 20 Mar 2026 19:53:10 +0000 Subject: [PATCH 1/5] Fix thread-safety races in TargetTypeDefinition member-wrapper caches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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.Get wrote to Dictionary 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: backed by ConcurrentDictionary> so that concurrent GetOrAdd calls for the same token race safely, with Lazy 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> --- src/ProvidedTypes.fs | 104 ++++++++++++------------- tests/BasicGenerativeProvisionTests.fs | 43 ++++++++++ 2 files changed, 94 insertions(+), 53 deletions(-) diff --git a/src/ProvidedTypes.fs b/src/ProvidedTypes.fs index d755119..087d185 100644 --- a/src/ProvidedTypes.fs +++ b/src/ProvidedTypes.fs @@ -2874,16 +2874,19 @@ module internal AssemblyReader = type ILMethodDefs(larr: Lazy) = - let mutable lmap = null + let mutable lmap : Dictionary = null + let syncObj = obj() 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 + lock syncObj (fun () -> + if isNull lmap then + 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 |] + lmap <- m + lmap) member __.Entries = larr.Force() member __.FindByName nm = @@ -3097,14 +3100,16 @@ module internal AssemblyReader = and ILTypeDefs(larr: Lazy<(string uoption * string * Lazy)[]>) = - let mutable lmap = null + let mutable lmap : Dictionary> = null + let syncObj = obj() let getmap() = - if isNull lmap then - lmap <- Dictionary() - for (nsp, nm, ltd) in larr.Force() do - let key = nsp, nm - lmap.[key] <- ltd - lmap + lock syncObj (fun () -> + if isNull lmap then + let m = Dictionary() + for (nsp, nm, ltd) in larr.Force() do + m.[(nsp, nm)] <- ltd + lmap <- m + lmap) member __.Entries = [| for (_, _, td) in larr.Force() -> td.Force() |] @@ -3142,14 +3147,16 @@ module internal AssemblyReader = override x.ToString() = "fwd " + x.Name and ILExportedTypesAndForwarders(larr:Lazy) = - let mutable lmap = null + let mutable lmap : Dictionary = null + let syncObj = obj() let getmap() = - if isNull lmap then - lmap <- Dictionary() - for ltd in larr.Force() do - let key = ltd.Namespace, ltd.Name - lmap.[key] <- ltd - lmap + lock syncObj (fun () -> + if isNull lmap then + let m = Dictionary() + for ltd in larr.Force() do + m.[(ltd.Namespace, ltd.Name)] <- ltd + lmap <- m + lmap) member __.Entries = larr.Force() member __.TryFindByName (nsp, nm) = match getmap().TryGetValue ((nsp, nm)) with true, v -> Some v | false, _ -> None @@ -4579,34 +4586,29 @@ module internal AssemblyReader = let mkCacheInt32 lowMem _infile _nm _sz = if lowMem then (fun f x -> f x) else - let cache = ref null + let cache = Dictionary() + let syncObj = obj() fun f (idx:int32) -> - let cache = - match !cache with - | null -> cache := new Dictionary(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 + lock syncObj (fun () -> + let mutable res = Unchecked.defaultof<_> + if cache.TryGetValue(idx, &res) then res + else + let v = f idx + cache.[idx] <- v + v) let mkCacheGeneric lowMem _inbase _nm _sz = if lowMem then (fun f x -> f x) else - let cache = ref null + let cache = Dictionary<'T, _>() + let syncObj = obj() 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 + lock syncObj (fun () -> + match cache.TryGetValue idx with + | true, cached -> cached + | false, _ -> + let v = f idx + cache.[idx] <- v + v) let seekFindRow numRows rowChooser = let mutable i = 1 @@ -7012,14 +7014,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() + let tab = System.Collections.Concurrent.ConcurrentDictionary>() 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 diff --git a/tests/BasicGenerativeProvisionTests.fs b/tests/BasicGenerativeProvisionTests.fs index 36d7ec1..699dc31 100644 --- a/tests/BasicGenerativeProvisionTests.fs +++ b/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) + +[] +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() + 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)) From c38ff71bc14c057a4b91dd665741921e6dc7e910 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Fri, 20 Mar 2026 20:02:25 +0000 Subject: [PATCH 2/5] ci: trigger checks From c7c8212e146cb9706426d7f4c5002646adfb5db9 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Sun, 22 Mar 2026 08:05:02 +0100 Subject: [PATCH 3/5] Simplify mkCacheInt32/mkCacheGeneric to use ConcurrentDictionary (#486) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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() 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 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> --- src/ProvidedTypes.fs | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/src/ProvidedTypes.fs b/src/ProvidedTypes.fs index 087d185..12bfde7 100644 --- a/src/ProvidedTypes.fs +++ b/src/ProvidedTypes.fs @@ -4586,29 +4586,25 @@ module internal AssemblyReader = let mkCacheInt32 lowMem _infile _nm _sz = if lowMem then (fun f x -> f x) else - let cache = Dictionary() - let syncObj = obj() + let cache = ConcurrentDictionary() fun f (idx:int32) -> - lock syncObj (fun () -> - let mutable res = Unchecked.defaultof<_> - if cache.TryGetValue(idx, &res) then res - else - let v = f idx - cache.[idx] <- v - v) + 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 = Dictionary<'T, _>() - let syncObj = obj() + let cache = ConcurrentDictionary<'T, _>() fun f (idx :'T) -> - lock syncObj (fun () -> - match cache.TryGetValue idx with - | true, cached -> cached - | false, _ -> - let v = f idx - cache.[idx] <- v - v) + match cache.TryGetValue idx with + | true, v -> v + | false, _ -> + let v = f idx + cache.TryAdd(idx, v) |> ignore + cache.[idx] let seekFindRow numRows rowChooser = let mutable i = 1 From 31ad54fa4cea3f037d9d1e67525ab2893a1b9b04 Mon Sep 17 00:00:00 2001 From: Sergey Tihon Date: Sun, 22 Mar 2026 08:27:10 +0100 Subject: [PATCH 4/5] Replace Dictionary+syncObj+lock with Lazy in IL reader caches 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. --- src/ProvidedTypes.fs | 54 +++++++++++++++++--------------------------- 1 file changed, 21 insertions(+), 33 deletions(-) diff --git a/src/ProvidedTypes.fs b/src/ProvidedTypes.fs index 12bfde7..cb1ffe4 100644 --- a/src/ProvidedTypes.fs +++ b/src/ProvidedTypes.fs @@ -2874,19 +2874,15 @@ module internal AssemblyReader = type ILMethodDefs(larr: Lazy) = - let mutable lmap : Dictionary = null - let syncObj = obj() - let getmap() = - lock syncObj (fun () -> - if isNull lmap then - 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 |] - lmap <- m - 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 = @@ -3100,16 +3096,12 @@ module internal AssemblyReader = and ILTypeDefs(larr: Lazy<(string uoption * string * Lazy)[]>) = - let mutable lmap : Dictionary> = null - let syncObj = obj() - let getmap() = - lock syncObj (fun () -> - if isNull lmap then - let m = Dictionary() - for (nsp, nm, ltd) in larr.Force() do - m.[(nsp, nm)] <- ltd - lmap <- m - 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() |] @@ -3147,16 +3139,12 @@ module internal AssemblyReader = override x.ToString() = "fwd " + x.Name and ILExportedTypesAndForwarders(larr:Lazy) = - let mutable lmap : Dictionary = null - let syncObj = obj() - let getmap() = - lock syncObj (fun () -> - if isNull lmap then - let m = Dictionary() - for ltd in larr.Force() do - m.[(ltd.Namespace, ltd.Name)] <- ltd - lmap <- m - 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 From ec4ff82562b89d9b8d2be3a7985ed7f75dd5927c Mon Sep 17 00:00:00 2001 From: Sergey Tihon Date: Sun, 22 Mar 2026 08:31:06 +0100 Subject: [PATCH 5/5] Remove fully qualified ConcurrentDictionary in TxTable by adding open --- src/ProvidedTypes.fs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/ProvidedTypes.fs b/src/ProvidedTypes.fs index cb1ffe4..2ddda76 100644 --- a/src/ProvidedTypes.fs +++ b/src/ProvidedTypes.fs @@ -6986,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 @@ -6998,7 +6999,7 @@ namespace ProviderImplementation.ProvidedTypes // Unique wrapped type definition objects must be translated to unique wrapper objects, based // on object identity. type TxTable<'T2>() = - let tab = System.Collections.Concurrent.ConcurrentDictionary>() + let tab = ConcurrentDictionary>() member __.Get inp f = let lazyVal = tab.GetOrAdd(inp, fun _ -> lazy (f())) lazyVal.Value