Possible thread-unsafe serialization - DefaultReferenceResolver

[kiziu]

Application I am developing has been throwing for some time intermittent exceptions:

Newtonsoft.Json.JsonSerializationException: Error writing object reference for 'TYPE'. Path '$values[INDEX]'. ---> System.ArgumentException: A different value already has the Id 'ID'.

I tried reproducing in debug sessions, but I couldn't get any results. After that, I started looking at the code based on the stack trace, which lead to method https:/JamesNK/Newtonsoft.Json/blob/master/Src/Newtonsoft.Json/Serialization/JsonSerializerInternalWriter.cs#L366. From there, I went to https:/JamesNK/Newtonsoft.Json/blob/master/Src/Newtonsoft.Json/Serialization/DefaultReferenceResolver.cs#L69. I suspect that JsonSerializer is not thread-safe because of this resolver. Although JsonSerializerInternalWriter is instantiated for each serialization/deserialization process (therefore the reference mappings are also "local"), the resolver is provided by JsonSerializer via method https:/JamesNK/Newtonsoft.Json/blob/master/Src/Newtonsoft.Json/JsonSerializer.cs#L1150, which means that the resolver is reused between different threads (which could be parallel). Since the _referenceCount field is reused, any access to it should be locked, as two parallel threads might start incrementing and using the same value for reference. Can you please confirm my suspicions and maybe fix this in next release? Thank you!

Steps to reproduce

Highly unlikely to reproduce, as the referenceCount incrementation must occur at almost the same time on two parallel threads.

[TylerBrinkley]

#1393 should fix this issue. If that's not accepted Interlocked.Increment should be used.

[TylerBrinkley]

@JamesNK this is a real issue due to your use of the post increment operator here. I'd suggest you should consider accepting #1393 which also corrects this issue. Alternatively you could simply fix this by replacing the post increment operator with a call to Interlocked.Increment instead.

[TylerBrinkley]

@JamesNK Please consider fixing this concurrency issue. I've laid out the potential fixes, just waiting on you to okay a direction.

[Terebi42]

@TylerBrinkley @JamesNK My app is hitting this issue during heavy usage in production :(

[Sergey-Terekhin]

It's actual for v.11

[TylerBrinkley]

@JamesNK Please consider fixing this concurrency issue. I've laid out the potential fixes, just waiting on you to okay a direction.

[jmrnilsson]

Me thought ReferanceResolver was the cause of wierd output but turned out to be static variable elsewhere..

[Sergey-Terekhin]

I use ThreadLocal<Newtonsoft.Json.JsonSerializer> instead of shared instance. It allows to avoid locks and be thread-safe. The cost is instance per thread (not so much I think)

[apdevelop]

Had a similar issue (but it's hard to reproduce) with DefaultContractResolver and latest (12.0.2) version. The JsonConvert.SerializeObject is called inside async method and in random cases exception is thrown:

Parameter name: attributeProvider System.ArgumentNullException
at System.Attribute[] GetAttributes(System.Object, System.Type, Boolean)
at Newtonsoft.Json.Utilities.ReflectionUtils.GetAttributes(Object attributeProvider, Type attributeType, Boolean inherit)
at Newtonsoft.Json.Utilities.ReflectionUtils.GetAttributes[T](Object attributeProvider, Boolean inherit)
at Newtonsoft.Json.Utilities.ReflectionUtils.GetAttribute[T](Object attributeProvider, Boolean inherit)
at Newtonsoft.Json.Serialization.JsonTypeReflector.GetAttribute[T](Type type)
at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey key, Func`2 valueFactory)
at Newtonsoft.Json.Serialization.DefaultContractResolver.InitializeContract(JsonContract contract)
at Newtonsoft.Json.Serialization.DefaultContractResolver.CreatePrimitiveContract(Type objectType)
at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey key, Func`2 valueFactory)
at Newtonsoft.Json.Serialization.DefaultContractResolver.ResolveContract(Type type)
at Newtonsoft.Json.Serialization.JsonSerializerInternalWriter.CalculatePropertyValues(JsonWriter writer, Object value, JsonContainerContract contract, JsonProperty member, JsonProperty property, JsonContract& memberContract, Object& memberValue)
at Newtonsoft.Json.Serialization.JsonSerializerInternalWriter.SerializeObject(JsonWriter writer, Object value, JsonObjectContract contract, JsonProperty member, JsonContainerContract collectionContract, JsonProperty containerProperty)
at Newtonsoft.Json.Serialization.JsonSerializerInternalWriter.Serialize(JsonWriter jsonWriter, Object value, Type objectType)
at Newtonsoft.Json.JsonSerializer.SerializeInternal(JsonWriter jsonWriter, Object value, Type objectType)
at Newtonsoft.Json.JsonConvert.SerializeObjectInternal(Object value, Type type, JsonSerializer jsonSerializer)
at Newtonsoft.Json.JsonConvert.SerializeObject(Object value)

[stricq]

Why has this not been fixed yet? I'm hitting this issue in 12.0.3. I really can't understand why this issue has been sitting unfixed for 3 YEARS.

[stricq]

One year on and still not fixed.

[sungam3r]

This is normal for oss.

[CenturySparkle]

This is still a major problem. Why has this not been fixed? Especially when the fix is so drop dead simple? I see version after version after version released for this software, and that's great. But why such a simple fix keeps getting ignored is beyond my understanding.

[sungam3r]

This is normal for oss project which is no longer actively developed.

[CenturySparkle]

Looks like it is actively being developed to me. And, the issue has been open for 5 years now.

[sungam3r]

https:/JamesNK/Newtonsoft.Json/graphs/contributors

Over the past few years, trends (commits, open/merged PRs) are obvious.

#862 (comment)

[jmrnilsson]

I did write something on an issue around here somewhere a few year ago. Anyways my issue was actually poor thread safety else were that made Newtonsoft go boom. Check for static variables elsewhere, that could be a clue for some. I believed it had something to do with extensive caching of Newtonsoft that made it appear like an error from here.