From 6cb55789bd6fe0f1a1300f660d9977cc39395efa Mon Sep 17 00:00:00 2001 From: Stephan Date: Mon, 15 Apr 2019 10:25:29 +0200 Subject: [PATCH 1/2] Fix mapper concurrency issue --- src/Umbraco.Core/Mapping/UmbracoMapper.cs | 14 ++-- src/Umbraco.Tests/Mapping/MappingTests.cs | 83 ++++++++++++++++++++++- 2 files changed, 92 insertions(+), 5 deletions(-) diff --git a/src/Umbraco.Core/Mapping/UmbracoMapper.cs b/src/Umbraco.Core/Mapping/UmbracoMapper.cs index 0831edab4e..a2ddebe431 100644 --- a/src/Umbraco.Core/Mapping/UmbracoMapper.cs +++ b/src/Umbraco.Core/Mapping/UmbracoMapper.cs @@ -1,5 +1,6 @@ using System; using System.Collections; +using System.Collections.Concurrent; using System.Collections.Generic; using System.Linq; @@ -29,11 +30,16 @@ namespace Umbraco.Core.Mapping /// public class UmbracoMapper { - private readonly Dictionary>> _ctors - = new Dictionary>>(); + // note + // + // the outer dictionary *can* be modified, see GetCtor and GetMap, hence have to be ConcurrentDictionary + // the inner dictionaries are never modified and therefore can be simple Dictionary - private readonly Dictionary>> _maps - = new Dictionary>>(); + private readonly ConcurrentDictionary>> _ctors + = new ConcurrentDictionary>>(); + + private readonly ConcurrentDictionary>> _maps + = new ConcurrentDictionary>>(); /// /// Initializes a new instance of the class. diff --git a/src/Umbraco.Tests/Mapping/MappingTests.cs b/src/Umbraco.Tests/Mapping/MappingTests.cs index 3435050cc5..47be99bb0e 100644 --- a/src/Umbraco.Tests/Mapping/MappingTests.cs +++ b/src/Umbraco.Tests/Mapping/MappingTests.cs @@ -1,5 +1,7 @@ -using System.Collections.Generic; +using System; +using System.Collections.Generic; using System.Linq; +using System.Threading; using NUnit.Framework; using Umbraco.Core.Mapping; using Umbraco.Core.Models; @@ -108,6 +110,69 @@ namespace Umbraco.Tests.Mapping var target = mapper.Map>(source); } + [Test] + [Explicit] + public void ConcurrentMap() + { + var definitions = new MapDefinitionCollection(new IMapDefinition[] + { + new MapperDefinition1(), + new MapperDefinition3(), + }); + var mapper = new UmbracoMapper(definitions); + + // the mapper currently has a map from Thing1 to Thing2 + // because Thing3 inherits from Thing1, it will map a Thing3 instance, + // and register a new map from Thing3 to Thing2, + // thus modifying its internal dictionaries + + // if timing is good, and mapper does have non-concurrent dictionaries, it fails + // practically, to reproduce, one needs to add a 1s sleep in the mapper's loop + // hence, this test is explicit + + var thing3 = new Thing3 { Value = "value" }; + var thing4 = new Thing4(); + Exception caught = null; + + void ThreadLoop() + { + for (var i = 0; i < 10; i++) + { + try + { + mapper.Map(thing4); + } + catch (Exception e) + { + caught = e; + Console.WriteLine($"{e.GetType().Name} {e.Message}"); + } + } + + Console.WriteLine("done"); + } + + var thread = new Thread(ThreadLoop); + thread.Start(); + Thread.Sleep(1000); + + try + { + Console.WriteLine($"{DateTime.Now:O} mapping"); + var thing2 = mapper.Map(thing3); + Console.WriteLine($"{DateTime.Now:O} mapped"); + + Assert.IsNotNull(thing2); + Assert.AreEqual("value", thing2.Value); + } + finally + { + thread.Join(); + } + + Assert.IsNull(caught); + } + private class Thing1 { public string Value { get; set; } @@ -121,6 +186,9 @@ namespace Umbraco.Tests.Mapping public string Value { get; set; } } + private class Thing4 + { } + private class MapperDefinition1 : IMapDefinition { public void DefineMaps(UmbracoMapper mapper) @@ -144,5 +212,18 @@ namespace Umbraco.Tests.Mapping private static void Map(Property source, ContentPropertyDto target, MapperContext context) { } } + + private class MapperDefinition3 : IMapDefinition + { + public void DefineMaps(UmbracoMapper mapper) + { + // just some random things so that the mapper contains things + mapper.Define(); + mapper.Define(); + mapper.Define(); + mapper.Define(); + mapper.Define(); + } + } } } From 791581f502e7a6a5cf36d710ee5ea28aa33e825f Mon Sep 17 00:00:00 2001 From: Stephan Date: Wed, 17 Apr 2019 14:52:38 +0200 Subject: [PATCH 2/2] Use ConcurrentDictionary specific methods --- src/Umbraco.Core/Mapping/UmbracoMapper.cs | 12 ++++++------ src/Umbraco.Tests/Mapping/MappingTests.cs | 3 +-- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/Umbraco.Core/Mapping/UmbracoMapper.cs b/src/Umbraco.Core/Mapping/UmbracoMapper.cs index a2ddebe431..2d495b38b5 100644 --- a/src/Umbraco.Core/Mapping/UmbracoMapper.cs +++ b/src/Umbraco.Core/Mapping/UmbracoMapper.cs @@ -107,16 +107,12 @@ namespace Umbraco.Core.Mapping private Dictionary> DefineCtors(Type sourceType) { - if (!_ctors.TryGetValue(sourceType, out var sourceCtor)) - sourceCtor = _ctors[sourceType] = new Dictionary>(); - return sourceCtor; + return _ctors.GetOrAdd(sourceType, _ => new Dictionary>()); } private Dictionary> DefineMaps(Type sourceType) { - if (!_maps.TryGetValue(sourceType, out var sourceMap)) - sourceMap = _maps[sourceType] = new Dictionary>(); - return sourceMap; + return _maps.GetOrAdd(sourceType, _ => new Dictionary>()); } #endregion @@ -332,6 +328,8 @@ namespace Umbraco.Core.Mapping if (_ctors.TryGetValue(sourceType, out var sourceCtor) && sourceCtor.TryGetValue(targetType, out var ctor)) return ctor; + // we *may* run this more than once but it does not matter + ctor = null; foreach (var (stype, sctors) in _ctors) { @@ -353,6 +351,8 @@ namespace Umbraco.Core.Mapping if (_maps.TryGetValue(sourceType, out var sourceMap) && sourceMap.TryGetValue(targetType, out var map)) return map; + // we *may* run this more than once but it does not matter + map = null; foreach (var (stype, smap) in _maps) { diff --git a/src/Umbraco.Tests/Mapping/MappingTests.cs b/src/Umbraco.Tests/Mapping/MappingTests.cs index 47be99bb0e..79d383857a 100644 --- a/src/Umbraco.Tests/Mapping/MappingTests.cs +++ b/src/Umbraco.Tests/Mapping/MappingTests.cs @@ -136,6 +136,7 @@ namespace Umbraco.Tests.Mapping void ThreadLoop() { + // keep failing at mapping - and looping through the maps for (var i = 0; i < 10; i++) { try @@ -169,8 +170,6 @@ namespace Umbraco.Tests.Mapping { thread.Join(); } - - Assert.IsNull(caught); } private class Thing1