Skip to content

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Oct 11, 2025

  • Analyze the thread safety issue in JsonObject.GetPath
  • Identify root cause: InitializeDictionary can create multiple dictionary instances in race conditions
  • Identify solution: Use Interlocked.CompareExchange to ensure single dictionary instance
  • Add reproduction test for the issue
  • Fix InitializeDictionary to use Interlocked.CompareExchange
  • Build and test the changes
  • Verify the fix resolves the issue - all 49,652 tests passing
  • Remove unnecessary Interlocked.MemoryBarrier (CompareExchange is already a full barrier)

Summary

Fixed thread safety issue in JsonObject.InitializeDictionary() where concurrent access could create multiple dictionary instances with different JsonNode objects, causing FindValue() to fail with InvalidOperationException when looking up child nodes by reference equality.

The fix uses Interlocked.CompareExchange to atomically ensure only one dictionary instance is published to the _dictionary field. 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 to GetPath() on child nodes.

Original prompt

This section details on the original issue you should resolve

<issue_title>Thread safety issue in STJ.JsonObject</issue_title>
<issue_description>### Description

Calling GetPath on a JsonObject child node in parallel triggers InvalidOperationException.

Reproduction Steps

var tree = (JsonNode.Parse(
    """
    {
      "oh": "noes"
    }
    """
) as JsonObject)!;

// Workaround, do this once before parallel
// tree.First().Value!.GetPath();

Parallel.ForEach(Enumerable.Range(0, 10), _ =>
{
    tree.First().Value!.GetPath();
});

Expected behavior

I expect parallel reads from the JsonNode/Object/Values to be thread safe.

Actual behavior

On JsonObject.GetPath, line string propertyName = FindValue(child)!.Value.Key; I get the following exception + stack trace:

System.InvalidOperationException: Nullable object must have a value.
   at System.Nullable`1.get_Value()
   at System.Text.Json.Nodes.JsonObject.GetPath(ValueStringBuilder& path, JsonNode child)
   at System.Text.Json.Nodes.JsonNode.GetPath()
   ...

Regression?

Possibly regression from #77421

Known Workarounds

Traverse the object hierarchy serially once before parallel access to force lazy-initialization.

Configuration

Using .net sdk 9.0.305.

I don't think the rest matters, but macOS 15.7.1, arm64.

Other information

I guess calling tree.First() in the repro triggers initialization of the underlying Dictionary and I can't see anything in place to avoid a race condition there.</issue_description>

<agent_instructions>Please validate or dispute my hypothesis in the issue, then come up with a fix. If my hypothesis is correct, it would involve using an interlocked to ensure that a single dictionary insurance is published. </agent_instructions>

Comments on the Issue (you are @copilot in this section)

@stephentoub Just based on code inspection, I suspect the issue is that #InitializeDictionary when called concurrently could end up returning two different dictionary instances with different node objects. While idempotent in terms of contents, reference equality will be different, and FindValue is relying on reference equality to locate the child object.

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.

Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

…rlocked.CompareExchange

Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
@Copilot 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
@Copilot Copilot AI requested a review from stephentoub October 11, 2025 15:35
Copilot finished work on behalf of stephentoub October 11, 2025 15:35
@stephentoub stephentoub marked this pull request as ready for review October 11, 2025 15:44
@Copilot Copilot AI review requested due to automatic review settings October 11, 2025 15:44
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.CompareExchange to 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>
@Copilot Copilot AI requested a review from stephentoub October 11, 2025 16:12
Copilot finished work on behalf of stephentoub October 11, 2025 16:12
@stephentoub stephentoub merged commit c277a32 into main Oct 13, 2025
84 of 86 checks passed
@stephentoub stephentoub deleted the copilot/fix-thread-safety-issue branch October 13, 2025 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Thread safety issue in STJ.JsonObject

4 participants