Simplify GlobalTypeMapper locking and fix races (#6538)

NinoFloris · NinoFloris · commit 4a55f7ceff2b · 2026-04-20T22:04:53.000+02:00
(cherry picked from commit 44f1394)
diff --git a/src/Npgsql/TypeMapping/GlobalTypeMapper.cs b/src/Npgsql/TypeMapping/GlobalTypeMapper.cs
@@ -3,8 +3,6 @@
 using System.Diagnostics.CodeAnalysis;
 using System.Linq;
 using System.Text.Json;
-using System.Text.Json.Nodes;
-using System.Threading;
 using Npgsql.Internal;
 using Npgsql.Internal.Postgres;
 using Npgsql.Internal.ResolverFactories;
@@ -16,87 +14,67 @@ namespace Npgsql.TypeMapping;
 sealed class GlobalTypeMapper : INpgsqlTypeMapper
 {
     readonly UserTypeMapper _userTypeMapper = new();
-    readonly List<PgTypeInfoResolverFactory> _pluginResolverFactories = new();
-    readonly ReaderWriterLockSlim _lock = new();
-    PgTypeInfoResolverFactory[] _typeMappingResolvers = Array.Empty<PgTypeInfoResolverFactory>();
+    readonly List<PgTypeInfoResolverFactory> _pluginResolverFactories = [];
+    readonly object _sync = new();
+    PgTypeInfoResolverFactory[] _typeMappingResolvers = [];
 
     internal List<HackyEnumTypeMapping> HackyEnumTypeMappings { get; } = new();
 
     internal IEnumerable<PgTypeInfoResolverFactory> GetPluginResolverFactories()
     {
-        var resolvers = new List<PgTypeInfoResolverFactory>();
-        _lock.EnterReadLock();
-        try
-        {
-            resolvers.AddRange(_pluginResolverFactories);
-        }
-        finally
-        {
-            _lock.ExitReadLock();
-        }
-
-        return resolvers;
+        lock (_sync)
+            return new List<PgTypeInfoResolverFactory>(_pluginResolverFactories);
     }
 
     internal PgTypeInfoResolverFactory? GetUserMappingsResolverFactory()
     {
-        _lock.EnterReadLock();
-        try
-        {
+        lock (_sync)
             return _userTypeMapper.Items.Count > 0 ? _userTypeMapper : null;
-        }
-        finally
-        {
-            _lock.ExitReadLock();
-        }
     }
 
     internal void AddGlobalTypeMappingResolvers(PgTypeInfoResolverFactory[] factories, Func<PgTypeInfoResolverChainBuilder>? builderFactory = null, bool overwrite = false)
     {
-        // Good enough logic to prevent SlimBuilder overriding the normal Builder.
-        if (overwrite || factories.Length > _typeMappingResolvers.Length)
+        lock (_sync)
         {
-            _builderFactory = builderFactory;
-            _typeMappingResolvers = factories;
-            ResetTypeMappingCache();
+            // Good enough logic to prevent SlimBuilder overriding the normal Builder.
+            if (overwrite || factories.Length > _typeMappingResolvers.Length)
+            {
+                _builderFactory = builderFactory;
+                _typeMappingResolvers = factories;
+                _typeMappingOptions = null;
+            }
         }
     }
 
-    void ResetTypeMappingCache() => _typeMappingOptions = null;
-
     PgSerializerOptions? _typeMappingOptions;
     Func<PgTypeInfoResolverChainBuilder>? _builderFactory;
     JsonSerializerOptions? _jsonSerializerOptions;
 
-    PgSerializerOptions TypeMappingOptions
+    PgSerializerOptions TypeMappingOptions => _typeMappingOptions ?? BuildTypeMappingOptions();
+
+    PgSerializerOptions BuildTypeMappingOptions()
     {
-        get
+        lock (_sync)
         {
-            if (_typeMappingOptions is not null)
-                return _typeMappingOptions;
-
-            _lock.EnterReadLock();
-            try
+            if (_typeMappingOptions is { } existing)
+                return existing;
+
+            var builder = _builderFactory?.Invoke() ?? new();
+            builder.AppendResolverFactory(_userTypeMapper);
+            foreach (var factory in _pluginResolverFactories)
+                builder.AppendResolverFactory(factory);
+            foreach (var factory in _typeMappingResolvers)
+                builder.AppendResolverFactory(factory);
+            var chain = builder.Build();
+            var options = new PgSerializerOptions(PostgresMinimalDatabaseInfo.DefaultTypeCatalog, chain)
             {
-                var builder = _builderFactory?.Invoke() ?? new();
-                builder.AppendResolverFactory(_userTypeMapper);
-                foreach (var factory in _pluginResolverFactories)
-                    builder.AppendResolverFactory(factory);
-                foreach (var factory in _typeMappingResolvers)
-                    builder.AppendResolverFactory(factory);
-                var chain = builder.Build();
-                return _typeMappingOptions = new(PostgresMinimalDatabaseInfo.DefaultTypeCatalog, chain)
-                {
-                    // This means we don't ever have a missing oid for a datatypename as our canonical format is datatypenames.
-                    PortableTypeIds = true,
-                    // Don't throw if our catalog doesn't know the datatypename.
-                    IntrospectionMode = true
-                };
-            }
-            finally
-            {
-                _lock.ExitReadLock();
-            }
+                // This means we don't ever have a missing oid for a datatypename as our canonical format is datatypenames.
+                PortableTypeIds = true,
+                // Don't throw if our catalog doesn't know the datatypename.
+                IntrospectionMode = true
+            };
+            _typeMappingOptions = options;
+            return options;
         }
     }
 
@@ -126,8 +104,7 @@ static GlobalTypeMapper()
     /// <inheritdoc />
     public void AddTypeInfoResolverFactory(PgTypeInfoResolverFactory factory)
     {
-        _lock.EnterWriteLock();
-        try
+        lock (_sync)
         {
             var type = factory.GetType();
 
@@ -145,51 +122,20 @@ public void AddTypeInfoResolverFactory(PgTypeInfoResolverFactory factory)
             }
 
             _pluginResolverFactories.Insert(0, factory);
-            ResetTypeMappingCache();
-        }
-        finally
-        {
-            _lock.ExitWriteLock();
+            _typeMappingOptions = null;
         }
     }
 
-    void ReplaceTypeInfoResolverFactory(PgTypeInfoResolverFactory factory)
-    {
-        _lock.EnterWriteLock();
-        try
-        {
-            var type = factory.GetType();
-
-            for (var i = 0; i < _pluginResolverFactories.Count; i++)
-            {
-                if (_pluginResolverFactories[i].GetType() == type)
-                {
-                    _pluginResolverFactories[i] = factory;
-                    break;
-                }
-            }
-
-            ResetTypeMappingCache();
-        }
-        finally
-        {
-            _lock.ExitWriteLock();
-        }
-    }
 
     /// <inheritdoc />
     public void Reset()
     {
-        _lock.EnterWriteLock();
-        try
+        lock (_sync)
         {
             _pluginResolverFactories.Clear();
             _userTypeMapper.Items.Clear();
             HackyEnumTypeMappings.Clear();
-        }
-        finally
-        {
-            _lock.ExitWriteLock();
+            _typeMappingOptions = null;
         }
     }
 
@@ -203,9 +149,25 @@ public INpgsqlNameTranslator DefaultNameTranslator
     /// <inheritdoc />
     public INpgsqlTypeMapper ConfigureJsonOptions(JsonSerializerOptions serializerOptions)
     {
-        _jsonSerializerOptions = serializerOptions;
-        // If JsonTypeInfoResolverFactory exists we replace it with a configured instance on the same index of the array.
-        ReplaceTypeInfoResolverFactory(new JsonTypeInfoResolverFactory(serializerOptions));
+        lock (_sync)
+        {
+            _jsonSerializerOptions = serializerOptions;
+
+            // If JsonTypeInfoResolverFactory exists we replace it with a configured instance on the same index of the array.
+            var factory = new JsonTypeInfoResolverFactory(serializerOptions);
+            var type = factory.GetType();
+
+            for (var i = 0; i < _pluginResolverFactories.Count; i++)
+            {
+                if (_pluginResolverFactories[i].GetType() == type)
+                {
+                    _pluginResolverFactories[i] = factory;
+                    break;
+                }
+            }
+
+            _typeMappingOptions = null;
+        }
         return this;
     }
 
@@ -216,7 +178,9 @@ public INpgsqlTypeMapper EnableDynamicJson(
         Type[]? jsonbClrTypes = null,
         Type[]? jsonClrTypes = null)
     {
-        AddTypeInfoResolverFactory(new JsonDynamicTypeInfoResolverFactory(jsonbClrTypes, jsonClrTypes, _jsonSerializerOptions));
+        // Use a re-entered lock to add the read of _jsonSerializerOptions to the total scope.
+        lock (_sync)
+            AddTypeInfoResolverFactory(new JsonDynamicTypeInfoResolverFactory(jsonbClrTypes, jsonClrTypes, _jsonSerializerOptions));
         return this;
     }
 
@@ -241,90 +205,68 @@ public INpgsqlTypeMapper EnableUnmappedTypes()
     /// <inheritdoc />
     public INpgsqlTypeMapper MapEnum<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicFields)] TEnum>(string? pgName = null, INpgsqlNameTranslator? nameTranslator = null) where TEnum : struct, Enum
     {
-        _lock.EnterWriteLock();
-        try
+        lock (_sync)
         {
             _userTypeMapper.MapEnum<TEnum>(pgName, nameTranslator);
 
-            // Temporary hack for EFCore.PG enum mapping compat
+		    // Temporary hack for EFCore.PG enum mapping compat
             if (_userTypeMapper.Items.FirstOrDefault(i => i.ClrType == typeof(TEnum)) is UserTypeMapping userTypeMapping)
                 HackyEnumTypeMappings.Add(new(typeof(TEnum), userTypeMapping.PgTypeName, nameTranslator ?? DefaultNameTranslator));
 
-            ResetTypeMappingCache();
-
+            _typeMappingOptions = null;
             return this;
         }
-        finally
-        {
-            _lock.ExitWriteLock();
-        }
     }
 
     /// <inheritdoc />
     public bool UnmapEnum<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicFields)] TEnum>(string? pgName = null, INpgsqlNameTranslator? nameTranslator = null) where TEnum : struct, Enum
     {
-        _lock.EnterWriteLock();
-        try
+        lock (_sync)
         {
             var removed = _userTypeMapper.UnmapEnum<TEnum>(pgName, nameTranslator);
 
             // Temporary hack for EFCore.PG enum mapping compat
             if (removed && ((List<UserTypeMapping>)_userTypeMapper.Items).FindIndex(m => m.ClrType == typeof(TEnum)) is > -1 and var index)
                 HackyEnumTypeMappings.RemoveAt(index);
 
-            ResetTypeMappingCache();
-
+            _typeMappingOptions = null;
             return removed;
         }
-        finally
-        {
-            _lock.ExitWriteLock();
-        }
     }
 
     /// <inheritdoc />
     [RequiresDynamicCode("Calling MapEnum with a Type can require creating new generic types or methods. This may not work when AOT compiling.")]
     public INpgsqlTypeMapper MapEnum([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicFields | DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]
         Type clrType, string? pgName = null, INpgsqlNameTranslator? nameTranslator = null)
     {
-        _lock.EnterWriteLock();
-        try
+        lock (_sync)
         {
             _userTypeMapper.MapEnum(clrType, pgName, nameTranslator);
 
             // Temporary hack for EFCore.PG enum mapping compat
             if (_userTypeMapper.Items.FirstOrDefault(i => i.ClrType == clrType) is UserTypeMapping userTypeMapping)
                 HackyEnumTypeMappings.Add(new(clrType, userTypeMapping.PgTypeName, nameTranslator ?? DefaultNameTranslator));
 
-            ResetTypeMappingCache();
+            _typeMappingOptions = null;
             return this;
         }
-        finally
-        {
-            _lock.ExitWriteLock();
-        }
     }
 
     /// <inheritdoc />
     public bool UnmapEnum([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicFields | DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]
         Type clrType, string? pgName = null, INpgsqlNameTranslator? nameTranslator = null)
     {
-        _lock.EnterWriteLock();
-        try
+        lock (_sync)
         {
             var removed = _userTypeMapper.UnmapEnum(clrType, pgName, nameTranslator);
 
             // Temporary hack for EFCore.PG enum mapping compat
             if (removed && ((List<UserTypeMapping>)_userTypeMapper.Items).FindIndex(m => m.ClrType == clrType) is > -1 and var index)
                 HackyEnumTypeMappings.RemoveAt(index);
 
-            ResetTypeMappingCache();
+            _typeMappingOptions = null;
             return removed;
         }
-        finally
-        {
-            _lock.ExitWriteLock();
-        }
     }
 
     /// <inheritdoc />
@@ -342,34 +284,24 @@ public bool UnmapEnum([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes
     public INpgsqlTypeMapper MapComposite([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.PublicFields)]
         Type clrType, string? pgName = null, INpgsqlNameTranslator? nameTranslator = null)
     {
-        _lock.EnterWriteLock();
-        try
+        lock (_sync)
         {
             _userTypeMapper.MapComposite(clrType, pgName, nameTranslator);
-            ResetTypeMappingCache();
+            _typeMappingOptions = null;
             return this;
         }
-        finally
-        {
-            _lock.ExitWriteLock();
-        }
     }
 
     /// <inheritdoc />
     [RequiresDynamicCode("Mapping composite types involves serializing arbitrary types which can require creating new generic types or methods. This is currently unsupported with NativeAOT, vote on issue #5303 if this is important to you.")]
     public bool UnmapComposite([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.PublicFields)]
         Type clrType, string? pgName = null, INpgsqlNameTranslator? nameTranslator = null)
     {
-        _lock.EnterWriteLock();
-        try
+        lock (_sync)
         {
             var result = _userTypeMapper.UnmapComposite(clrType, pgName, nameTranslator);
-            ResetTypeMappingCache();
+            _typeMappingOptions = null;
             return result;
         }
-        finally
-        {
-            _lock.ExitWriteLock();
-        }
     }
 }
