[release/5.0-rc2] Prevent JSON async deserialization errors when partial buffer ends in the middle of null token (#42158)

[layomia]

Backport of #42158 ("Fix continuation of JSON deserialization") to release/5.0-rc2. Updated this PR's title to more accurately reflect the issue being fixed.

Customer Impact

• Fixes System.Text.Json.JsonException deserializing large Json stream with .NET 5.0 Preview 8 (but not 3.1) #42070.
• Common scenario
• Regression from 3.1.
• Reported by multiple users.
• Issue labeled as blocking-release/regression-from-last-release.

Prior to this PR, the serializer could throw a JSON exception when async deserializing JSON payloads that contain null tokens. If a partial segment of the payload being read ended in the middle of a null token, the serializer could throw a JsonException after fetching more data and resuming deserialization.

A simple example:

Initial buffer: {"Foo":"Bar","Baz":nu

Next buffer: ll,"Qux":"Quux"} // Serializer throws error when resuming deserializing after fetching this data.

---

async and null handling are both commonly used features in the serializer. This issue would likely be faced by numerous users if not fixed.

Testing

The PR has been tested extensively across multiple factors: null token position, matching property type (nullable/non-nullable), deserialization constructor mode (parameterized/non-parameterized), payload size (small/large), and so on.

The fixing PR has also been verified on customer input which flagged down the issue.

Risk

This fix is small and targeted. It is unlikely to cause issues elsewhere.

[danmoseley]

Great info in template. However if this is a bad regression why did we only know about it now at the last minute? Or did it regress recently?

[layomia]

I think more people are trying out System.Text.Json in .NET 5 due to RC1 release, especially as JSON improvements were highlighted in the blog post. The plan is to bolster the tests for more continuation scenarios - #42158 (comment) and continue testing/validating other features.

[danmoseley]

Thanks. It would be good to include that extended testing in this PR, if it’s for related issues. The goal is to minimize the possibility we have to bring another. We can take to tactics tomorrow even if that part is not ready, but that way if something else is found we can potentially roll it in before we merge it.

[steveharter]

LGTM; definitely agree that we need this in 5.0.

[layomia]

Okay to wait until the extended testing is in - #42393.

[danmoseley]

@ericstj I’m supportive, feel free to send mail to tactics if you support it

[danmoseley]

Don't let it stop this PR, but I think these test strings of > 200KB probably should be in the test data repo as they just bloat the repo. Once you've merged them though, that bloat is locked in so it's more of a future suggestion.

[ericstj]

approved over mail. Merge at will.

You can address the string suggestion in master. @danmosemsft we're usually more sensitive about the type of data and how much it changes. Text data is typically OK at somewhat larger sizes if it doesn't change too often, since it compresses really well in git. In fact git uses zlib compression which is based on DEFLATE same as zip used in nupkg. Binary data, especially binary data that changes often, is what we make sure to put in assets packages. Also any large data that would be considered "optional" for some repo consumers. Maybe we could write some guidelines down somewhere :)

[danmoseley]

@ericstj that makes sense. sounds good

[layomia]

You can address the string suggestion in master.

Maybe we could write some guidelines down somewhere :)

Opened #42428 to track these.