Merge pull request #5266 from umbraco/v8/bugfix/5187-mapper-issue

Fix mapper concurrency issue

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
     /// </remarks>
     public class UmbracoMapper
     {
-        private readonly Dictionary<Type, Dictionary<Type, Func<object, MapperContext, object>>> _ctors
-            = new Dictionary<Type, Dictionary<Type, Func<object, MapperContext, object>>>();
+        // 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<Type, Dictionary<Type, Action<object, object, MapperContext>>> _maps
-            = new Dictionary<Type, Dictionary<Type, Action<object, object, MapperContext>>>();
+        private readonly ConcurrentDictionary<Type, Dictionary<Type, Func<object, MapperContext, object>>> _ctors
+            = new ConcurrentDictionary<Type, Dictionary<Type, Func<object, MapperContext, object>>>();
+
+        private readonly ConcurrentDictionary<Type, Dictionary<Type, Action<object, object, MapperContext>>> _maps
+            = new ConcurrentDictionary<Type, Dictionary<Type, Action<object, object, MapperContext>>>();
 
         /// <summary>
         /// Initializes a new instance of the <see cref="UmbracoMapper"/> class.
@@ -101,16 +107,12 @@ namespace Umbraco.Core.Mapping
 
         private Dictionary<Type, Func<object, MapperContext, object>> DefineCtors(Type sourceType)
         {
-            if (!_ctors.TryGetValue(sourceType, out var sourceCtor))
-                sourceCtor = _ctors[sourceType] = new Dictionary<Type, Func<object, MapperContext, object>>();
-            return sourceCtor;
+            return _ctors.GetOrAdd(sourceType, _ => new Dictionary<Type, Func<object, MapperContext, object>>());
         }
 
         private Dictionary<Type, Action<object, object, MapperContext>> DefineMaps(Type sourceType)
         {
-            if (!_maps.TryGetValue(sourceType, out var sourceMap))
-                sourceMap = _maps[sourceType] = new Dictionary<Type, Action<object, object, MapperContext>>();
-            return sourceMap;
+            return _maps.GetOrAdd(sourceType, _ => new Dictionary<Type, Action<object, object, MapperContext>>());
         }
 
         #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)
             {

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<IEnumerable<ContentPropertyDto>>(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<Thing2>(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<Thing2>(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<int, object>();
+                mapper.Define<string, object>();
+                mapper.Define<double, object>();
+                mapper.Define<UmbracoMapper, object>();
+                mapper.Define<Property, object>();
+            }
+        }
     }
 }