Stack memory issue in locale tests (overflows on Mac)

dangerman commented 2 months ago

Bogus NuGet Package

35.5.0

.NET Version

.NET 8

Visual Studio Version

VS Code

What operating system are you using?

MacOS

What locale are you using with Bogus?

en

Problem Description

On MacOS I get a stack overflow in the LocaleSchemaTests.ensure_wellknown_locale_schema() test for the en locale.

After a bit of investigation it seems to be related to the custom InterceptedContractResolver's ResolveContract() method, as I think we're (inadvertently, due to an Argon implementation detail) reassigning the InterceptSerializeItem callback such that it's essentially 'repeating' itself and ends up running O(n^2) times relative to the size of the JSON.

I think it's happening with the en locale because it's the largest. And I think it's only on Mac because there's probably less stack memory available (512KB for secondary threads apparently).

LINQPad Example or Reproduction Steps

Steps to reproduce stack overflow on Mac

Check out repo
build using dotnet nuke --root . (I think this is the equivalent of the build.cmd script)
run tests using dotnet nuke test --root . or just use dotnet test or VS Code's test explorer

Gist to demonstrate memory issue

We can show the issue using a copy of the Resolver, with a trivial JSON and the help of some (admittedly crude) Console.WriteLine debugging. https://gist.github.com/dangerman/b3d674f4628dc9e5225f0618e1459215

In this example, we have a JSON full of fruit, and the corresponding one is printed whenever InterceptSerializeItem is run.

Expected	Actual

Expected Behavior

Tests run and pass

Actual Behavior

Stack overflow

...

Cause

We're using a custom InterceptedContractResolver to simplify arrays in our Verify Snapshots (so that arrays get summarised like Numbers [string, 20]).

In the ResolveContract() implementation we're:

saving the original jdc.InterceptSerializeItem as defaultIntercept
assigning jdc.InterceptSerializeItem = (key, val)...
- and in this callback, we either summarise the list, or we call the defaultIntercept() as necessary

https://github.com/bchavez/Bogus/blob/b72a4f9de627363b18b45fbdc7ff185501d45468/Source/Bogus.Tests/SchemaTests/LocaleSchemaTests.cs#L77-L95

HOWEVER, the jdc we get from this.defaultResolver.ResolveContract(type) happens to rely on an implementation that returns a cached contract if possible.

Verify's default contract resolver has logic for its own scrubbing and ignoring settings, but it's still relying on the base implementation from Argon: https://github.com/SimonCropp/Argon/blob/main/src/Argon/Serialization/DefaultContractResolver.cs#L55-L56\ ☝🏽 And I think this is the cache.

Anyway, if we're reassigning the jdc.InterceptSerializeItem on the same jdc object, then the defaultIntercept() in the callback is now what we previously assigned it to.

This means that when the InterceptSerializeItem is run, if it calls defaultIntercept(), then that defaultIntercept(), will call its defaultIntercept(), which calls its defaultIntercept()... ...until it runs the original InterceptSerializeItem implementation we want from Verify.

In the case of our en locale JSON, we have 1000s of items - a defaultIntercept() will be referencing 1000s of defaultIntercept() calls, eating up stack memory I guess.

Known Workarounds

We only need to assign InterceptSerializeItem once, assuming it's not replaced with a different implementation or we get a different Contract.

We can add a class variable to save the callback, so that next time we can see if it's already been updated (by us). If it hasn't we can do the assignment.

private InterceptSerializeDictionaryItem? updatedIntercept;

...

       if (contract is JsonDictionaryContract jdc && jdc.InterceptSerializeItem != updatedIntercept)
        {
            var defaultIntercept = jdc.InterceptSerializeItem;
            jdc.InterceptSerializeItem = (key, val) =>
            {
...
            };
            this.updatedIntercept = jdc.InterceptSerializeItem;

Could you help with a pull-request?

Yes

bchavez commented 2 months ago

Hey @dangerman, thank you for the bug report. This is pretty interesting. I'll take a look at this now and try to reproduce on MacOS.

We're using a custom InterceptedContractResolver to simplify arrays in our Verify Snapshots (so that arrays get summarised like Numbers [string, 20]).

Yep! Your understanding on needing this custom InterceptedContractResolver to simplify array summary checks is spot on. The reason I need these snapshot tests is that when doing locale updates, eg: faker.js v5 -> v6, I need some JSON schema/geometry tests to let me know if the JSON structure changed.

bchavez commented 2 months ago

Confirmed the StackOverflow happens on MacOS.

Also, another workaround to get past the StackOverflow is setting the following ENV variable on MacOS:

https://github.com/steveharter/dotnet_coreclr/blob/master/Documentation/project-docs/clr-configuration-knobs.md
```
export COMPlus_DefaultStackSize=512000
```
and the StackOverflow, for me, won't be encountered Mac and all unit tests pass.

Additionally, we can reproduce on Windows with:

set COMPlus_DefaultStackSize=128000
C:\Code\Projects\Public\Bogus\Source\Bogus.Tests> dotnet test

Sure enough, if I break and step into the test problem area; I see defaultIntercept calling over and over again for the same key/val and increasing in stack size when processing the seemingly same key/val:

00342_devenv

@dangerman , you can move forward with your fix/PR and I'll be happy to merge.

bchavez / Bogus