NullReferenceException in JsonSerializer.GetMatchingConverter(IList<JsonConverter> converters, Type objectType)

[RinaAndersHansen]

NullReferenceException in JsonSerializer.GetMatchingConverter(IList converters, Type objectType)

from newtonsoft.json.

[elgonzo]

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 a List<T> instance by the JsonSerializerSettings constructor, and List<T> is not thread-safe:
  

  Newtonsoft.Json/Src/Newtonsoft.Json/JsonSerializerSettings.cs

  Lines 457 to 460 in 4738a64

  	public JsonSerializerSettings()
  	{
  	Converters = new List<JsonConverter>();
  	}

• 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

  	private static void ApplySerializerSettings(JsonSerializer serializer, JsonSerializerSettings settings)
  	{
  	if (!CollectionUtils.IsNullOrEmpty(settings.Converters))
  	{
  	// insert settings converters at the beginning so they take precedence
  	// if user wants to remove one of the default converters they will have to do it manually
  	for (int i = 0; i < settings.Converters.Count; i++)
  	{
  	serializer.Converters.Insert(i, settings.Converters[i]);
  	}
  	}

• the implementation of List<T>.Add adjusts the list size before adding the new item to the backing array (_size equals List<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:

1. Thread 1 wants to execute JsonConvert.DeserializeObject<...>(..., JSONSettings), but first has to execute the JSONSettings getter to get the JsonSerializerSettings instance from it.
2. Thread 1 enters the JSONSettings getter, tests for _jsonSettings being null, enters the if block and completes execution of _jsonSettings = new JsonSerializerSettings()
3. Now thread 2 wants to execute JsonConvert.DeserializeObject<...>(..., JSONSettings), but it also first has to execute the JSONSettings getter to get the JsonSerializerSettings instance from it.
4. Thread 2 enters the JSONSettings getter, the if expression "sees" that _jsonSettings contains the JsonSerializerSettings instance created by thread 1 and returns this instance.
5. Thread 2 is able to invoke JsonConvert.DeserializeObject<...>(..., JSONSettings) now, leading it to begin execution of the ApplySerializerSettings method shown above
6. 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
7. 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).
8. Thread 2, being a wily quick fox, executes CollectionUtils.IsNullOrEmpty(settings.Converters)) in the ApplySerializerSettings method, which sees that the _jsonSettings..Converters count is 1 and therefore returns true.
9. Thread 2 now happily starts copying items from the _jsonSettings.Converters list into the serializer's own converters list.
10. 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)
11. 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)

[elgonzo]

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;
    }
);

[elgonzo]

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)

[RinaAndersHansen]

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.

[RinaAndersHansen]

Closing issue, as it seems resolved for my part.

[RinaAndersHansen]

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.

[RinaAndersHansen]

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.