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

Developers should be able to pass state to custom converters. #63795

Open
4 tasks
Tracked by #77018
eiriktsarpalis opened this issue Jan 14, 2022 · 18 comments
Open
4 tasks
Tracked by #77018

Developers should be able to pass state to custom converters. #63795

eiriktsarpalis opened this issue Jan 14, 2022 · 18 comments
Labels
area-System.Text.Json Cost:L Work that requires one engineer up to 4 weeks Priority:2 Work that is important, but not critical for the release Team:Libraries User Story A single user-facing feature. Can be grouped under an epic.
Milestone

Comments

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Jan 14, 2022

Background and Motivation

The current model for authoring custom converters in System.Text.Json is general-purpose and powerful enough to address most serialization customization requirements. Where it falls short currently is in the ability to accept user-provided state scoped to the current serialization operation; this effectively blocks a few relatively common scenaria:

  1. Custom converters requiring dependency injection scoped to the current serialization. A lack of a reliable state passing mechanism can prompt users to rebuild the converter cache every time a serialization operation is performed.

  2. Custom converters do not currently support streaming serialization. Built-in converters avail of the internal "resumable converter" abstraction, a pattern which allows partial serialization and deserialization by marshalling the serialization state/stack into a state object that gets passed along converters. It lets converters suspend and resume serialization as soon as the need to flush the buffer or read more data arises. This pattern is implemented using the internal JsonConveter<T>.TryWrite and JsonConverter<T>.TryRead methods.

    Since resumable converters are an internal implementation detail, custom converters cannot support resumable serialization. This can create performance problems in both serialization and deserialization:

    • In async serialization, System.Text.Json will delay flushing the buffer until the custom converter (and any child converters) have completed writing.
    • In async deserialization, System.Text.Json will need to read ahead all JSON data at the current depth to ensure that the custom converter has access to all required data at the first read attempt.

    We should consider exposing a variant of that abstraction to advanced custom converter authors.

  3. Custom converters are not capable of passing internal serialization state, often resulting in functional bugs when custom converters are encountered in an object graph (cf. ReferenceHandler.IgnoreCycles doesn't work with Custom Converters #51715, Incorrect JsonException.Path when using non-leaf custom JsonConverters #67403, Keep LineNumber, BytePositionInLine and Path when calling JsonSerializer.Deserialize<TValue>(ref reader) #77345)

Proposal

Here is a rough sketch of how this API could look like:

namespace System.Text.Json.Serialization;

public struct JsonWriteState
{
    public CancellationToken { get; }
    public Dictionary<string, object> UserState { get; }
}

public struct JsonReadState
{
    public CancellationToken { get; }
    public Dictionary<string, object> UserState { get; }
}

public abstract class JsonStatefulConverter<T> : JsonConverter<T>
{
     public abstract void Write(Utf8JsonWriter writer, T value, JsonSerializerOptions options, ref JsonWriteState state);
     public abstract T? Read(ref Utf8JsonReader writer, Type typeToConvert, JsonSerializerOptions options, ref JsonReadState state);

      // Override the base methods: implemented in terms of the new virtuals and marked as sealed
      public sealed override void Write(Utf8JsonWriter writer, T value, JsonSerializerOptions options) {}
      public sealed override T? Read(ref Utf8JsonReader writer, Type typeToConvert, JsonSerializerOptions options) {}
}

public abstract class JsonResumableConverter<T> : JsonConverter<T>
{
     public abstract bool TryWrite(Utf8JsonWriter writer, T value, JsonSerializerOptions options, ref JsonWriteState state);
     public abstract bool TryRead(ref Utf8JsonReader writer, Type typeToConvert, JsonSerializerOptions options, ref JsonReadState state, out T? result);

      // Override the base methods: implemented in terms of the new virtuals and marked as sealed
      public sealed override void Write(Utf8JsonWriter writer, T value, JsonSerializerOptions options) {}
      public sealed override T? Read(ref Utf8JsonReader writer, Type typeToConvert, JsonSerializerOptions options) {}
}

public partial class JsonSerializer
{
     // Overloads to existing methods accepting state
     public static string Serialize<T>(T value, JsonSerializerOptions options, ref JsonWriteState state);
     public static string Serialize<T>(T value, JsonTypeInfo<T> typeInfo, ref JsonWriteState state);
     public static T? Deserialize<T>(string json, JsonSerializerOptions options, ref JsonReadState state);
     public static T? Deserialize<T>(string json, JsonTypeInfo<T> typeInfo, ref JsonReadState state);

     // New method groups enabling low-level streaming serialization
     public static bool TrySerialize(T value, JsonTypeInfo<T> typeInfo, ref JsonWriteState state);
     public static bool TryDeserialize(string json, JsonTypeInfo<T> typeInfo, ref JsonReadState state);
}

Users should be able to author custom converters that can take full advantage of async serialization, and compose correctly with the contextual serialization state. This is particularly important in the case of library authors, who might want to extend async serialization support for custom sets of types. It could also be used to author top-level async serialization methods that target other data sources (e.g. using System.IO.Pipelines cf. #29902)

Usage Examples

MyPoco value = new() { Value = "value" };
JsonWriteState state = new() { UserState = { ["sessionId"] = "myId" }};
JsonSerializer.Serialize(value, options, state); // { "sessionId" : "myId", "value" : "value" }

public class MyConverter : JsonStatefulConverter<MyPoco>
{
    public override void Write(Utf8JsonWriter writer, MyPoco value, JsonSerializerOptions options, ref JsonWriteState state)
    {
         writer.WriteStartObject();
         writer.WriteString("sessionId", (string)state.UserState["sessionId"]);
         writer.WriteString("value", value.Value);
     }
}

Alternative designs

We might want to consider the viability attaching the state values as properties on Utf8JsonWriter and Utf8JsonReader. It would avoid the need of introducing certain overloads, but on the flip side it could break scenaria where the writer/reader objects are being passed to nested serialization operations.

Goals

Progress

  • Author prototype
  • API proposal & review
  • Implementation & tests
  • Conceptual documentation & blog posts.
@eiriktsarpalis eiriktsarpalis added area-System.Text.Json User Story A single user-facing feature. Can be grouped under an epic. Priority:2 Work that is important, but not critical for the release Cost:M Work that requires one engineer up to 2 weeks Team:Libraries labels Jan 14, 2022
@eiriktsarpalis eiriktsarpalis added this to the 7.0.0 milestone Jan 14, 2022
@ghost
Copy link

ghost commented Jan 14, 2022

Tagging subscribers to this area: @dotnet/area-system-text-json
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and Motivation

Async serialization in System.Text.Json is a powerful feature that lets users serialize and deserialize from streaming JSON data, without the need to load the entire payload in-memory. At the converter level, this is achieved with "resumable converters", a pattern that allows for partial serialization and deserialization by marshalling the serialization state/stack into a mutable struct that gets passed along converters. It lets converters suspend and resume serialization as soon as the need to flush the buffer or read more data arises. This pattern is implemented using the internal JsonConveter<T>.TryWrite and JsonConverter<T>.TryRead methods.

Since resumable converters are an internal implementation detail, custom converters cannot support resumable serialization. This can create performance problems in both serialization and deserialization:

  • In async serialization, System.Text.Json will delay flushing the buffer until the custom converter (and any child converters) have completed writing.
  • In async deserialization, System.Text.Json will need to read ahead all JSON data at the current depth to ensure that the custom converter has access to all required data at the first read attempt.

Because custom converters cannot pass the serialization state, System.Text.Json suffers from a class of functional bugs that arise because of the serialization state resetting every time a custom converter is encountered in the object graph (cf. #51715).

Proposal

Users should be able to author custom converters that can take full advantage of async serialization, and compose correctly with the contextual serialization state. This is particularly important in the case of library authors, who might want to extend async serialization support for custom sets of types. It could also be used to author top-level async serialization methods that target other data sources (e.g. using System.IO.Pipelines cf. #29902)

This is a proposal to publicize aspects of the TryWrite/TryRead pattern to the users of System.Text.Json. It should be noted that this is a feature aimed exclusively towards "advanced" users, primarily third-party library and framework authors.

Goals

Progress

  • Author prototype
  • API proposal & review
  • Implementation & tests
  • Conceptual documentation & blog posts.
Author: eiriktsarpalis
Assignees: -
Labels:

area-System.Text.Json, User Story, Priority:2, Cost:M, Team:Libraries

Milestone: 7.0.0

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jan 14, 2022
@eiriktsarpalis
Copy link
Member Author

Tagging @steveharter who might be interested in this.

@jeffhandley
Copy link
Member

I'm updating this feature's milestone to Future as it is not likely to make it into .NET 7.

@eiriktsarpalis eiriktsarpalis changed the title Developers should be able to write custom async converters Developers should be able to author converters that accept serialization state. Oct 14, 2022
@eiriktsarpalis eiriktsarpalis changed the title Developers should be able to author converters that accept serialization state. Developers should be able to pass user state to custom converters. Oct 14, 2022
@eiriktsarpalis eiriktsarpalis changed the title Developers should be able to pass user state to custom converters. Developers should be able to pass state to custom converters. Oct 14, 2022
@eiriktsarpalis
Copy link
Member Author

I've updated the issue description and examples to broaden the scope to stateful converters. Sketch of API proposal and a basic example has been included. PTAL.

@layomia
Copy link
Contributor

layomia commented Oct 14, 2022

There are scenarios where a custom converter might want metadata info about the type (including it's properties) or property it is processing, e.g. #35240 (comment). I understand that with JsonSerializerOptions.GetTypeInfo(Type), it is now possible to retrieve type metadata within converters, but should there be a first class mechanism, e.g. adding relevant type/property metadata to the state object passed to converters?

I believe I've also come across scenarios where a converter might want to know where in the object graph it is being invoked, i.e the root object vs property values. Is that also state that should be passed?

@eiriktsarpalis
Copy link
Member Author

Yes that's plausible. Effectively we should investigate what parts of ReadStack/WriteStack could/should be exposed to the user. I'm not sure if we could meaningfully expose what amounts to WriteStack.Current, since that only gets updated consistently when the internal converters are being called. Prototyping is certainly required to validate feasibility.

@thomaslevesque
Copy link
Member

To be honest, with the proposed design, this feature would only be moderately useful, because it requires passing state explicitly to the Serialize/Deserialize methods. This doesn't help when you don't have control over these callsites, as is the case in ASP.NET Core JSON input/output formatters.

I realize what I'm talking about isn't really "state", at least not callsite-specific state, but it's what was requested in #71718, which has been closed in favor this issue... I don't think the proposed design addresses the requirements of #71718.

@eiriktsarpalis
Copy link
Member Author

I realize what I'm talking about isn't really "state", at least not callsite-specific state, but it's what was requested in #71718, which has been closed in favor this issue... I don't think the proposed design addresses the requirements of #71718.

Yes, this proposal specifically concerns state scoped to the operation rather than the options instance. The latter is achievable if you really need it whereas the former is outright impossible.

@stevejgordon
Copy link
Contributor

@eiriktsarpalis I must admit that I'd not read this through and assumed it solved the suggestion from my original proposal. I agree that this solves a different problem (which I've not personally run into). It would not help at all with the scenario I have when building a library. Is there any reason not to re-open #71718 to complement this? I think both are valid scenarios that should be possible to achieve with less ceremony.

@eiriktsarpalis
Copy link
Member Author

Agree, I've reopened the issue based on your feedback.

@eiriktsarpalis eiriktsarpalis added Cost:L Work that requires one engineer up to 4 weeks and removed Cost:M Work that requires one engineer up to 2 weeks labels Jan 23, 2023
@eiriktsarpalis eiriktsarpalis modified the milestones: 8.0.0, Future Jan 23, 2023
@osexpert
Copy link

osexpert commented May 9, 2023

"We might want to consider the viability attaching the state values as properties on Utf8JsonWriter and Utf8JsonReader"
I see when Deserialize from Stream, an Utf8JsonReader instance is made for every iteration of a while-loop, so does not seem like a good fit.

@dennis-yemelyanov
Copy link
Contributor

Was just looking for something like this. I'd like to be able to extract some value from the object during serialization and make it available after the serialization is done. It seems like currently the only way to achieve this is to be instantiating a new converter for each serialization, which is not great for perf.

Is this still planned to be implemented at some point?

@brantburnett
Copy link
Contributor

Personally, I'm more interested in the performance benefits of the resuming bits of this proposal. While I realize they're somewhat related, I wonder if the lack of progress here could be partially related to the scope of including both resuming and user state in the same proposal. Should it be separated so the more valuable one (whichever that is) could be done independently, so long as the design has a path forward to the other?

@eiriktsarpalis
Copy link
Member Author

Should it be separated so the more valuable one (whichever that is) could be done independently, so long as the design has a path forward to the other?

I think both are equally valuable. At the same time, we should be designing the serialization state types in a way that satisfy the requirements for both use cases.

@andrewjsaid
Copy link
Contributor

What is the purpose of the JsonReadState and JsonWriteState parameters being passed in as ref?
The dictionary is already mutable so it's probably not that...

@eiriktsarpalis
Copy link
Member Author

The types already exist as internal implementation detail and hold additional state which wouldn't be visible to users.

@JustDre
Copy link

JustDre commented Nov 11, 2024

Given that #64182 was closed in deference to this issue, I don't know why the solution needs to involve async converters when DeserializeAsyncEnumerable does not and yet provides significant memory savings over DeserializeAsync (whenever streaming deserialization is appropriate). It seems the ability to call DeserializeAsyncEnumerable at any arbitrary point of an incoming stream, and leaving the stream open at the appropriate byte location upon exit, would be immensely useful. While the solutions discussed above are optimal from a resource perspective, forcing all developers to fully support async converters seems like a bigger lift than necessary, simply to support non-root array deserialization. If an overload can be implemented to achieve this, then once async converters become available, developers can opt in to implement them as appropriate.

@JustDre
Copy link

JustDre commented Nov 11, 2024

Maybe it's not important, but issue #77018 (that tracks this one) is currently closed. I don't see it being tracked by an equivalent issue for .NET 9, but since it's days (hours?) away from release, it probably needs to be tracked by the equivalent for .NET 10.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Text.Json Cost:L Work that requires one engineer up to 4 weeks Priority:2 Work that is important, but not critical for the release Team:Libraries User Story A single user-facing feature. Can be grouped under an epic.
Projects
None yet
Development

No branches or pull requests

10 participants