fix(ShadowNodeRegistry): thread safety issue in ShadowNodeRegistry (microsoft#1337)

rozele · web-flow · commit ba8b2f8fefc3 · 2017-08-31T08:42:10.000-04:00
Various reports of an InvalidOperationException occurring sporadically on app start have been filed (microsoft#1326, microsoft#266). It seems that this is related to the enumeration of root nodes tags in a dictionary from a KeyCollection while the dictionary is being updated. This PR copies the keys to a list (we reuse the list to reduce heap impact) to get around the exception (at least until a better architecture for this shadow node registry is proposed). Fixes microsoft#1326 Fixes microsoft#266
diff --git a/ReactWindows/ReactNative.Shared.Tests/UIManager/ShadowNodeRegistryTests.cs b/ReactWindows/ReactNative.Shared.Tests/UIManager/ShadowNodeRegistryTests.cs
@@ -3,6 +3,8 @@
 using System;
 using System.Collections.Generic;
 using System.Linq;
+using System.Threading;
+using System.Threading.Tasks;
 
 namespace ReactNative.Tests.UIManager
 {
@@ -82,5 +84,36 @@ public void ShadowRegistryNode_AddNode()
                 AssertEx.Throws<KeyNotFoundException>(() => registry.GetNode(i));
             }
         }
+
+        [Test]
+        public async Task ShadowRegistryNode_AddRootNode_WhileEnumerating()
+        {
+            var n1 = new ReactShadowNode { ReactTag = 1 };
+            var n2 = new ReactShadowNode { ReactTag = 2 };
+
+            var enter = new AutoResetEvent(false);
+            var exit = new AutoResetEvent(false);
+
+            var registry = new ShadowNodeRegistry();
+
+            var task = Task.Run(() =>
+            {
+                enter.WaitOne();
+                registry.AddRootNode(n2);
+                exit.Set();
+            });
+
+            registry.AddRootNode(n1);
+
+            foreach (var tag in registry.RootNodeTags)
+            {
+                enter.Set();
+                exit.WaitOne();
+            }
+
+            await task;
+
+            Assert.AreEqual(2, registry.RootNodeTags.Count);
+        }
     }
 }
diff --git a/ReactWindows/ReactNative.Shared/UIManager/ShadowNodeRegistry.cs b/ReactWindows/ReactNative.Shared/UIManager/ShadowNodeRegistry.cs
@@ -10,20 +10,40 @@ namespace ReactNative.UIManager
     /// </summary>
     public class ShadowNodeRegistry
     {
+        private readonly object _gate = new object();
+
         private readonly IDictionary<int, ReactShadowNode> _tagsToCssNodes =
             new Dictionary<int, ReactShadowNode>();
 
         private readonly IDictionary<int, bool> _rootTags =
             new Dictionary<int, bool>();
 
+        // The RootNodeTags API is called from UI thread instead of the
+        // layout thread. Occasionally, we would get an exception related to
+        // the enumeration of the Keys collection being disrupted by an
+        // AddRootNode operation. This was especially likely in Debug mode, but
+        // also could occur during a reload of the application from CodePush.
+        // To get around this, we copy the key collection into this list.
+        private List<int> _rootNodeTags = new List<int>();
+
         /// <summary>
         /// The collection of root node tags.
         /// </summary>
-        public ICollection<int> RootNodeTags
+        public IReadOnlyList<int> RootNodeTags
         {
             get
             {
-                return _rootTags.Keys;
+                _rootNodeTags.Clear();
+
+                lock (_gate)
+                {
+                    foreach (var tag in _rootTags.Keys)
+                    {
+                        _rootNodeTags.Add(tag);
+                    }
+                }
+
+                return _rootNodeTags;
             }
         }  
 
@@ -38,7 +58,11 @@ public void AddRootNode(ReactShadowNode node)
 
             var tag = node.ReactTag;
             _tagsToCssNodes[tag] = node;
-            _rootTags[tag] = true;
+
+            lock (_gate)
+            {
+                _rootTags[tag] = true;
+            }
         }
 
         /// <summary>
@@ -54,7 +78,11 @@ public void RemoveRootNode(int tag)
             }
 
             _tagsToCssNodes.Remove(tag);
-            _rootTags.Remove(tag);
+
+            lock (_gate)
+            {
+                _rootTags.Remove(tag);
+            }
         }
 
         /// <summary>
