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

[API Proposal]: JsonSerializer.TryReadValue(ref Utf8JsonReader) #29902

Closed
davidfowl opened this issue Jun 16, 2019 · 13 comments
Closed

[API Proposal]: JsonSerializer.TryReadValue(ref Utf8JsonReader) #29902

davidfowl opened this issue Jun 16, 2019 · 13 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json backlog-cleanup-candidate An inactive issue that has been marked for automated closure. needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Milestone

Comments

@davidfowl
Copy link
Member

Today if the JSON passed into the JsonSerializer needs to be the fully formed, this means its basically impossible to let the caller handle buffering (which is unfortunate). This seems solvable if we add a TryReadValue which would return false if the Serializer failed to Read the full object from the Utf8JsonReader.

public static class JsonSerializer
{
    public static bool TryReadValue(ref Utf8JsonReader reader, Type returnType, out object value, JsonSerializerOptions options = null);
    public static bool TryReadValue<TValue>(ref Utf8JsonReader reader, out TValue value, JsonSerializerOptions options = null);
}

This currently wouldn't preserve the serializer state, but it would let the caller buffer an entire JSON payload from a Stream of bytes without a surrounding envelope.

@davidfowl davidfowl changed the title Support JsonSerializer.TryRead(ref Utf8JsonReader) [API Proposal]: JsonSerializer.TryReadValue(ref Utf8JsonReader) Jun 16, 2019
@mqudsi
Copy link

mqudsi commented Jun 17, 2019

I agree, this would make things easier.

this means its basically impossible to let the caller handle buffering (which is unfortunate)

I thought so too, but then I realized there was sufficient low-level state available to work around the limitations.

I actually just finished (technically it was at 3am, but who's keeping track? 😂) implementing a non-blocking, asynchronous Stream (or anything else, I guess) deserializer on top of the presently exposed System.Text.Json API surface. That was actually the easy part, the difficulty was in creating an async non-blocking equivalent to (yet better than!) the old Newtonsoft JsonTextReader. It wasn't fun (especially because Stream is ornery and ugly (why oh why can I not read from a Stream to a byte *?) and I need .NET Standard support, and System.IO.Pipelines doesn't help here at all in its latest non-preview state), but it's working with more-or-less zero copy and zero large-object allocations regardless of payload length while still allowing for caller-controlled buffering (and being called from an async context, no less).

There are some issues I ran into (which I've been filing along the way), but it's certainly doable!

@layomia layomia self-assigned this Nov 5, 2019
@layomia layomia removed their assignment Dec 3, 2019
@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the 5.0 milestone Feb 1, 2020
@frankhaugen
Copy link

Would name this TryDeserialize()

And include a TrySerialize(), because suddenly you have something too complex or self recursive references that will cause a crash unless wrapped in try/catch. when creating a library you have no control over what is being serialized, it results in so many bug reports

@ghost
Copy link

ghost commented Oct 15, 2021

Due to lack of recent activity, this issue has been marked as a candidate for backlog cleanup. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will undo this process.

This process is part of the experimental issue cleanup initiative we are currently trialing in a limited number of areas. Please share any feedback you might have in the linked issue.

@davidfowl
Copy link
Member Author

😢

@ghost ghost removed the no-recent-activity label Oct 15, 2021
@udlose
Copy link

udlose commented Oct 16, 2021

Sad to see this being killed off

@frankhaugen
Copy link

What is this missing to get the attention of the API reviewers? I really want "Try" -methods on the serializer

@eiriktsarpalis
Copy link
Member

@davidfowl I realize that this is a fairly old issue, but don't the async deserialization methods address that particular use case today?

FWIW this API shape is precisely how async serialization is implemented internally, but you still need to somehow marshall the deserialization stack (this is done using the ReadState struct internally). We plan on possibly exposing this API in 7.0.0, but only on the converter level (cf. #36785)

@ghost
Copy link

ghost commented Nov 9, 2021

This issue has been automatically marked no recent activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no recent activity.

@davidfowl
Copy link
Member Author

I'd still love a PipeReader overload on DeserializeAsync. This was a workaround for that missing

@ghost ghost added needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration and removed needs more info no-recent-activity labels Nov 10, 2021
@eiriktsarpalis
Copy link
Member

I see. We probably wouldn't be able to take a Pipes dependency on STJ, so presumably you'd need the ability to roll your own method.

We should consider offering this as part of #60904. Closing in favor of that issue.

@steveharter
Copy link
Member

Note that PipeReader\Writer async were in the initial commit for the serializer back in 3.0. However, due to concerns it was pulled:

read
write
tests

Original issue: #28325
Older prototype: dotnet/corefxlab#2647 (review)

@davidfowl
Copy link
Member Author

Bring it back 🥲

@steveharter
Copy link
Member

Bring it back 🥲

Yeah we'd have to re-evaluate the dependency from STJ to System.IO.Pipelines. Some options:

  • Take the dependency
    • We'd have to account for lack of it being in NetStandard so in-box only? If part of the package, users will have additional assembly(s) that they may not use.
    • Trimming should work fine for Blazor client and stand-alone apps.
  • Add an extension assembly like STJ.PipeLines.
    • Since C# doesn't support static extension methods to add the new methods to JsonSerializer class, there would likely need to be a new class (e.g. JsonSerializerPipelines), a new extension model, or we add Serializer\Deserialize extension methods to PipeReader\Writer.
    • We'd also need a public way to call into the currently internal serializer methods that deal with internal classes like ReadState etc (we already have some other reasons to do this).

@ghost ghost locked as resolved and limited conversation to collaborators Dec 12, 2021
@eiriktsarpalis eiriktsarpalis added the backlog-cleanup-candidate An inactive issue that has been marked for automated closure. label Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Text.Json backlog-cleanup-candidate An inactive issue that has been marked for automated closure. needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Projects
None yet
Development

No branches or pull requests

8 participants