-2

I have code like this:

public ConcurrentDictionary<Guid, DeviceItem> deviceStatesCache = new ConcurrentDictionary<Guid, DeviceItem>();
private readonly object deviceStatesCacheLock = new object();

public void StoreDeviceStateInCache(Guid guid, DeviceItem deviceState) 
{
        bool added, removed, updated = false;
        
        lock (deviceStatesCacheLock)
        { 
            added = deviceStatesCache.TryAdd(guid, deviceState);

            if (!added) {
                removed = deviceStatesCache.TryRemove(guid, out var _);
                if (removed)
                { 
                    updated = deviceStatesCache.TryAdd(guid, deviceState);
                }
            }
        }

        if (!updated) 
            throw new Exception("WTF: cannot update deviceStatesCache!");
}

private QueryDevicesResponse CreateQueryDevicesResponse()
{
        var deviceItems = new List<DeviceItem>();

        lock (deviceStatesCacheLock)
        {
            foreach (var item in deviceStatesCache)
            {
                deviceItems.Add(item.Value);
            }
        }

        var response = new QueryDevicesResponse()
        {
            eventData = new QueryDevicesResponse.EventData()
            {
                devices = deviceItems
            }
        };

        // response.eventSourceGuid = Guid.Empty.ToString();

        return response;
}

(edit)

My lock here is supposed to work in situations:

  1. When another thread tries to get value when item is between remove and insert in StoreDeviceStateInCache method (and only this).

  2. When another thread started reading all items in CreateQueryDevicesResponse() method.

Can I simplify StoreDeviceStateInCache() method a bit?

I know that AddOrUpdate method exists, but I can't find it helpful since I have to define additional methods and that would look less readable than my code.

looks like Michael Liu suggestion with deviceStatesCache[guid] = deviceState; should be enough

Can I simplify CreateQueryDevicesResponse() method a bit and read complete dictionary "atomically"?

(WTF stands for "What a Terrible Failure" kind of exception of course)

Michael Liu
  • 52,147
  • 13
  • 117
  • 150
Kamil
  • 13,363
  • 24
  • 88
  • 183
  • 1
    Please explain exactly what your operation is supposed to be doing, and what kind of thread safety you're aiming for. – Lasse V. Karlsen Nov 04 '20 at 20:27
  • One way to simplify the code is to replace the `ConcurrentDictionary` with a normal `Dictionary`. If you have to lock around *any* of the `ConcurrentDictionary`'s members, then you have to lock around *all* of them (otherwise it won't be thread-safe). And in that case all the advantages of the `ConcurrentDictionary` disappear, and you are left with its disadvantages (inconvenient API and synchronization overhead). – Theodor Zoulias Nov 04 '20 at 20:53

2 Answers2

4

If I'm understanding your code correctly, you want to add a dictionary entry if the key doesn't exist, and replace the dictionary entry if the key exists.

You can accomplish this by using the ConcurrentDictionary's indexer without any additional locking:

deviceStatesCache[guid] = deviceState;
Michael Liu
  • 52,147
  • 13
  • 117
  • 150
0

You can also use AddOrUpdate method of ConcurrentDictionary type to achieve this. something like below

deviceStatesCache.AddOrUpdate(guid, deviceState, (key, value) => deviceState);

Ramesh Kanjinghat
  • 721
  • 5
  • 6
  • Can you explain what should I put into `key` and `value` from your example? – Kamil Nov 04 '20 at 21:15
  • @Kamil: `key` and `value` are just arbitrary names for the lambda parameters, whose values in this case are unused. When ConcurrentDictionary invokes the lambda, it will pass the value of `guid` and the existing value of `deviceStatesCache[guid]`, respectively. – Michael Liu Nov 04 '20 at 21:42