What is the correct approach when trying to remove some items in ConcurrentDictionary?

Question

Is this better:

public void Test()
{
    ConcurrentDictionary<int, string> dictionary = new();

    dictionary.TryAdd(0, "A");
    dictionary.TryAdd(1, "B");
    dictionary.TryAdd(2, "A");
    dictionary.TryAdd(3, "D");

    foreach (var item in dictionary)
    {
        string foundItem;

        if (dictionary.TryGetValue(item.Key, out foundItem))
        {
            if (foundItem == "A")
            {
                if (dictionary.TryRemove(item.Key, out foundItem))
                {
                    // Success
                }
            }
        }
    }
}

Than this?:

public void Test2()
{
    ConcurrentDictionary<int, string> dictionary = new();

    dictionary.TryAdd(0, "A");
    dictionary.TryAdd(1, "B");
    dictionary.TryAdd(2, "A");
    dictionary.TryAdd(3, "D");

    foreach (var item in dictionary)
    {
        string foundItem;

        if (item.Value == "A")
        {
            if (dictionary.TryRemove(item.Key, out foundItem))
            {
                // Success
            }
        }
    }
}

This method will be accessed by multiple thread.

My confusion is, whenever I want to remove an item, I try to get it first, then remove it. But in the first place, I have used foreach loop, meaning I have already get the item. Any idea would be appreciated.

First option doesn't makes sense. When you already have an item with you, why would you call TryGetValue :? — Sriram Sakthivel
– Sriram Sakthivel, Commented Mar 18, 2015 at 14:23
I use TryGetValue because I'm thinking may be I may get an outdated value... :) — fiberOptics
– fiberOptics, Commented Mar 18, 2015 at 14:25
Well what if some thread modified the value after you called TryGetValue but before you call TryRemove ? Still there is a race :) — Sriram Sakthivel
– Sriram Sakthivel, Commented Mar 18, 2015 at 14:26
I thought TryGeValue will block other methods like TryRemove. Isn't like that? — fiberOptics
– fiberOptics, Commented Mar 18, 2015 at 14:27
This seems like a very bad idea. You can write the code so it won't crash, but what actually happens isn't going to be deterministic at all. You can make so very few assumptions about what's actually going to happen when you run it that it's unlikely to be particularly useful, and it's highly likely to end up causing some bugs somewhere in your application. ConcurrentDictionary is designed to be used such that you're only ever performing one method at a time, not many operations in aggregate. You're probably better off just using a regular dictionary and locking around access to it. — Servy
– Servy, Commented Mar 18, 2015 at 14:53

Jon Skeet · Accepted Answer · 2015-03-18 14:23:08Z

4

I don't see any benefit in the first approach. I'd just use LINQ to find the items though:

foreach (var entry in dictionary.Where(e => e.Value == "A"))
{
    string ignored;
    // Do you actually need to check the return value?
    dictionary.TryRemove(entry.Key, out ignored);
}

Of course, you need to consider what you want to happen if another thread adds a new entry with value "A" or updates an existing entry (possibly to make the value "A", possibly to make the value not-"A" while you're iterating... does it matter to you whether or not that entry is removed? It's not guaranteed what will happen. (The iterator doesn't take a snapshot, but isn't guaranteed to return entirely up-to-date data either.)

You may want to check that the value you've removed really is "A" by checking the variable I've called ignored afterwards. It really depends on your context. When you've got multiple threads modifying the map, you need to think that anything can happen at any time - within the operations that your code actually performs.

There's also the fact that you're effectively having to trawl through the whole dictionary... are you looking up by key elsewhere?

answered Mar 18, 2015 at 14:23

Jon Skeet

1.5m893 gold badges9.3k silver badges9.3k bronze badges

Sign up to request clarification or add additional context in comments.

14 Comments

fiberOptics Over a year ago

That's a big relief for me Jon.. I think I misunderstood the locking mechanism of this collection.

fiberOptics Over a year ago

Hi Jon, if I do, dictionary.TryGetValue(entry.Key, out ignored); and then check if ignored == "A" and then perform dictionary.TryRemove(entry.Key, out ignored); will that ensure I am not getting an outdated value? If NO, then how can I make sure that I'm getting the latest value?

Jon Skeet Over a year ago

@fiberOptics: No, it won't - because something could still change the value after the TryGet but before the TryRemove. As per my answer, if you want to check that the entry you removed had a value of "A", use the out parameter for TryRemove.

fiberOptics Over a year ago

I think that is the only option. Thanks Jon.

Servy Over a year ago

@JonSkeet Of course, that begs the question of what you should actually do if you find that you removed an item with a value other than "A"? Do you add it back in again? What if someone else tried to add/update the value after you removed it and before you added it back, or what if someone tried to actually remove it (like, for real) after you removed it and before you add it back in?

|

Juan · Accepted Answer · 2015-03-18 14:22:37Z

0

Second approach sounds good. If you have multiple threads trying to remove the item some of them will fail in the TryRemove but one will succeed, which should be good.

answered Mar 18, 2015 at 14:22

Juan

3,70323 silver badges34 bronze badges

Collectives™ on Stack Overflow

What is the correct approach when trying to remove some items in ConcurrentDictionary?

2 Answers 2

14 Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

14 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related