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

In JsonConverter, .Skip() throws, but .TrySkip() never returns false during DeserializeAsync #39795

Closed
joshlang opened this issue Jul 22, 2020 · 2 comments

Comments

@joshlang
Copy link

Details here: https://stackoverflow.com/questions/63038334/how-do-i-handle-partial-json-in-a-jsonconverter-while-using-deserializeasync-on

Essentially, calling .Skip() in my custom JsonConverter<T> during a DeserializeAsync<T>(stream, ...) throws InvalidOperationException: Cannot skip tokens on partial JSON. Either get the whole payload and create a Utf8JsonReader instance where isFinalBlock is true or call TrySkip.

Calling TrySkip never returns false, though. So... why would Skip fail?

Anyway - this may be a case of me not knowing how to write a JsonConverter properly. However, apparently it doesn't occur in 3.1 and I was encouraged to open an issue. So here it is :D

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Text.Json untriaged New issue has not been triaged by the area owner labels Jul 22, 2020
@joshlang
Copy link
Author

Looking through source, I see "Skip" is not even attempted if a non-final block is passed in. I was expecting the logic to be basically if (!TrySkip()) { throw new Explosion(); }. Looks like I was wrong.

@layomia
Copy link
Contributor

layomia commented Jul 23, 2020

In the existing JsonConverter model, custom converters do not have to worry about handling partial data, as the serializer passes all the data for the current JSON scope. Skip/TrySkip logic is unnecessary. cc @tdykstra we should consider adding a section about this in the docs.

The "read ahead" logic that the serializer performs to make this possible does have some perf implications. There has been some discussion around a new model to, among other benefits, enable more performant async-handling when custom converters are used - #1562. Relevant parts of the new model have been implemented internally in serializer, but exposing this publicly is not currently on the roadmap for System.Text.Json.

@layomia layomia added this to the 5.0.0 milestone Jul 23, 2020
@layomia layomia removed the untriaged New issue has not been triaged by the area owner label Jul 23, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants