diff --git a/src/Umbraco.Core/Mapping/UmbracoMapper.cs b/src/Umbraco.Core/Mapping/UmbracoMapper.cs index 0831edab4e..2d495b38b5 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. @@ -101,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 @@ -326,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) { @@ -347,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 3435050cc5..79d383857a 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,68 @@ 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() + { + // keep failing at mapping - and looping through the maps + 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(); + } + } + private class Thing1 { public string Value { get; set; } @@ -121,6 +185,9 @@ namespace Umbraco.Tests.Mapping public string Value { get; set; } } + private class Thing4 + { } + private class MapperDefinition1 : IMapDefinition { public void DefineMaps(UmbracoMapper mapper) @@ -144,5 +211,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(); + } + } } }