JamesNK / Newtonsoft.Json

Json.NET is a popular high-performance JSON framework for .NET
https://www.newtonsoft.com/json
MIT License
10.73k stars 3.25k forks source link

Supporting thread safety around JsonObjectContract.ExtensionDataSetter #2842

Open sivakumargopalakrishnan opened 1 year ago

sivakumargopalakrishnan commented 1 year ago

Problem statement

We have a fairly large data model involving thousands of properties and a large object hierarchy. We use a custom contract resolver to handle serialization. We also need special handling of missing member data due to the nature of how our models can change over time and we need the ability to capture missing members and store them in a custom collection within the model. The way how we achieveed this was via setting up JsonObjectContract.ExtensionDataSetter. For performance reasons we need to use a static contract resolver and when concurrent deserialization occurs JsonObjectContract.ExtensionDataSetter will get called across deserialization sessions and there is not any good way to know which JsonReader is invoking the ExtensionDataSetter. It will be useful to have ExtensionDataSetter include the JsonReader reference when ExtensionDataSetter is called., The expected signature would be like below

public delegate void ExtensionDataSetter(JsonReader reader, object o, string key, object? value);

If this is not an option, what can be used as a workaround to tie the JsonObjectContract.ExtensionDataSetter to the multiple active Json readers?

elgonzo commented 1 year ago

Why precisely do you need the JsonReader instance in the ExtensionDataSetter? It's not like the ExtensionDataSetter function has to read the extension data itself. The extension data to be set has already been read and is passed to the ExtensionDataSetter function. Not sure if and what kind of workarounds would be possible, as it is difficult to understand why/how exactly the behavior of your ExtensionDataSetter function would depend on a given JsonReader instance.

That said, as the Newtonsoft.Json (de)serializer is basically single-threaded, you might perhaps look into using ThreadLocal / AsyncLocal to carry state into the ExtensionDataSetter function.

sivakumargopalakrishnan commented 1 year ago

The reason to inject the reader in that event handler is to give some context to the event subscription within the contract resolver. As I mentioned already, this is a huge data model we are dealing with and we are using a static instance of contract resolver to read many jsons concurrently by attaching same contract resolver instance via JsonSerializer instances for each JsonTextReader. Sharing a static contract resolver is ideal in this case for performance reasons. We need to track missing members coming from each of these json readers separately. If we simply let the delegate fire for every json reader concurrently there is no way for us to know which json document fired a particular event. Without that context info, we cannot associate the event to a particular document we are reading is the problem I am trying to address with this change. If we start sending the reader instance, I can have a custom reader implelented to capture the missing members of the docuemnt with the JsonReader it is associated with.

By the way given the problem, If there is a better way to handle this within the library I am more than happy to try that out. Please let me know.