Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restrict indentation size in JsonWriterOptions theories. #101498

Merged
merged 1 commit into from
Apr 25, 2024

Conversation

eiriktsarpalis
Copy link
Member

Fix #101470.

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.

@eiriktsarpalis eiriktsarpalis added this to the 9.0.0 milestone Apr 24, 2024
@@ -7637,7 +7637,7 @@ private static IEnumerable<JsonWriterOptions> JsonOptions()
return from indented in new[] { true, false }
from skipValidation in new[] { true, false }
from indentCharacter in indented ? new char?[] { null, ' ', '\t' } : []
from indentSize in indented ? new int?[] { null, 0, 1, 2, 127 } : []
from indentSize in indented ? new int?[] { null, 0, 1, 2, 3 } : []
Copy link
Member

Choose a reason for hiding this comment

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

Why would the indent size result in OOMs? Significantly larger buffer sizes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. The impacted test creates very deep objects so a large indent size would contribute to substantially bigger buffer sizes.

@carlossanlop
Copy link
Member

We will need this backported to release/9.0-preview4 to clean up the large amount of related failures.

@mmitche
Copy link
Member

mmitche commented Apr 24, 2024

@carlossanlop Can you backport this when ready?

@carlossanlop
Copy link
Member

I was able to link all CI failures to a KnownBuildError except for one, which would be impossible to match to an ErrorMessage and would end up grouping anything. I described it here: #101524 (comment)

For that reason, I will make an exception and will JIT elevate myself to bypass the merge on green restriction.

@carlossanlop carlossanlop merged commit 5bbd0e1 into dotnet:main Apr 25, 2024
81 of 87 checks passed
@carlossanlop
Copy link
Member

/backport to release/9.0-preview4

Copy link
Contributor

Started backporting to release/9.0-preview4: https://github.com/dotnet/runtime/actions/runs/8826312659

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

System.Text.Json.Tests.Utf8JsonWriterTests.WritingTooDeep failure
5 participants