`JsonConvert.DeserializeObject` throws an exception when called multiple time from multiple threads

[cbagpipe]

Library/API/IoT binding

nanoFramework.Json

Visual Studio version

VS2022 (17.3.4)

.NET nanoFramework extension version

2022.2.0.41

Target name(s)

ESP32_REV0

Firmware version

1.8.0.594

Device capabilities

System Information
HAL build info: nanoCLR running @ ESP32 built with ESP-IDF 1b16ef6
  Target:   ESP32
  Platform: ESP32

Firmware build Info:
  Date:        Sep 22 2022
  Type:        MinSizeRel build, chip rev. >= 0, without support for PSRAM
  CLR Version: 1.8.0.594
  Compiler:    GNU ARM GCC v8.4.0

OEM Product codes (vendor, model, SKU): 0, 0, 0

Serial Numbers (module, system):
  00000000000000000000000000000000
  0000000000000000

Target capabilities:
  Has nanoBooter: NO
  IFU capable: NO
  Has proprietary bootloader: YES

AppDomains:

Assemblies:
  nFHelloWorld, 1.0.0.0
  System.Threading, 1.1.8.6695
  System.IO.Streams, 1.1.15.36091
  nanoFramework.Json, 2.2.34.8594
  System.IO.Ports, 1.1.25.11551
  nanoFramework.Runtime.Native, 1.5.4.3
  System.IO.FileSystem, 1.1.5.25011
  nanoFramework.Runtime.Events, 1.10.0.3
  nanoFramework.System.Collections, 1.4.0.3
  mscorlib, 1.12.0.4
  System.Math, 1.4.4.3
  Windows.Storage.Streams, 1.14.8.30993
  nanoFramework.System.Text, 1.2.7.60109

Native Assemblies:
  mscorlib v100.5.0.17, checksum 0x004CF1CE
  nanoFramework.Runtime.Native v100.0.9.0, checksum 0x109F6F22
  nanoFramework.Hardware.Esp32 v100.0.7.3, checksum 0xBE7FF253
  nanoFramework.Hardware.Esp32.Rmt v100.0.3.0, checksum 0x0A915860
  nanoFramework.Device.OneWire v100.0.4.0, checksum 0xB95C43B4
  nanoFramework.Networking.Sntp v100.0.4.4, checksum 0xE2D9BDED
  nanoFramework.ResourceManager v100.0.0.1, checksum 0xDCD7DF4D
  nanoFramework.System.Collections v100.0.1.0, checksum 0x2DC2B090
  nanoFramework.System.Text v100.0.0.1, checksum 0x8E6EB73D
  nanoFramework.Runtime.Events v100.0.8.0, checksum 0x0EAB00C9
  EventSink v1.0.0.0, checksum 0xF32F4C3E
  System.IO.FileSystem v1.0.0.0, checksum 0x3AB74021
  System.Math v100.0.5.4, checksum 0x46092CB1
  System.Net v100.1.5.0, checksum 0x5BAB8CB3
  System.Device.Adc v100.0.0.0, checksum 0xE5B80F0B
  System.Device.Dac v100.0.0.6, checksum 0x02B3E860
  System.Device.Gpio v100.1.0.6, checksum 0x097E7BC5
  System.Device.I2c v100.0.0.1, checksum 0xFA806D33
  System.Device.Pwm v100.1.0.4, checksum 0xABF532C3
  System.IO.Ports v100.1.6.1, checksum 0xB798CE30
  System.Device.Spi v100.1.2.0, checksum 0x3F6E2A7E
  System.Device.Wifi v100.0.6.4, checksum 0x1C1D3214
  Windows.Storage v100.0.2.0, checksum 0x954A4192



++++++++++++++++++++++++++++++++
++        Memory Map          ++
++++++++++++++++++++++++++++++++
  Type     Start       Size
++++++++++++++++++++++++++++++++
  RAM   0x3ffe49ac  0x0001b000
  FLASH 0x00000000  0x00400000



+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
++                   Flash Sector Map                        ++
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
  Region     Start      Blocks   Bytes/Block    Usage
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
      0    0x00010000       1      0x1A0000     nanoCLR
      1    0x001B0000       1      0x1F0000     Deployment
      2    0x003C0000       1      0x040000     Configuration


+++++++++++++++++++++++++++++++++++++++++++++++++++
++              Storage Usage Map                ++
+++++++++++++++++++++++++++++++++++++++++++++++++++
  Start        Size (kB)           Usage
+++++++++++++++++++++++++++++++++++++++++++++++++++
  0x003C0000    0x040000 (256kB)    Configuration
  0x00010000    0x1A0000 (1664kB)   nanoCLR
  0x001B0000    0x1F0000 (1984kB)   Deployment



Deployment Map
Empty

Description

When running multiple threads, in each of which JsonConvert.DeserializeObject called often, it begins to throw exceptions from random place of JsonConvert.DeserializeObject's inside...

How to reproduce

1. Create empty project
2. Paste sample code below
3. Run

Expected behaviour

JSON deserialized to object without errors

Screenshots

Sample project or code

public class Program
    {
        public static void Main()
        {
            ArrayList threads = new ArrayList();

            for (int i = 0; i < 2; i++)
            {
                var countThread = new Thread(() => ThreadMethod(i));
                countThread.Start();
                threads.Add(countThread);
                Thread.Sleep(10000);
            }

            Thread.Sleep(Timeout.Infinite);
        }

        private static void ThreadMethod(int i)
        {
            var json = "{\"TestString\":\"towel\"}";
            int count = 0;
            while (true)
            {
                try
                {
                    JsonConvert.DeserializeObject(json, typeof(TestObject));
                    Debug.Write($"{count++}; ");
                }
                catch (Exception ex)
                {
                    Debug.WriteLine($">>>T{i}<<<");
                }
                Thread.Sleep(10);
            }
        }
    }

    public class TestObject
    {
        public string TestString { get; set; }
    }

Aditional information

No response

[josesimoes]

I recall that some of those classes had sync objects to lock the internal operations. Not sure if this is still there...
Agreed that we need to improved this library to make it thread safe.

cc @torbacz @Ellerbach

[torbacz]

@josesimoes
To be honest, I can't remember any locks/sync objects in JSON classes. At least back when I started making changes... But it's clearly multithreading issue.

[josesimoes]

Yeah... this was probably 2 or 3 iterations ago. This library it's a serious case of global refactoring and improvements! And it keeps getting better and better.😅

[cbagpipe]

I added lock to sample code and it's working now

    public class Program
    {
        static object monitor = new object();

        public static void Main()
        {
            ArrayList threads = new ArrayList();

            for (int i = 0; i < 10; i++)
            {
                var countThread = new Thread(() => ThreadMethod(i));
                countThread.Start();
                threads.Add(countThread);
            }

            Thread.Sleep(Timeout.Infinite);
        }

        private static void ThreadMethod(int i)
        {
            var json = "{\"TestString\":\"towel\"}";
            int count = 0;
            while (true)
            {
                try
                {
                    lock (monitor)
                    {
                        JsonConvert.DeserializeObject(json, typeof(TestObject));
                    }
                    Debug.Write($"{count++}; ");
                }
                catch (Exception ex)
                {
                    Debug.WriteLine($">>>T{i}<<<");
                }
                Thread.Sleep(10);
            }
        }
    }

    public class TestObject
    {
        public string TestString { get; set; }
    }

[josesimoes]

@cbagpipe yes, that should be the "obvious" fix.
Now, we should relieve the caller from having to deal with that.

Several options are possible:

1. Adding the lock inside the DeserializeObject method.
2. Find out where lock's should exist internally in the library code to save us from locking the complete class.
3. Making a note that the library is not thread safe (I personally dislike this one).
4. Another approach(es) that someone else can suggest.

[torbacz]

https://github.com/nanoframework/nanoFramework.Json/blob/be9eda9925a39bd04fbd83a1d5daefdd02ddb1aa/nanoFramework.Json/JsonConvert.cs#L865
Check this out, if you are running multiple threads, and there is no locks it will likely override static fields in deserialization. And that's why you are getting exception.

My suggestion is to move

        private static byte[] jsonBytes;     // Do all deserialization using this byte[]
        private static int jsonPos;      // Current position in jsonBytes[]

into

        private static object Deserialize(string sourceString)
        {
            **var** jsonBytes = new byte[sourceString.Length];
            jsonBytes = Encoding.UTF8.GetBytes(sourceString);
            **var** jsonPos = 0;
            return Deserialize(); <- pass here
        }

It should be thread safe then.

BUT

• It may impact performance
• For sure it will require more runtime memory, because each call will allocate new memory

Haven't test that, but I'm pretty sure that it's causing the problem.