Comments (7)
When exactly in the control flow of your program is the code
_jsonSettings = new JsonSerializerSettings();
_jsonSettings.Formatting = Newtonsoft.Json.Formatting.None;
_jsonSettings.NullValueHandling = NullValueHandling.Ignore;
_jsonSettings.Converters.Add(new Newtonsoft.Json.Converters.StringEnumConverter());
_jsonSettings.FloatParseHandling = FloatParseHandling.Decimal;
being executed, and how exactly is the link between the JSONSettings
variable/property used in var obj = JsonConvert.DeserializeObject<SerticaFormObject>(json, JSONSettings);
and the _jsonSettings
variable/field realized?
Because depending on how you implemented this, there is the potential for a race condition regarding the _jsonSettings.Converters
collection. Let me give an example of how the right/wrong implementation of a (hypothetical) JSONSettings
property used in concurrent executions of JsonConvert.DeserializeObject<SerticaFormObject>(json, JSONSettings)
could lead to a race condition that would result in a NRE in the same way you observed.
For the race condition to occur, it is required that the JsonSerializerSettings
instance is initialized on demand and made available to serialization tasks before setting up the _jsonSettings.Converters
collection completes.
An example of a JSONSettings
property that would expose this kind of behavior would be:
public JsonSerializerSettings JSONSettings
{
get
{
if (_jsonSettings == null)
{
_jsonSettings = new JsonSerializerSettings();
...
_jsonSettings.Converters.Add(new Newtonsoft.Json.Converters.StringEnumConverter());
...
}
return _jsonSettings;
}
}
In this example, note how the JsonSerializerSettings instance becomes accessible through the JSONSettings
property as soon as it is assigned to _jsonSettings
even though it isn't completely configured yet.
To understand how the race condition materializes, know that:
-
the
JsonSerializerSettings.Converters
property is being initialized with aList<T>
instance by the JsonSerializerSettings constructor, andList<T>
is not thread-safe:
Newtonsoft.Json/Src/Newtonsoft.Json/JsonSerializerSettings.cs
Lines 457 to 460 in 4738a64
-
when a JsonSerializer instance is created with a given JsonSerializerSettings instance (as JsonConvert.DeserializeObject does internally), JsonSerializer copies the JsonSerializerSettings.Converters items into its own
JsonSerializer.Converters
collection:
Newtonsoft.Json/Src/Newtonsoft.Json/JsonSerializer.cs
Lines 651 to 661 in 4738a64
-
the implementation of
List<T>.Add
adjusts the list size before adding the new item to the backing array (_size
equalsList<T>.Count
): https://github.com/dotnet/runtime/blob/6effb8f669442cbebf0eb3a62b0cd393fb3955be/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/List.cs#L196-L221
This opens the door to the following possible chain of events leading to a NRE as you observed:
- Thread 1 wants to execute
JsonConvert.DeserializeObject<...>(..., JSONSettings)
, but first has to execute theJSONSettings
getter to get the JsonSerializerSettings instance from it. - Thread 1 enters the
JSONSettings
getter, tests for_jsonSettings
being null, enters theif
block and completes execution of_jsonSettings = new JsonSerializerSettings()
- Now thread 2 wants to execute
JsonConvert.DeserializeObject<...>(..., JSONSettings)
, but it also first has to execute theJSONSettings
getter to get the JsonSerializerSettings instance from it. - Thread 2 enters the
JSONSettings
getter, theif
expression "sees" that _jsonSettings contains the JsonSerializerSettings instance created by thread 1 and returns this instance. - Thread 2 is able to invoke
JsonConvert.DeserializeObject<...>(..., JSONSettings)
now, leading it to begin execution of the ApplySerializerSettings method shown above - Thread 1, having a bad day and being slow and constantly being suspended or interrupted for this or that reason has so far only managed to execute the
JSONSettings
getter to the point where it is now starting to add converters to_jsonSettings.Converters
- Due to the implementation of
List<T>.Add
, thread 1 increases the_jsonSettings.Converters
list size/count to 1 but the converter is not yet added to the list's backing array (thus the respective slot in that array still being the default value null at this point). - Thread 2, being a wily quick fox, executes
CollectionUtils.IsNullOrEmpty(settings.Converters))
in theApplySerializerSettings
method, which sees that the _jsonSettings..Converters count is 1 and therefore returns true. - Thread 2 now happily starts copying items from the _jsonSettings.Converters list into the serializer's own converters list.
- Thread 1, having really the worst day of its life, still hasn't come around placing the added converter into the _jsonSettings.Converters backing array yet (probably been busy with having to grow/resize/reallocate the backing array)
- Thread 2 attempts to get the 1st item from the _jsonSettings.Converters list, but the respective slot in its backing array is still null. Thread 2 gets that null value from _jsonSettings.Converters and places it into the serializer's own converter list. KABOOM!
Note that a NRE is not the only failure mode of this race condition. The same race could also lead to an IndexOutOfRangeException in case thread 2 tries to access the list's backing array before it gets resized/reallocated by thread 1 and the used array index happens to be out of bounds for the old and outdated but still active backing array.
The chances for this happening are slim, as the time window for this to occur is very narrow, and the runtime behavior of the CPU cores executing these threads needs to differ in just the "right" way for this narrow time window to open. This could explain why you see this happening only exceedingly rarely and why it would be exceedingly difficult to reliably reproduce the problem.
Please remember that the example i gave is just an illustrative example of how some innocuously looking code could lay the foundations of a race condition that is not obvious to see. I do not claim to know what your code with respect to this JSONSettings
variable/property is. There might very well something else going on in your code.
Also, i am not associated or related to the Newtonsoft.Json project and its author/maintainer. I am just a fellow user of the library (or perhaps former user, as these days it's pretty much always STJ over Newtonsoft.Json)
from newtonsoft.json.
No, the lock implementation as shown in your last comment does NOT prevent the race condition.
Note that any second thread will not enter the lock when it sees _jsonSettings
not being null anymore. Which means, the race is still on, and the second thread can access the JsonSerializerSettings instance in _jsonSettings
even though _jsonSettings is not yet fully configured by the first thread executing the locked section.
I would suggest to not use lock
in this case, and instead use something like Lazy<T>
:
public static JsonSerializerSettings JSONSettings => _jsonSettings.Value;
private static readonly Lazy<JsonSerializerSettings> _jsonSettings = new Lazy<JsonSerializerSettings>(
() => {
var settings = new JsonSerializerSettings();
settings.Formatting = Newtonsoft.Json.Formatting.None;
settings.NullValueHandling = NullValueHandling.Ignore;
settings.Converters.Add(new Newtonsoft.Json.Converters.StringEnumConverter());
settings.FloatParseHandling = FloatParseHandling.Decimal;
return settings;
}
);
from newtonsoft.json.
I should obviously wait assigning the private backing variable until done with initializing.
That alone would not be sufficient to avoid the race condition for certain. You would also need to make the field _jsonSettings
volatile, otherwise the JIT compiler and/or the CPU are allowed to reorder the execution, for example to something like this:
var newSettings = new JsonSerializerSettings();
_jsonSettings = newSettings;
newSettings.Formatting = Newtonsoft.Json.Formatting.None;
newSettings.NullValueHandling = NullValueHandling.Ignore;
newSettings.Converters.Add(new Newtonsoft.Json.Converters.StringEnumConverter());
newSettings.FloatParseHandling = FloatParseHandling.Decimal;
Not sure if that is going to happen or not for some given CLR/JIT version and/or CPU arch/family, but this kind of reordering is permissible by the C# language standard (7.10 Execution order)
from newtonsoft.json.
That sounds reasonable.
Thumbs up for a thorough answer, and for seeing the potential even though I left out the important null check.
The property for the SerializerSettings is a static method, that two threads potentially can race. And exactly as you write.
In code they are a bit further apart, and part of something bigger, which was why I pasted as I did.
[Browsable(false)]
[JsonIgnore]
public static JsonSerializerSettings JSONSettings
{
get
{
if (_jsonSettings == null)
{
_jsonSettings = new JsonSerializerSettings();
_jsonSettings.Formatting = Newtonsoft.Json.Formatting.None;
_jsonSettings.NullValueHandling = NullValueHandling.Ignore;
_jsonSettings.Converters.Add(new Newtonsoft.Json.Converters.StringEnumConverter());
_jsonSettings.FloatParseHandling = FloatParseHandling.Decimal;
}
return _jsonSettings;
}
}
But your point is definitely valid, though hard to reproduce as you state.
For reference, we will fix it on our side by putting a lock statement around the getter, so we are sure that only one thread is initializing this at a time. Costing only a fraction of time the first time someone needs this in the entire API lifetime.
private static object jsonSettingslock = new object();
[ [Browsable(false)]
[JsonIgnore]
public static JsonSerializerSettings JSONSettings
{
get
{
if (_jsonSettings == null)
{
lock(jsonSettingslock)
{
if (_jsonSettings == null)
{
_jsonSettings = new JsonSerializerSettings();
_jsonSettings.Formatting = Newtonsoft.Json.Formatting.None;
_jsonSettings.NullValueHandling = NullValueHandling.Ignore;
_jsonSettings.Converters.Add(new Newtonsoft.Json.Converters.StringEnumConverter());
_jsonSettings.FloatParseHandling = FloatParseHandling.Decimal;
}
}
}
return _jsonSettings;
}
}
public static FormObject DeSerialize(string json)
{
var obj = JsonConvert.DeserializeObject<FormObject>(json, JSONSettings);
obj.Fields.ForEach(x => x.ParentObject = obj);
return obj;
}
Once again, thank you very much for the rapid and thorough answer.
from newtonsoft.json.
Closing issue, as it seems resolved for my part.
from newtonsoft.json.
You are absolutely right.
I chose lock because that is the pattern that our solution uses other places. And is well known to all the other developers.
I should obviously wait assigning the private backing variable until done with initializing.
private static object jsonSettingslock = new object();
[Browsable(false)]
[JsonIgnore]
public static JsonSerializerSettings JSONSettings
{
get
{
if (_jsonSettings == null)
{
lock(jsonSettingslock)
{
if (_jsonSettings == null)
{
var newSettings = new JsonSerializerSettings();
newSettings.Formatting = Newtonsoft.Json.Formatting.None;
newSettings.NullValueHandling = NullValueHandling.Ignore;
newSettings.Converters.Add(new Newtonsoft.Json.Converters.StringEnumConverter());
newSettings.FloatParseHandling = FloatParseHandling.Decimal;
_jsonSettings = newSettings;
}
}
}
return _jsonSettings;
}
}
I will create an internal issue here to get it fixed and reviewed, Will take a look on Lazy and see if that is a better fit.
from newtonsoft.json.
I have replaced the logic with Lazy.
Found four other places in other modules that has similar flaws, so those are changed to Lazy as well.
So now for the manual testing, and off to a colleague to verify that everything else is still working.
Thanks for the explanations and assistance once again.
from newtonsoft.json.
Related Issues (20)
- Json.NET deserializes nested types using only the last entry in the nested array HOT 2
- error CS0104: 'Func<,>' is an ambiguous reference between 'Newtonsoft.Json.Serialization.Func<T, TResult>' and 'System.Func<T, TResult>' HOT 5
- Exception during deserialization with TypeNameHandling = TypeNameHandling.All a json that was saved by browser HOT 2
- Could not determine JSON object type for type System.Collections.Generic.KeyValuePair HOT 1
- Unity cyclic references of Unity objects HOT 2
- Ignoring JsonProperty Name HOT 2
- [Enhancement] when serializing/deserializing a netstandard lib used in .net framework/ net core
- JsonConvert.DeserializeObject appends value for no reason HOT 2
- Ask for the "PrivacyInfo.xcprivacy" required by Apple
- [Bug] CreateProperties adds JsonIgnore fields when it should not MemberSerialization.OptOut HOT 2
- Could not create an instance of type X. Type is an interface or abstract class and cannot be instantiated. HOT 2
- Problem with Deserialize number like 0...8 and 0...9 HOT 2
- deserialize object Ignore case HOT 1
- Deserializing object of missing class as it's Base class (C#) HOT 1
- Source code version problem HOT 1
- [Feature Request] MissingMemberHandling attribute HOT 4
- JsonConvert.DeserializeObject using $type does not (at least for the root object) not use the converter HOT 3
- JsonConverter is called despite that CanConvert returns false, and despite that the target type isn't decorated with JsonConverterAttribute HOT 6
- ReferenceLoopHandling.Ignore unexpectedly skips objects - false positive if Equals/HashCode result is the same
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from newtonsoft.json.