Skip to content

Commit c277a32

Browse files
Copilotstephentoub
andauthored
Fix thread safety issue in JsonObject.InitializeDictionary causing GetPath failures (#120619)
* Initial plan * Fix thread safety issue in JsonObject.InitializeDictionary using Interlocked.CompareExchange Co-authored-by: stephentoub <[email protected]> * Remove unnecessary Interlocked.MemoryBarrier since CompareExchange is already a full barrier Co-authored-by: stephentoub <[email protected]> * Update src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonObject.IDictionary.cs --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: stephentoub <[email protected]> Co-authored-by: Stephen Toub <[email protected]>
1 parent 3149da8 commit c277a32

File tree

2 files changed

+37
-6
lines changed

2 files changed

+37
-6
lines changed

src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonObject.IDictionary.cs

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -250,7 +250,7 @@ public bool Remove(string propertyName)
250250

251251
if (dictionary is null)
252252
{
253-
dictionary = CreateDictionary(Options);
253+
OrderedDictionary<string, JsonNode?> newDictionary = CreateDictionary(Options);
254254

255255
if (jsonElement.HasValue)
256256
{
@@ -259,14 +259,24 @@ public bool Remove(string propertyName)
259259
JsonNode? node = JsonNodeConverter.Create(jElementProperty.Value, Options);
260260
node?.Parent = this;
261261

262-
dictionary.Add(jElementProperty.Name, node);
262+
newDictionary.Add(jElementProperty.Name, node);
263263
}
264264
}
265265

266-
// Ensure _jsonElement is written to after _dictionary
267-
_dictionary = dictionary;
268-
Interlocked.MemoryBarrier();
269-
_jsonElement = null;
266+
// Ensure only one dictionary instance is published using CompareExchange
267+
OrderedDictionary<string, JsonNode?>? exchangedDictionary = Interlocked.CompareExchange(ref _dictionary, newDictionary, null);
268+
if (exchangedDictionary is null)
269+
{
270+
// We won the race and published our dictionary
271+
// Ensure _jsonElement is written to after _dictionary
272+
_jsonElement = null;
273+
dictionary = newDictionary;
274+
}
275+
else
276+
{
277+
// Another thread won the race, use their dictionary
278+
dictionary = exchangedDictionary;
279+
}
270280
}
271281

272282
return dictionary;

src/libraries/System.Text.Json/tests/System.Text.Json.Tests/JsonNode/JsonObjectTests.cs

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1789,5 +1789,26 @@ public static void Deserialize_WrongType(string json)
17891789
{
17901790
Assert.Throws<JsonException>(() => JsonSerializer.Deserialize<JsonObject>(json));
17911791
}
1792+
1793+
[Fact]
1794+
public static void GetPath_IsThreadSafe()
1795+
{
1796+
for (int attempt = 0; attempt < 20; attempt++)
1797+
{
1798+
var tree = (JsonNode.Parse(
1799+
"""
1800+
{
1801+
"oh": "noes"
1802+
}
1803+
"""
1804+
) as JsonObject)!;
1805+
1806+
Parallel.ForEach(Enumerable.Range(0, 100), new ParallelOptions { MaxDegreeOfParallelism = 16 }, _ =>
1807+
{
1808+
string path = tree.First().Value!.GetPath();
1809+
Assert.Equal("$.oh", path);
1810+
});
1811+
}
1812+
}
17921813
}
17931814
}

0 commit comments

Comments
 (0)