Ensure delayed initialization of JsonArray/JsonObject is thread-safe (#…

…77567) * Ensure delayed initialization of JsonArray/JsonObject is thread-safe * Address feedback * Add null check before invoking delayed initializers
dotnet · Oct 28, 2022 · 7fbf1c1 · 7fbf1c1
1 parent 7479cbe
commit 7fbf1c1
Show file tree

Hide file tree

Showing 4 changed files with 91 additions and 34 deletions.
diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonArray.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonArray.cs
@@ -5,6 +5,7 @@
 using System.Diagnostics;
 using System.Diagnostics.CodeAnalysis;
 using System.Text.Json.Serialization.Converters;
+using System.Threading;
 
 namespace System.Text.Json.Nodes
 {
@@ -85,7 +86,7 @@ private void InitializeFromArray(JsonNode?[] items)
             throw new InvalidOperationException(SR.Format(SR.NodeElementWrongType, nameof(JsonValueKind.Array)));
         }
 
-        internal JsonArray (JsonElement element, JsonNodeOptions? options = null) : base(options)
+        internal JsonArray(JsonElement element, JsonNodeOptions? options = null) : base(options)
         {
             Debug.Assert(element.ValueKind == JsonValueKind.Array);
             _jsonElement = element;
@@ -180,33 +181,47 @@ public override void WriteTo(Utf8JsonWriter writer, JsonSerializerOptions? optio
 
         private void CreateNodes()
         {
-            if (_list == null)
+            if (_list is null)
             {
-                List<JsonNode?> list;
+                CreateNodesCore();
+            }
 
-                if (_jsonElement == null)
-                {
-                    list = new List<JsonNode?>();
-                }
-                else
-                {
-                    JsonElement jElement = _jsonElement.Value;
-                    Debug.Assert(jElement.ValueKind == JsonValueKind.Array);
+            void CreateNodesCore()
+            {
+                // Even though _list initialization can be subject to races,
+                // ensure that contending threads use a coherent view of jsonElement.
+
+                // Because JsonElement cannot be read atomically there might be torn reads,
+                // however the order of read/write operations guarantees that that's only
+                // possible if the value of _list is non-null.
+                JsonElement? jsonElement = _jsonElement;
+                Interlocked.MemoryBarrier();
+                List<JsonNode?>? list = _list;
 
-                    list = new List<JsonNode?>(jElement.GetArrayLength());
+                if (list is null)
+                {
+                    list = new();
 
-                    foreach (JsonElement element in jElement.EnumerateArray())
+                    if (jsonElement.HasValue)
                     {
-                        JsonNode? node = JsonNodeConverter.Create(element, Options);
-                        node?.AssignParent(this);
-                        list.Add(node);
+                        JsonElement jElement = jsonElement.Value;
+                        Debug.Assert(jElement.ValueKind == JsonValueKind.Array);
+
+                        list = new List<JsonNode?>(jElement.GetArrayLength());
+
+                        foreach (JsonElement element in jElement.EnumerateArray())
+                        {
+                            JsonNode? node = JsonNodeConverter.Create(element, Options);
+                            node?.AssignParent(this);
+                            list.Add(node);
+                        }
                     }
 
-                    // Clear since no longer needed.
+                    // Ensure _jsonElement is written to after _list
+                    _list = list;
+                    Interlocked.MemoryBarrier();
                     _jsonElement = null;
                 }
-
-                _list = list;
             }
         }
 

diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonObject.IDictionary.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonObject.IDictionary.cs
@@ -5,6 +5,7 @@
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.Text.Json.Serialization.Converters;
+using System.Threading;
 
 namespace System.Text.Json.Nodes
 {
@@ -261,32 +262,47 @@ IEnumerator IEnumerable.GetEnumerator()
 
         private void InitializeIfRequired()
         {
-            if (_dictionary != null)
+            if (_dictionary is null)
             {
-                return;
+                InitializeCore();
             }
 
-            bool caseInsensitive = Options.HasValue ? Options.Value.PropertyNameCaseInsensitive : false;
-            var dictionary = new JsonPropertyDictionary<JsonNode?>(caseInsensitive);
-            if (_jsonElement.HasValue)
+            void InitializeCore()
             {
-                JsonElement jElement = _jsonElement.Value;
+                // Even though _dictionary initialization can be subject to races,
+                // ensure that contending threads use a coherent view of jsonElement.
+
+                // Because JsonElement cannot be read atomically there might be torn reads,
+                // however the order of read/write operations guarantees that that's only
+                // possible if the value of _dictionary is non-null.
+                JsonElement? jsonElement = _jsonElement;
+                Interlocked.MemoryBarrier();
+                JsonPropertyDictionary<JsonNode?>? dictionary = _dictionary;
 
-                foreach (JsonProperty jElementProperty in jElement.EnumerateObject())
+                if (dictionary is null)
                 {
-                    JsonNode? node = JsonNodeConverter.Create(jElementProperty.Value, Options);
-                    if (node != null)
+                    bool caseInsensitive = Options.HasValue ? Options.Value.PropertyNameCaseInsensitive : false;
+                    dictionary = new JsonPropertyDictionary<JsonNode?>(caseInsensitive);
+                    if (jsonElement.HasValue)
                     {
-                        node.Parent = this;
+                        foreach (JsonProperty jElementProperty in jsonElement.Value.EnumerateObject())
+                        {
+                            JsonNode? node = JsonNodeConverter.Create(jElementProperty.Value, Options);
+                            if (node != null)
+                            {
+                                node.Parent = this;
+                            }
+
+                            dictionary.Add(new KeyValuePair<string, JsonNode?>(jElementProperty.Name, node));
+                        }
                     }
 
-                    dictionary.Add(new KeyValuePair<string, JsonNode?>(jElementProperty.Name, node));
+                    // Ensure _jsonElement is written to after _dictionary
+                    _dictionary = dictionary;
+                    Interlocked.MemoryBarrier();
+                    _jsonElement = null;
                 }
-
-                _jsonElement = null;
             }
-
-            _dictionary = dictionary;
         }
     }
 }
diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/JsonNode/JsonArrayTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/JsonNode/JsonArrayTests.cs
@@ -5,6 +5,7 @@
 using System.Collections.Generic;
 using System.IO;
 using System.Linq;
+using System.Threading.Tasks;
 using Xunit;
 
 namespace System.Text.Json.Nodes.Tests
@@ -473,5 +474,17 @@ public static void GetJsonArrayIEnumerable()
             Assert.True(jArrayEnumerator.MoveNext());
             Assert.Equal("value", ((JsonValue)jArrayEnumerator.Current).GetValue<string>());
         }
+
+        [Fact]
+        public static void LazyInitializationIsThreadSafe()
+        {
+            string arrayText = "[\"elem0\",\"elem1\"]";
+            JsonArray node = Assert.IsType<JsonArray>(JsonNode.Parse(arrayText));
+            Parallel.For(0, 128, i =>
+            {
+                Assert.Equal("elem0", (string)node[0]);
+                Assert.Equal("elem1", (string)node[1]);
+            });
+        }
     }
 }
diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/JsonNode/JsonObjectTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/JsonNode/JsonObjectTests.cs
@@ -6,6 +6,7 @@
 using System.IO;
 using System.Linq;
 using System.Text.Json.Serialization.Tests;
+using System.Threading.Tasks;
 using Xunit;
 
 namespace System.Text.Json.Nodes.Tests
@@ -956,5 +957,17 @@ static void Test(JsonObject jObject)
                 Assert.Equal("World", (string)jObject["hello"]); // Test case insensitivity
             }
         }
+
+        [Fact]
+        public static void LazyInitializationIsThreadSafe()
+        {
+            string arrayText = "{\"prop0\":0,\"prop1\":1}";
+            JsonObject jObj = Assert.IsType<JsonObject>(JsonNode.Parse(arrayText));
+            Parallel.For(0, 128, i =>
+            {
+                Assert.Equal(0, (int)jObj["prop0"]);
+                Assert.Equal(1, (int)jObj["prop1"]);
+            });
+        }
     }
 }