JsonNode is not thread safe #77421
Comments
|
I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label. |
|
Area is |
|
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis Issue DetailsDescriptionI have an issue reported where there is a failure in a multithreaded environment. (I have taken many precautions in my code to ensure that it is threadsafe.) In isolating the issue, I found that Reproduction Steps[Test]
public void Issue337_ParallelComparisons()
{
string? failed = null;
var arrayText = "[\"DENIED\",\"GRANTED\"]";
var valueText = "\"GRANTED\"";
var array = JsonNode.Parse(arrayText);
var value = JsonNode.Parse(valueText);
try
{
Parallel.ForEach(Enumerable.Range(1, 1000000).ToList().AsParallel(), i =>
{
var array0 = array[0];
var array1 = array[1];
var array0Hash = JsonNodeEqualityComparer.Instance.GetHashCode(array0);
var array1Hash = JsonNodeEqualityComparer.Instance.GetHashCode(array1);
var valueHash = JsonNodeEqualityComparer.Instance.GetHashCode(value);
if (array0Hash != valueHash && array1Hash != valueHash)
{
failed ??= $@"Hashcode failed on iteration {i}
value: {valueHash} - {value.AsJsonString()}
array[0]: {array0Hash} - {array0.AsJsonString()}
array[1]: {array1Hash} - {array1.AsJsonString()}";
return;
}
//if (!JsonNodeEqualityComparer.Instance.Equals(array0, value) &&
// !JsonNodeEqualityComparer.Instance.Equals(array1, value))
//{
// failed ??= "Equals failed";
//}
//if (!array.Contains(value, JsonNodeEqualityComparer.Instance))
//{
// failed ??= "Contains failed";
//}
});
}
finally
{
if (failed != null)
{
Console.WriteLine(failed);
Assert.Fail();
}
}
}The test was distilled from an issue attempting to identify a single JSON value within an array (see linked issue above). Expected behaviorThe test should succeed, proving consistency in hash code generation and int comparisons. Actual behavior.Net Core 3.1The test completes but fails. Usually, I find that the failure occurs in the 20K-30Kth iteration. It seems that the hash codes are generated properly, but for some reason the
I also noticed behavior where the hashcode checks succeed but the equality or contains checks fail. However, I haven't thoroughly checked to make sure my code ( .Net 5, 6, & 7(rc2)The test fails with Stack traceRegression?I can't tell if this ever worked in any runtime, but given that it's not working as expected in .Net Core 3.1, I'd say it's always been an issue. I don't know what the issue may be in later runtimes. The exception is at least indicative that something went wrong, although it's not clear what that was. Known WorkaroundsNo response ConfigurationThis is all run on Windows 11, x64. I doubt it's related to the configuration, though. Other informationNo response
|
|
It would help if you could share the full implementation of the equality comparer, although my guess would be that you have hit #76440. TL;DR Could you try your repro with your latest nightly build of the nuget package to verify? |
eiriktsarpalis
added
the
needs-author-action
|
This issue has been marked |
|
@eiriktsarpalis, I don't believe it's the same issue, or at least not solely. The stack trace shows the error stemming from JsonArray.CreateNodes; JsonArray has a If we actually intend for the whole JsonDocument/Element/Node/etc. type set to be thread-safe, someone needs to audit all of the code... it appears to be doing a lot of lazy initialization without much mind paid to thread-safety. It doesn't actually look like it was developed with the intent of being thread-safe. |
Agreed that this is not the case. |
|
Yes, it does call that, but only if the values are strings (which is true for this test). I can try the test with numbers to see if the problem persists. (EDIT The test passes for numbers instead of strings.) The code that it's calling is here and here The important bits are toward the bottom of each method where it's comparing string values. The rest (i.e. the code for objects and arrays) isn't invoked because the test is only ever getting the hash code for nodes containing strings. I don't know if it's a quirk of the debugger, but I'm also very concerned about the screenshot I posted of the immediate window that shows a comparison between two obviously equal integers resulting as not equal. As you can see from the test, I'm storing the calculated hash codes into vars, then comparing the vars. The hash code calculation shouldn't factor into the test, but I don't know how this is lowered by the compiler, so I can't say for sure. |
ghost
added
needs-further-triage
Could you try running the original test using the nightly .NET 8 nuget package? |
|
I can reproduce this in a nightly build. A reduced example: using System.Text.Json.Nodes;
using System.Runtime.InteropServices;
Console.WriteLine(RuntimeInformation.FrameworkDescription);
var arrayText = "[\"DENIED\",\"GRANTED\"]";
var valueText = "\"GRANTED\"";
var array = JsonNode.Parse(arrayText)!.AsArray();
_ = JsonNode.Parse(valueText);
Parallel.ForEach(Enumerable.Range(0, int.MaxValue), i =>{
_ = array![0];
_ = array![1];
});The output of Stack Trace |
|
It appears that the issue boils down to The same appears to be true for the lazy initialization logic in JsonObject: We should try to make both methods thread safe. |
eiriktsarpalis
added
bug
and removed
needs-further-triage
ghost
added
the
in-pr
ghost
removed
the
in-pr
ghost
locked as resolved and limited conversation to collaborators
@eiriktsarpalis did we do this and update the documentation? Or are we still not making an overall thread-safety garuntee? |
|
Yes, #77567 addressed all thread safety issues that I could find related to lazy initialization, so it should now be consistent with what is asserted in the docs (i.e. thread safety on read). |
Description
I have an issue reported where there is a failure in a multithreaded environment. (I have taken many precautions in my code to ensure that it is threadsafe.)
In isolating the issue, I found that
JsonNodeitself doesn't seem to be thread safe, and so my code can ultimately never be.Reproduction Steps
The test was distilled from an issue attempting to identify a single JSON value within an array (see linked issue above).
Expected behavior
The test should succeed, proving consistency in hash code generation and int comparisons.
Actual behavior
.Net Core 3.1
The test completes but fails. Usually, I find that the failure occurs in the 20K-30Kth iteration.
It seems that the hash codes are generated properly, but for some reason the
intcomparison fails. The VS debugger screenshot below clearly shows the values are the same, and yet they're being evaluated as unequal.I also noticed behavior where the hashcode checks succeed but the equality or contains checks fail. However, I haven't thoroughly checked to make sure my code (
JsonNodeEqualityComparer) isn't at fault here. This is why they're commented out..Net 5, 6, & 7(rc2)
The test fails with
InvalidOperationExceptionwhen attempting to accessarray[0]stating "Nullable object must have a value." This generally occurs on subsequent iterations, not the first one, and usually on multiple threads.Stack trace
Regression?
I can't tell if this ever worked in any runtime, but given that it's not working as expected in .Net Core 3.1, I'd say it's always been an issue. I don't know what the issue may be in later runtimes. The exception is at least indicative that something went wrong, although it's not clear what that was.
Known Workarounds
No response
Configuration
This is all run on Windows 11, x64. I doubt it's related to the configuration, though.
Other information
No response
The text was updated successfully, but these errors were encountered: