Lock for ConcurrentDictionary when AddOrUpdate-ing?

Question

I use a ConcurrentDictioanry<string, HashSet<string>> to access some data across many threads.

I read in this article (scroll down) that the method AddOrUpdate is not executed in the lock, so it could endanger thread-safety.

My code is as follows:

//keys and bar are not the concern here
ConcurrentDictioanry<string, HashSet<string>> foo = new ...;
foreach(var key in keys) {
    foo.AddOrUpdate(key, new HashSet<string> { bar }, (key, val) => {
        val.Add(bar);
        return val;
    });
}

Should I enclose the AddOrUpdate call in a lock statement in order to be sure everything is thread-safe?

Jon Skeet · Accepted Answer · 2012-09-13T21:29:34.707

8

Locking during AddOrUpdate on its own wouldn't help - you'd still have to lock every time you read from the set.

If you're going to treat this collection as thread-safe, you really need the values to be thread-safe too. You need a ConcurrentSet, ideally. Now that doesn't exist within the framework (unless I've missed something) but you could probably create your own ConcurrentSet<T> which used a ConcurrentDictionary<T, int> (or whatever TValue you like) as its underlying data structure. Basically you'd ignore the value within the dictionary, and just treat the presence of the key as the important part.

You don't need to implement everything within ISet<T> - just the bits you actually need.

You'd then create a ConcurrentDictionary<string, ConcurrentSet<string>> in your application code, and you're away - no need for locking.

edited Sep 13 '12 at 21:29

answered Sep 13 '12 at 20:54

Jon Skeet

1,421,763
867
9,128
9,194

What if my instance of `ConcurrentDictionary` is accessed (read & write) through an API? So the client code won't actually notice that there's a `ConcurrentDictionary` then I'm safe doing the lock inside the public methods of my API, right? I'm just tryign to understand things here... – Paul Sep 14 '12 at 12:24
1

@Paul: Well yes, you *could* make sure you always lock whenever you access anything... but then what do you think ConcurrentDictionary is really buying you? – Jon Skeet Sep 14 '12 at 14:52
Please see my question, which is exactly about that: http://stackoverflow.com/questions/4306936/how-to-implement-concurrenthashset-in-net – Sebastian Nov 29 '13 at 23:06

score 4 · Answer 2 · answered Sep 13 '12 at 21:15

You'll need to fix this code, it creates a lot of garbage. You create a new HashSet even if none is required. Use the other overload, the one that accepts the valueFactory delegate. So the HashSet is only created when the key isn't yet present in the dictionary.

The valueFactory might be called multiple times if multiple threads concurrently try to add the same value of key and it is not present. Very low odds but not zero. Only one of these hashsets will be used. Not a problem, creating the HashSet has no side effects that could cause threading trouble, the extra copies just get garbage collected.

Thanks, fixed that by using the other overload. – Paul Sep 14 '12 at 12:25 — Paul, Sep 14 '12 at 12:25

score 0 · Answer 3 · answered Sep 13 '12 at 21:00

The article states that the add delegate is not executed in the dictionary's lock, and that the element you get might not be the element created in that thread by the add delegate. That's not a thread safety issue; the dictionary's state will be consistent and all callers will get the same instance, even if a different instance was created for each of them (and all but one get dropped).

eaglei22 · Answer 4 · 2017-06-06T13:51:10.457

0

Seems the better answer would be to use Lazy, per this article on the methods that pass in a delegate.

Also another good article Here on Lazy loading the add delegate.

edited Jun 06 '17 at 13:51

answered Jun 06 '17 at 13:44

eaglei22

2,589
1
38
53

Lock for ConcurrentDictionary when AddOrUpdate-ing?

4 Answers4