From c685a63747bb7be6a85c0bf819a5c094bf9045c9 Mon Sep 17 00:00:00 2001 From: Richard Irons Date: Thu, 1 May 2025 16:49:49 +0100 Subject: [PATCH 1/2] Fixed concurrency with mapping method cache --- .../HomeDbCaching/HomeDbCacheTests.cs | 2 +- .../Mapping/RecordMappingTests.cs | 74 ++++++++++++++++++- .../Public/Mapping/RecordObjectMapping.cs | 18 ++--- 3 files changed, 83 insertions(+), 11 deletions(-) diff --git a/Neo4j.Driver/Neo4j.Driver.Tests/HomeDbCaching/HomeDbCacheTests.cs b/Neo4j.Driver/Neo4j.Driver.Tests/HomeDbCaching/HomeDbCacheTests.cs index 8a7a5b4d0..d996b204b 100644 --- a/Neo4j.Driver/Neo4j.Driver.Tests/HomeDbCaching/HomeDbCacheTests.cs +++ b/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; diff --git a/Neo4j.Driver/Neo4j.Driver.Tests/Mapping/RecordMappingTests.cs b/Neo4j.Driver/Neo4j.Driver.Tests/Mapping/RecordMappingTests.cs index 8c1f087cf..5a77d535a 100644 --- a/Neo4j.Driver/Neo4j.Driver.Tests/Mapping/RecordMappingTests.cs +++ b/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,75 @@ 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(numberOfThreads); + var resetEvent = new ManualResetEventSlim(false); + var exceptions = new ConcurrentBag(); + + 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) + { + lock (exceptions) + { + 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 +701,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; diff --git a/Neo4j.Driver/Neo4j.Driver/Public/Mapping/RecordObjectMapping.cs b/Neo4j.Driver/Neo4j.Driver/Public/Mapping/RecordObjectMapping.cs index 8f5ca56a8..542c403b6 100644 --- a/Neo4j.Driver/Neo4j.Driver/Public/Mapping/RecordObjectMapping.cs +++ b/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 /// Controls global record mapping configuration. public class RecordObjectMapping : IRecordObjectMapping { - private readonly Dictionary _mapMethods = new(); - private readonly Dictionary _mappers = new(); + private readonly ConcurrentDictionary _mapMethods = new(); + private readonly ConcurrentDictionary _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.Map)); + return methodInfo; } - - var typedInterface = typeof(IRecordMapper<>).MakeGenericType(type); - var mapMethod = typedInterface.GetMethod(nameof(IRecordMapper.Map)); - _mapMethods[type] = mapMethod; - return mapMethod; } /// Maps a record to an object of the given type according to the global mapping configuration. From a9b39440707e72a85e9b33a1048eb7788289bdff Mon Sep 17 00:00:00 2001 From: Richard Irons Date: Thu, 1 May 2025 16:57:50 +0100 Subject: [PATCH 2/2] remove unnecessary lock --- .../Neo4j.Driver.Tests/Mapping/RecordMappingTests.cs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/Neo4j.Driver/Neo4j.Driver.Tests/Mapping/RecordMappingTests.cs b/Neo4j.Driver/Neo4j.Driver.Tests/Mapping/RecordMappingTests.cs index 5a77d535a..244b0c39a 100644 --- a/Neo4j.Driver/Neo4j.Driver.Tests/Mapping/RecordMappingTests.cs +++ b/Neo4j.Driver/Neo4j.Driver.Tests/Mapping/RecordMappingTests.cs @@ -566,10 +566,7 @@ public async Task MapMethods_ShouldBeThreadSafe() } catch (Exception ex) { - lock (exceptions) - { - exceptions.Add(ex); - } + exceptions.Add(ex); } })); }