Skip to content

Conversation

steveharter
Copy link
Contributor

@steveharter steveharter commented Aug 18, 2021

Fixes #39055

The test failure reports in that issue reference PathForChildDictionaryFails however only for .NET framework 461. However, the refactored PathForChildDictionaryFails test now repros on net6 as well. I believe the 461 ArrayPool was returning just the right size pooled buffer from a previous test that caused the continuation issue, and net6 returned a larger pool (likely on a boundary).

This also occurs in 5.0.

@ghost
Copy link

ghost commented Aug 18, 2021

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #39055

The test failure reports in that issue reference PathForChildDictionaryFails however only for .NET framework 461. However, the refactored PathForChildDictionaryFails test now repros on net6 as well. I believe the 461 ArrayPool was returning just the right size pooled buffer from a previous test that caused the continuation issue, and net6 returned a larger pool (likely on a boundary).

This appears to be a regression from #54420 where the "continuation count" is now 1 less than before.

Author: steveharter
Assignees: steveharter
Labels:

area-System.Text.Json

Milestone: 6.0.0

Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

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

Thanks. Out of curiosity why did this only repro in netfx? Should we backport this to release/6.0?

@steveharter
Copy link
Contributor Author

steveharter commented Aug 19, 2021

Thanks. Out of curiosity why did this only repro in netfx? Should we backport this to release/6.0?

Actually this did repro in 5.0. I don't think it makes the bar for back-porting. Also, I don't think it meets the bar for 6.0 porting to main either (no customer reports + not a regression + used for debugging bad JSON and not runtime functionality).

Here's a standalone repro:

for (int i = 1; i < 30; i++)
{
    string json = @"{""Child"":{""MyDictionary"":{""Key"": bad]";
    JsonSerializerOptions options = new() { DefaultBufferSize = i };
    MemoryStream ms = new MemoryStream(System.Text.Encoding.ASCII.GetBytes(json));
    try
    {
        await JsonSerializer.DeserializeAsync<RootClass>(ms, options);
    }
    catch (JsonException ex)
    {
        Console.WriteLine($"{i}:" + ex.Path); // should be "$.Child.MyDictionary.Key"
    }
}

public class RootClass
{
    public ChildClass Child { get; set; }
}

public class ChildClass
{
    public int MyInt { get; set; }
    public int[] MyIntArray { get; set; }
    public Dictionary<string, ChildClass> MyDictionary { get; set; }
    public ChildClass[] Children { get; set; }
}

@steveharter steveharter merged commit b5cd281 into dotnet:main Aug 19, 2021
@steveharter steveharter deleted the JsonPathIssue branch August 19, 2021 17:08
@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Aug 24, 2021

Should we backport this to release/6.0?

Depending on how often this happens in CI, it might actually meet the bar citing build stability issues.

Nevermind, it seems like #57834 should be a sufficient intervention.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 23, 2021
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.

Test failure: System.Text.Json.Serialization.Tests.ConstructorTests_Stream.PathForChildDictionaryFails

2 participants