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

Fix handling of large streams #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Hawxy
Copy link

@Hawxy Hawxy commented Jul 28, 2022

Hello @arogozine, we use this library extensively and ran into the below exception when deserializing large payloads with some unusually shaped records:

System.Text.Json.JsonException: The JSON value could not be converted to System.ValueTuple`2[System.Guid,TupleJsonUnitTests.UnitTests+DataRecord]. Path: $[0] | LineNumber: 0 | BytePositionInLine: 81. ---> System.InvalidOperationException: Cannot skip tokens on partial JSON. Either get the whole payload and create a Utf8JsonReader instance where isFinalBlock is true or call TrySkip.
    at System.Text.Json.Utf8JsonReader.Skip()
   at System.Text.Json.Serialization.Converters.ObjectWithParameterizedConstructorConverter`1.OnTryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
   at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
   at System.Text.Json.Serialization.JsonResumableConverter`1.Read(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options)
   at TupleAsJsonArray.TupleConverterBase`1.ReadValue[T](Utf8JsonReader& reader, JsonSerializerOptions options)
   at TupleAsJsonArray.ValueTupleConverter`2.Read(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options)

The issue only appears with very large entity counts and when using a stream.

Changing the implementation to always delegate the deserialization/serialization to the serializer instead of pulling out a converter appears to fix this issue. I'm not sure if this has an unintended side-effects but the tests continue to pass.

@arogozine arogozine added the bug Something isn't working label Aug 8, 2022
@arogozine
Copy link
Owner

arogozine commented Aug 8, 2022

I'm seeing #if NET6_0_OR_GREATER

Is this something specific to .NET 6? Specific to record types?

I might release a new version targeting .NET 6 then to keep up with the LTS releases.

@Hawxy
Copy link
Author

Hawxy commented Aug 9, 2022

We use records for the majority of our entities thus the reproduction was created around them. It's possible it impacts normal classes but we haven't tested.

@arogozine
Copy link
Owner

arogozine commented Aug 10, 2022

Alright, this took me like an hour and half to debug you test example.

If you limit the loop to like 100 instead of 100000 - everything works.

Specifically I found that the error started to occur between 178 and 179.
178 entry array has a length of 16378 and 179 length array has 16470

The magic number is probably 16384
The options.DefaultBufferSize is 16384

So the culprit is the buffer size... adding,

options.DefaultBufferSize = 9000000;

fixed the example.

More importantly, a non-async deserialization,

            string result = Encoding.UTF8.GetString(memoryStream.ToArray());
            var test = JsonSerializer.Deserialize<List<(Guid, DataRecord)>>(result, options);

works without any issue,

The TupleAsJsonArray might not be at fault here.

Thoughts?

@osexpert
Copy link

osexpert commented Mar 7, 2023

Known issue it seems: dotnet/runtime#74108

@osexpert
Copy link

I think I found a much more worse problem with this logic:
This give NullReferenceException:

			JsonSerializerOptions options = new JsonSerializerOptions();
			JsonConverter<Version> jsonConverter = (JsonConverter<Version>)options.GetConverter(typeof(Version));
			Version v = null;
			jsonConverter.Write(new Utf8JsonWriter(new MemoryStream()), v, options);

This works:

			JsonSerializerOptions options = new JsonSerializerOptions();
			Version v = null;
			JsonSerializer.Serialize(new MemoryStream(), v, options);

The crazy thing is, all(?) the builtin converters does not seem to handle null directly and is not safe to invoke directly. But when used indirectly, via JsonSerializer, they work just fine. This is insane IMO and a very bad design, if my observations are correct.

@osexpert
Copy link

I am correct
dotnet/runtime#84037

So this is a bug for sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants