ConcurrentQueue in a ConcurrentDictionary Duplicate Error

Question

I have a thread that handles the message receiving every 10 seconds and have another one write these messages to the database every minute. Each message has a different sender which is named serialNumber in my case.

Therefore, I created a ConcurrentDictionary like below.

public ConcurrentDictionary<string, ConcurrentQueue<PacketModel>> _dicAllPackets;

The key of the dictionary is serialNumber and the value is the collection of 1-minute messages. The reason I want to collect a minute of data is instead of going database every 10 seconds is go once in every minute so I can reduce the process by 1/6 times.

public class ShotManager
    {
        private const int SLEEP_THREAD_FOR_FILE_LIST_DB_SHOOTER = 25000;
        private bool ACTIVE_FILE_DB_SHOOT_THREAD = false;
        private List<Devices> _devices = new List<Devices>();
        public ConcurrentDictionary<string, ConcurrentQueue<PacketModel>> _dicAllPackets;

        public ShotManager()
        {
            ACTIVE_FILE_DB_SHOOT_THREAD = Utility.GetAppSettings("AppConfig", "0", "ACTIVE_LIST_DB_SHOOT") == "1";
            init();
        }
        private void init()
        {
            using (iotemplaridbContext dbContext = new iotemplaridbContext())
                _devices = (from d in dbContext.Devices select d).ToList();

            if (_dicAllPackets is null)
                _dicAllPackets = new ConcurrentDictionary<string, ConcurrentQueue<PacketModel>>();

            foreach (var device in _devices)
            {
                if(!_dicAllPackets.ContainsKey(device.SerialNumber))
                    _dicAllPackets.TryAdd(device.SerialNumber, new ConcurrentQueue<PacketModel> { });
            }
        }
        public void Spinner()
        {
            while (ACTIVE_FILE_DB_SHOOT_THREAD)
            {
                try
                {
                    Parallel.ForEach(_dicAllPackets, devicePacket =>
                    {
                        Thread.Sleep(100);
                        readAndShot(devicePacket);
                    });
                    Thread.Sleep(SLEEP_THREAD_FOR_FILE_LIST_DB_SHOOTER);
                    //init();
                }
                catch (Exception ex)
                {
                    //init();
                    tLogger.EXC("Spinner exception for write...", ex);
                }
            }
        }
        public void EnqueueObjectToQueue(string serialNumber, PacketModel model)
        {
            if (_dicAllPackets != null)
            {
                if (!_dicAllPackets.ContainsKey(serialNumber))
                    _dicAllPackets.TryAdd(serialNumber, new ConcurrentQueue<PacketModel> { });
                else
                    _dicAllPackets[serialNumber].Enqueue(model);
            }
        }
        private void readAndShot(KeyValuePair<string, ConcurrentQueue<PacketModel>> keyValuePair)
        {
            StringBuilder sb = new StringBuilder();
            if (keyValuePair.Value.Count() <= 0)
            {
                return;
            }
            sb.AppendLine($"INSERT INTO ......) VALUES(");
//the reason why I don't use while(TryDequeue(out ..)){..} is there's constantly enqueue to this dictionary, so the thread will be occupied with a single device for so long
            for (int i = 0; i < 10; i++)
            {
                keyValuePair.Value.TryDequeue(out PacketModel packet);
                if (packet != null)
                {
                    /*
                     *** do something and fill the sb...
                     */
                }
                else
                {
                    Console.WriteLine("No packet found! For Device: " + keyValuePair.Key);
                    break;
                }
            }
            insertIntoDB(sb.ToString()[..(sb.Length - 5)] + ";");
        }
    }

EnqueueObjectToQueue caller is from a different class like below.

private void packetToDictionary(string serialNumber, string jsonPacket, string messageTimeStamp)
{
            PacketModel model = new PacketModel {
                MachineData = jsonPacket, 
                DataInsertedAt = messageTimeStamp
            };
            _shotManager.EnqueueObjectToQueue(serialNumber, model);
}

How I call the above function is from the handler function itself.

private void messageReceiveHandler(object sender, MessageReceviedEventArgs e){
   //do something...parse from e and call the func
   string jsonPacket = ""; //something parsed from e 
   string serialNumber = ""; //something parsed from e
   string message_timestamp = DateTime.Now().ToString("yyyy-MM-dd HH:mm:ss");
   ThreadPool.QueueUserWorkItem(state => packetToDictionary(serialNumber, str, message_timestamp));
}

The problem is sometimes some packets are enqueued under the wrong serialNumber or repeat itself(duplicate entry).

Is it clever to use ConcurrentQueue in a ConcurrentDictionary like this?

Answer 1

No, it's not a good idea to use a ConcurrentDictionary with nested ConcurrentQueues as values. It's impossible to update atomically this structure. Take this for example:

if (!_dicAllPackets.ContainsKey(serialNumber))
    _dicAllPackets.TryAdd(serialNumber, new ConcurrentQueue<PacketModel> { });
else
    _dicAllPackets[serialNumber].Enqueue(model);

This little piece of code is riddled with race conditions. A thread that is running this code can be intercepted by another thread at any point between the ContainsKey, TryAdd, the [] indexer and the Enqueue invocations, altering the state of the structure, and invalidating the conditions on which the correctness of the current thread's work is based.

A ConcurrentDictionary is a good idea when you have a simple Dictionary that contains immutable values, you want to use it concurrently, and using a lock around each access could potentially create significant contention. You can read more about this here: When should I use ConcurrentDictionary and Dictionary?

My suggestion is to switch to a simple Dictionary<string, Queue<PacketModel>>, and synchronize it with a lock. If you are careful and you avoid doing anything irrelevant while holding the lock, the lock will be released so quickly that rarely other threads will be blocked by it. Use the lock just to protect the reading and updating of a specific entry of the structure, and nothing else.

Alternative designs

A ConcurrentDictionary<string, Queue<PacketModel>> structure might be a good option, under the condition that you never removed queues from the dictionary. Otherwise there is still space for race conditions to occur. You should use exclusively the GetOrAdd method to get or add atomically a queue in the dictionary, and also use always the queue itself as a locker before doing anything with it (either reading or writing):

Queue<PacketModel> queue = _dicAllPackets
    .GetOrAdd(serialNumber, _ => new Queue<PacketModel>());
lock (queue)
{
    queue.Enqueue(model);
}

Using a ConcurrentDictionary<string, ImmutableQueue<PacketModel>> is also possible because in this case the value of the ConcurrentDictionary is immutable, and you won't need to lock anything. You'll need to use always the AddOrUpdate method, in order to update the dictionary with a single call, as an atomic operation.

_dicAllPackets.AddOrUpdate
(
    serialNumber,
    key => ImmutableQueue.Create<PacketModel>(model),
    (key, queue) => queue.Enqueue(model)
);

The queue.Enqueue(model) call inside the updateValueFactory delegate does not mutate the queue. Instead it creates a new ImmutableQueue<PacketModel> and discards the previous one. The immutable collections are not very efficient in general. But if your goal is to minimize the contention between threads, at the cost of increasing the work that each thread has to do, then you might find them useful.