Async Stream-based JSON serialization#1976
Conversation
daiplusplus
changed the title
|
Any new information about when it will be merged? Thanks in advance |
|
I cannot merge this PR as there are too many unrelated changes (e.g. change of full qualified types to using usings which do pollute the namespace with lots of types which might lead to name clashes/compile errors for some users) and other breaking changes. Changes like this must not break existing users. Please create/update the PR with only the stream-based JSON serialization feature for a review... |
|
@Jehoel |
daiplusplus
force-pushed
the
dev/better-templates
branch
from
ce3118f to
050f762
Compare
|
@RicoSuter @juanperezADD Sorry for the delay - I've rebased my changes to the latest I wonder if we could profile/benchmark my changes against the current generated code against large and small JSON inputs. |
daiplusplus
changed the title
|
Btw: The new json serializers for .net core 3.0 do not use async, because for small payloads it is killing your performance, due to many context switches. |
|
@SebastianStehle off-topic: but if Microsoft's JSON API will not support async reading from streams, what is their official guidance for deserializing large objects? Surely they could use some form of buffering? I do wish the Async IO API in .NET included a way for sources to pass metadata like ideal buffer size to their sinks so synchronous parsers can read tokens from an async source, for example. |
|
@Jehoel I think the guidance is to use another serializer ;) ... The System.Text.Json thing is optimized for Web Apis where you typically have small payloads of a few KB to MB |
|
@RicoSuter are we good to merge? :) |
| using (System.IO.StreamReader streamReader = new System.IO.StreamReader(responseStream)) | ||
| using (Newtonsoft.Json.JsonTextReader jsonTextReader = new Newtonsoft.Json.JsonTextReader(streamReader)) | ||
| { | ||
| Newtonsoft.Json.JsonSerializer serializer = Newtonsoft.Json.JsonSerializer.Create(this.JsonSerializerSettings); |
There was a problem hiding this comment.
Maybe we should not recreate the serializer for each request?
There was a problem hiding this comment.
This is a bit tricky. While JsonSerializer is thread-safe, JsonTextReader is not, and the JsonSerializerSettings object is also directly exposed via the JsonSerializerSettings { get { ... } } property. See here: https://stackoverflow.com/questions/36186276/is-the-json-net-jsonserializer-threadsafe which implicitly makes any object using an exposed JsonSerializerSettings instance also unsafe for concurrency - I note that this issue also exists in the current NSwag-generated code too.
I did try to use a single instance of JsonSerializer, however the JsonSerializerSettings property makes use of a Lazy<T> field to store it, and creating two objects from a single Lazy<T> is non-trivial. Also, JsonSerializerSettings lacks a Clone method that could be used to create a private copy for thread-safety purposes.
I don't think there are any good solutions while we use Lazy<T> and have an exposed property at the same time.
There was a problem hiding this comment.
I personally would define set up the implicit rule that the settings cannot be changed when the first operation method is called only before when setting up the client... create a new client when you need different settings. With this rule we could reuse the serializer... The serialization settings are only exposed to use in external serializations not to modify it - but there is not a read-only version of the settings that's why the mutable settings have to be exposed... (public settings property option)
|
Please review the PR. I've fixed the templates. |
|
I just fixed a bug I overlooked earlier where |
|
I need to do more testing before i can merge (with actual running the client. Did anyone do that with a bigger api? |
|
I’ve added two more comments and i think it would also make sense to change the wording of responseobject vs objectresponse as this is a bit confusing 🙂 Ill do that if it’s ok? |
This reverts commit 8938b74.
4 participants
As mentioned in this issue thread: #1666
This PR also includes other changes that are orthogonal to the intended change of only enabling stream-based deserialization instead of always using an intermediateStringinstance.This PR passes all JSON object deserialization work to a new method:
This method uses the
StreamfromHttpResponseMessage.ReadAsStreamAsync()andJsonTextReaderto deserialize the expected object from the stream without using an intermediateStringinstance. This should result in reduced memory consumption, especially when dealing with large JSON object responses. Because the response is not buffered into a string it means that if an error is encountered the program cannot view the raw response body as a string.If the
DEBUGcompilation symbol is defined then the method will instead read the response using an intermediate string (Passing the string fromReadAsStringAsync()intoJsonConvert.DeserializeObject) and it will include that string in any raised exceptions.I wonder if
#if DEBUGcould be replaced with#if STRING_DESERIALIZATIONand adding#if DEBUG\r\n#define STRING_DESERIALIZATION\r\n#endifto the top of the file might be more flexible. I'll see what the project maintainers say.Unfortunately Json.NET still does not offer an Async
DeserializeObject<T>method. WhileJsonReader/JsonWriterdo support async IO, they're only for reading and writing JSON tokens (JToken,JObject,JArray, etc) and not for whole-object deserialization/serialization. I was thinking that I could read the response asynchronously usingJsonReaderinto a rootJTokenhowever it isn't feasible to then deserialize an object from aJToken- a quick look inside Json.NET revealed that deserialization uses a very different code-path entirely and doesn't useJTokenas an intermediary. It doesn't look like this will be fixed in Json.NET given it's a long outstanding request (dating back to 2012) and Microsoft have announced their own forthcoming JSON serialization/deserialization library which they say will have an emphasis on performance.