Fix thread safety issue in JsonObject.InitializeDictionary causing GetPath failures#120619
Merged
stephentoub merged 4 commits intomainfrom Oct 13, 2025
Merged
Fix thread safety issue in JsonObject.InitializeDictionary causing GetPath failures#120619stephentoub merged 4 commits intomainfrom
stephentoub merged 4 commits intomainfrom
Conversation
Contributor
|
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis |
…rlocked.CompareExchange Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix thread safety issue in STJ.JsonObject
Fix thread safety issue in JsonObject.InitializeDictionary causing GetPath failures
Oct 11, 2025
stephentoub
reviewed
Oct 11, 2025
src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonObject.IDictionary.cs
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a thread safety issue in JsonObject.InitializeDictionary() that was causing GetPath() calls to fail with InvalidOperationException when multiple threads accessed the same JsonObject concurrently.
Key changes:
- Replaced direct assignment with atomic
Interlocked.CompareExchangeto ensure only one dictionary instance is published - Added comprehensive thread safety test with 20 iterations of 100 parallel tasks
- Preserved existing memory barrier logic for proper ordering guarantees
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| JsonObject.IDictionary.cs | Fixed race condition in dictionary initialization using atomic compare-exchange pattern |
| JsonObjectTests.cs | Added thread safety test to validate concurrent GetPath() calls don't fail |
… already a full barrier Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
stephentoub
reviewed
Oct 11, 2025
src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonObject.IDictionary.cs
Show resolved
Hide resolved
…Object.IDictionary.cs
stephentoub
approved these changes
Oct 11, 2025
src/libraries/System.Text.Json/src/System/Text/Json/Nodes/JsonObject.IDictionary.cs
Show resolved
Hide resolved
eiriktsarpalis
approved these changes
Oct 13, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixed thread safety issue in
JsonObject.InitializeDictionary()where concurrent access could create multiple dictionary instances with different JsonNode objects, causingFindValue()to fail withInvalidOperationExceptionwhen looking up child nodes by reference equality.The fix uses
Interlocked.CompareExchangeto atomically ensure only one dictionary instance is published to the_dictionaryfield. Threads that lose the race use the already-published dictionary instead of their local copy, ensuring all threads see consistent JsonNode references.Test coverage includes new
GetPath_IsThreadSafe()test that validates concurrent calls toGetPath()on child nodes.Original prompt
Fixes #120617
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.