How may I lock an element of a ConcurrentDictionary?

Question

I have a ConcurrentDictionary stoing Items:

ConcurrentDictionary<ulong, Item> items;

Now I would like to lock on an Item from this dictionary so I may safely operate on it.

Is this code correct?

try
{
    Item item;
    lock(item = items[itemId])
    {
        // do stuff
        // possibly remove itemId from the dictionary
    }
}
catch(KeyNotFoundException)
{
    // ...
}

My fear is this. I suppose lock(item = items[itemId]) can be decomposed into two operations:

Assign reference of items[itemId] to item
Lock on item

Which are not necessarily atomic.

So, I'm fearing the following race condition:

Thread 1 performs item = items[itemId], but doesn't yet perform lock(item)
Thread 2 performs lock(item = items[itemId]) for the same value of itemId
Thread 2 erases itemId from items
Thread 2 releases its lock
Thread 1 performs lock(item), not knowing itemId is no longer in the dictionary
Thread 1 incorrectly operates on item, instead of going to its catch block, as it should.

Is the above analysis correct?

In that case, would it be sufficient to modify my code this way?

try
{
    Item item;
    lock(items[itemId])
    {
        item = items[itemId];
        // do stuff
        // possibly remove itemId from the dictionary
    }
}
catch(KeyNotFoundException)
{
    // ...
}

EDIT: Because I'm starting to suppose I've fallen for the XY problem. Here's the background.

Multiplayer chess game. itemId is the game's ID. item is the game's current state. The dict holds ongoing items. The operation is to process a player's move, like "knight from e3 goes to d1". If because of a player's move the game completes, then we return the final state of the game and remove the game from the dictionary. It is, of course, invalid to perform any further moves on a completed game, hence the try/catch block.

The try/catch block is supposed to correctly detect the following situations:

a player sends an invalid command, ordering to make a move in a non-existent game;
Because of network lag, a players command to make a move on a valid game arrives to the server after their timer run out.

Isnt the whole point of `ConcurrentDictionary` is that you _dont_ need lock statements? — maccettura, May 24 '18 at 17:06
@maccettura AFAIK `ConcurrentDictionary` means I can atomically retrieve / add elements from / to this dictionary (so I don't have to lock the whole dict just to access or add an element). But if I want to operate on any element, I still need to lock this particular element. Am I wrong? — , May 24 '18 at 17:07
Not only retrieve but also add/remove items from list. - I would do the following use `TryRemove` check for return value and use the out paramter to do stuff with when you are done with your operations re-add it if necessary — Rand Random, May 24 '18 at 17:10
@RandRandom Not sure if I understand you well. With what you suggest, when thread 1 operates on an item, won't thread 2 incorrectly see the item is not in the dictionary, instead of correctly waiting till thread1 finishes? — , May 24 '18 at 17:26
Yes, that would happen - I thought its your goal to only do your operation on the item once and not for every call than consider `Item item; if (items.TryGetValue(itemId, out item) { lock(item) { //do your operation } }` ( and drop your `try … catch` ) — Rand Random, May 24 '18 at 17:38
@RandRandom For every call. Removing the item from the dict means that no further operations are permitted on this item; that is because typically, if the item is not in the dict, then semantically the item is no more. Until this happens, there may be an arbitrary number of operations on this item. — , May 24 '18 at 17:43
Since you are doing a chess game, how would it even be possible that 2 or more operations happen at the same time on a single game object? Chess is a turn based game so only 1 player can interact with the game at any given time, so based on that information - I believe what you are doing is just not necessary. So, I more say your clients shouldnt be able to send operations to the server when it isnt their turn. — Rand Random, May 25 '18 at 08:53
Let us [continue this discussion in chat](https://chat.stackoverflow.com/rooms/171789/discussion-between-gaazkam-and-rand-random). — , May 25 '18 at 15:21
@RandRandom Quack, one misclick on my part and that's what happens. Chat, wtf. Anyway: (a) The clients are web browsers, I'm not in control of that ofc; (b) This is a slightly modified version of chess, two players move simultaneously, a turn only happens when both players issue their commands. — , May 25 '18 at 15:23

score 0 · Answer 1 · answered May 24 '18 at 17:56

Your updated code is no better. It's still possible for a thread to get the value from the dictionary, not lock yet, then have another thread remove the item before the lock gets taken out.

Fundamentally you want to have code be atomic that is something other than merely a single call to your ConcurrentDictionary, and as such you just need to do your own locking, and use a regular dictionary.

The main problem here stems from your attempts to lock on an object that may or may not be there, or that may be changing. That's just going to cause you all sorts of problems. One alternate solution would be to not do that, and ensure that the dictionary isn't changing or removing values for a key, so that you can safely lock on it. One way of doing that is by creating a wrapper object:

public static void Foo(ConcurrentDictionary<ulong, ItemWrapper> items, ulong itemId)
{
    if (!items.TryGetValue(itemId, out ItemWrapper wrapper))
    {
        //no item
    }
    lock (wrapper)
    {
        if (wrapper.Item == null)
        {
            //no actual item
        }
        else
        {
            if (ShouldRemoveItem(wrapper.Item))
            {
                wrapper.Item = null;
            }
        }
    }
}

How may I lock an element of a ConcurrentDictionary?

1 Answers1