CODEWITHSUNDEEP
c#.netthread-safetyvolatile
baira
baira

Reputation: 59

C# Is it threadsafe to set dictionary object from one thread, get from other thread

Suppose we have some strange class which contains

Is it threadsafe to call method Update from one thread and get Map from other thread?

public class Example
{
    public ReadOnlyDictionary<string, string> Map{ get; private set; }

    public void Update(IEnumerable<KeyValuePair<string, string>> items)
    {
        Map = items
            .ToLookup(x => x.Key)
            .ToDictionary(x => x.Key, x => x.First())
            .ToReadOnlyDictionary(); // helper method
    }
}

Is it possible that there will be the moment when we call Map getter and it returns null or something in bad state, because of setter execution is not atomic?

Upvotes: 3

Views: 2850

Answers (4)

arekzyla
arekzyla

Reputation: 2898

To make your code simpler and less bug-prone you could use ImmutableDictionary:

public class Example
{
    public IReadOnlyDictionary<string, string> Map{ get; private set; } = 
        ImmutableDictionary.Create<string, string>();

    public void Update(IEnumerable<KeyValuePair<string, string>> items)
    {
        Map = items
            .ToImmutableDictionary();
    }
}

ImmutableDictionary is guaranteed to be thread safe so there is no way of breaking it. Usual Dictionary is not thread safe however in your implementation there should not be any thread safety issues but this requires this special usage.

ImmutableDictionary can be downloaded here https://www.nuget.org/packages/System.Collections.Immutable/

Another thread-safe dictionary is ConcurrentDictionary. Using it may not necessarily provide additional cost because read operation are lock-free. The docs states:

Read operations on the dictionary are performed in a lock-free manner.

So those two dictionaries are thread-safe in any case (on dictionary API level) and Dictionary is thread-safe in some implementations like yours.

Upvotes: 0

Burak OZEROGLU
Burak OZEROGLU

Reputation: 29

Concurrent Collections = Thread Safe :)

I think you should use Concurrent collection. Concurrent Generics make sense for me.

Upvotes: 0

Valery Petrov
Valery Petrov

Reputation: 673

because of setter execution is not atomic

The safety of this code is not about atomicity (read/write a reference is an atomic operation in .NET, unless you screw up alignment).
So, the only issue in this code is memory reordering. It might happen that another thread will get the reference to partially constructed object.
But it may happen only on hardware with weak memory model ('weak' means that a lot of reorderings are allowed and there is not many restrictions).
To prevent this you just have to publish this reference via Volatile.Write, which is nop for x86 and x86_64 architectures and a kind of memory fence for ARM and other.

Upvotes: 0

Evk
Evk

Reputation: 101443

"Thread safe" is a vague term. Wikipedia defines thread-safe code as:

Thread-safe code only manipulates shared data structures in a manner that ensures that all threads behave properly and fulfill their design specifications without unintended interaction

So we cannot say that certain piece of code is "thread safe" without knowing that design specification, which includes how this code is actually used.

This code is "thread-safe" in a sense that it's not possible for any thread to observe Example object in a corrupted or partial state.

Reference assignment is atomic, so thread that reads Map can only read either one vesrion of it or another, it cannot observe partial or intermediate state of Map. Because both keys and values of Map are immutable (strings) - the whole object is immutable and can safely be used by multiple threads after it was read from Map property.

However, doing something like this:

var example = GetExample();
if (example.Map.ContainsKey("key")) {
    var key = example.Map["key"];
}

Is of course not thread safe, because Map object, as a whole, might have changed between check and read from dictionary, in a way that old version contained "key" key, but new version does not. Doing this on the other hand:

var example = GetExample();
var map = example.Map;
if (map.ContainsKey("key")) {
    var key = map["key"];
}

is fine. Any code that does not rely on example.Map to stay the same between reads should be "safe".

So you have to ensure that given code is "thread safe" in your particular use case.

Note that using ConcurrentDictionary is absolutely useless here. Since your dictionary is readonly - it's already safe to read it from multiple threads.

Update: valid point raised in a comment is that ReadOnlyDictionary by itself does not guarantee that underlying data is not modified, because ReadOnlyDictionary is just a wrapper around regular mutable Dictionary.

However, in this case you create ReadOnlyDictionary in Update method from non-shared instance of dictionary (received from ToDictionary), which is not used for anything else except wrapping it in ReadOnlyDictionary. This is a safe usage, because there is no code which has access to mutable underlying dictionary except ReadOnlyDictionary itself, and read only dictionary prevents its modification. Reading from even regular Dictionary is thread safe as long as there are no writes.

Upvotes: 3

Related Questions