Fixed concurrency with mapping method cache (#848)

* Fixed concurrency with mapping method cache

* remove unnecessary lock

Author: RichardIrons-neo4j

Neo4j.Driver/Neo4j.Driver.Tests/HomeDbCaching/HomeDbCacheTests.cs

@@ -144,7 +144,7 @@ public async Task ShouldBeThreadSafe()
                                 cache.TryGetCached(key, out _);
                                 break;
 
-                            case 2: // Remove and re-add
+                            case 2: // both
                                 cache.AddOrUpdate(key, value);
                                 cache.TryGetCached(key, out _);
                                 break;

Neo4j.Driver/Neo4j.Driver.Tests/Mapping/RecordMappingTests.cs

@@ -13,7 +13,10 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+using System;
+using System.Collections.Concurrent;
 using System.Collections.Generic;
+using System.Threading;
 using System.Threading.Tasks;
 using FluentAssertions;
 using Neo4j.Driver.Internal.Types;
@@ -512,6 +515,72 @@ public void AsObject_ShouldMapRecordToObjectType()
         result.Should().BeEquivalentTo(expectedPerson);
     }
 
+    [Fact]
+    public async Task MapMethods_ShouldBeThreadSafe()
+    {
+        var typesToTest = new[]
+        {
+            typeof(TestPerson),
+            typeof(SimpleTestPerson),
+            typeof(PersonInDict),
+            typeof(Movie),
+            typeof(Person),
+            typeof(ProducingCareer),
+            typeof(CarAndPainting),
+            typeof(Painting),
+            typeof(Car),
+            typeof(PersonWithoutBornSetter),
+            typeof(TestPersonWithoutBornMapped),
+            typeof(Book),
+            typeof(Author),
+            typeof(Song),
+            typeof(ClassWithInitProperties),
+            typeof(ClassWithDefaultConstructor),
+            typeof(ClassWithDefaultConstructorWithAttributes),
+            typeof(TestXY)
+        };
+
+        const int numberOfThreads = 4;
+        var tasks = new List<Task>(numberOfThreads);
+        var resetEvent = new ManualResetEventSlim(false);
+        var exceptions = new ConcurrentBag<Exception>();
+
+        for (var i = 0; i < numberOfThreads; i++)
+        {
+            tasks.Add(
+                Task.Run(
+                    () =>
+                    {
+                        try
+                        {
+                            resetEvent.Wait(); // Wait for the signal to start
+                            for (var j = 0; j < 100; j++)
+                            {
+                                foreach (var type in typesToTest)
+                                {
+                                    RecordObjectMapping.Instance.GetMapMethodForType(type);
+                                }
+
+                                RecordObjectMapping.Reset();
+                            }
+                        }
+                        catch (Exception ex)
+                        {
+                            exceptions.Add(ex);
+                        }
+                    }));
+        }
+
+        resetEvent.Set(); // Signal all tasks to start
+        await Task.WhenAll(tasks);
+
+        // Fail the test if any exceptions were caught
+        if (exceptions.Count > 0)
+        {
+            throw new AggregateException("Thread safety issues detected.", exceptions);
+        }
+    }
+
     private class TestPerson
     {
         [MappingDefaultValue("A. Test Name")]
@@ -629,7 +698,7 @@ private class PersonWithoutBornSetter
     private class TestPersonWithoutBornMapped
     {
         [MappingSource("name")]
-        public string Name { get; set;  } = "A. Test Name";
+        public string Name { get; set; } = "A. Test Name";
 
         [MappingIgnored]
         public int? Born { get; set; } = 9999;

Neo4j.Driver/Neo4j.Driver/Public/Mapping/RecordObjectMapping.cs

@@ -14,6 +14,7 @@
 // limitations under the License.
 
 using System;
+using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.Reflection;
 using Neo4j.Driver.Internal.Mapping;
@@ -51,8 +52,8 @@ internal interface IRecordObjectMapping : IMappingRegistry
 /// <summary>Controls global record mapping configuration.</summary>
 public class RecordObjectMapping : IRecordObjectMapping
 {
-    private readonly Dictionary<Type, MethodInfo> _mapMethods = new();
-    private readonly Dictionary<Type, object> _mappers = new();
+    private readonly ConcurrentDictionary<Type, MethodInfo> _mapMethods = new();
+    private readonly ConcurrentDictionary<Type, object> _mappers = new();
     private readonly IMappingTypeConversionManager _typeConversionManager = new MappingTypeConversionManager();
     private IConventionTranslator _conventionTranslator = new NoOpConventionTranslator();
 
@@ -263,15 +264,14 @@ public static void RegisterProvider(IMappingProvider provider)
 
     public MethodInfo GetMapMethodForType(Type type)
     {
-        if (_mapMethods.TryGetValue(type, out var method))
+        return _mapMethods.AddOrUpdate(type, GetMapMethod, (_, m) => m);
+
+        MethodInfo GetMapMethod(Type t)
         {
-            return method;
+            var typedInterface = typeof(IRecordMapper<>).MakeGenericType(t);
+            var methodInfo = typedInterface.GetMethod(nameof(IRecordMapper<object>.Map));
+            return methodInfo;
         }
-
-        var typedInterface = typeof(IRecordMapper<>).MakeGenericType(type);
-        var mapMethod = typedInterface.GetMethod(nameof(IRecordMapper<object>.Map));
-        _mapMethods[type] = mapMethod;
-        return mapMethod;
     }
 
     /// <summary>Maps a record to an object of the given type according to the global mapping configuration.</summary>