Test failure: System.Text.Json.Serialization.Tests.JsonElementTests.DeepEquals_TooDeepJsonDocument_ThrowsInsufficientExecutionStackException

[v-wenyuxu]

Failed in: runtime-coreclr crossgen2 20240724.1

Failed tests:

net9.0-linux-Release-x64-TestReadyToRun_Release-Ubuntu.2204.Amd64.Open
    - System.Text.Json.Serialization.Tests.JsonElementTests.DeepEquals_TooDeepJsonDocument_ThrowsInsufficientExecutionStackException
net9.0-linux-Release-x64-TestReadyToRun_Release-(Centos.8.Amd64.Open)[email protected]/dotnet-buildtools/prereqs:centos-stream8-helix
    - System.Text.Json.Serialization.Tests.JsonElementTests.DeepEquals_TooDeepJsonDocument_ThrowsInsufficientExecutionStackException
net9.0-linux-Release-x64-TestReadyToRun_Release-(Debian.11.Amd64.Open)[email protected]/dotnet-buildtools/prereqs:debian-11-helix-amd64
    - System.Text.Json.Serialization.Tests.JsonElementTests.DeepEquals_TooDeepJsonDocument_ThrowsInsufficientExecutionStackException

Error message:

 Assert.Throws() Failure: No exception was thrown
Expected: typeof(System.InsufficientExecutionStackException)

Stack trace:

   at System.Text.Json.Serialization.Tests.JsonElementTests.DeepEquals_TooDeepJsonDocument_ThrowsInsufficientExecutionStackException() in /_/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/JsonElementTests.cs:line 237

   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

[lambdageek]

@stephentoub interesting. the test you updated doesn't throw when crossgen is enabled. I'm not sure if it's expected that crossgen'd code uses less stack. Is there a way to make that test case deeper?

[stephentoub]

We can change this 10_000:

runtime/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/JsonElementTests.cs

Line 240 in bee05c8

using JsonDocument jDoc = CreateDeepJsonDocument(10_000);

to something arbitrarily large. The value should be limited only by available memory. The goal here is effectively to stack overflow, so, yeah 😄

[ivdiazsa]

Just for clarification, is this a test failure rather than a product failure? And is it being worked on already? @lambdageek @stephentoub

[lambdageek]

@ivdiazsa yea my suspicion is that the test needs to create more recursion. the way the test works is it creates some data structure and then starts a new thread with a small amount of stack space and then does some recursive calls exploring the data structure. The expectation is that it will throw an InsufficientExecutionStackException. With the JIT the current test case is deep enough. but with ReadyToRun, apparently not.

I don't think anyone is working on it.

[stephentoub]

I'll submit a PR to bump up the value.

[EgorBo]

then does some recursive calls exploring the data structure.

Just a side note that such assumptions may not be valid when JIT applies tail-call optimizations. e.g.:

void foo()
{
    foo();
}

does this code throw SO? the answer is: depends on optimization level

[stephentoub]

then does some recursive calls exploring the data structure.

Just a side note that such assumptions may not be valid when JIT applies tail-call optimizations. e.g.:

void foo()
{
foo();
}
does this code throw SO? the answer is: depends on optimization level

I wouldn't expect tail call optimizations to kick in here, though:

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonElement.cs

Lines 1353 to 1358 in 1337553

	if (!DeepEquals(prop1.Value, prop2.Value))
	{
	return false;
	}
	count--;

Is my mental model off?

[EgorBo]

I wouldn't expect tail call optimizations to kick in here, though:

Agree