CODEWITHSUNDEEP
c#multithreadingthread-safety
善逸様
善逸様

Reputation: 11

Multithreaded access to a Dictionary

I've search across the web and I get a bit confused about multithreading (lock, Monitor.Enter, volatile etc.) So, instead of asking the solution here, I've tried something "homemade" about multithreading management and I would like to have your advices.

Here's my context :

-I have a static class that contains a static Dictionary<int,string>

-I have a lot of tasks (let's say 1000) reading in this Dictionary every seconds

-I have one another task, that will update this Dictionary every 10s.

Here's the code of the cache:

public static class Cache
{
    public static bool locked = false;
    public static Dictionary<int, string> Entries = new Dictionary<int, string>();
    public static Dictionary<int, string> TempEntries = new Dictionary<int, string>();

    // Called by 1000+ Tasks
    public static string GetStringByTaskId(int taskId)
    {
        string result;

        if (locked)
            TempEntries.TryGetValue(taskId, out result);
        else
            Entries.TryGetValue(taskId, out result);

        return result;
    }

    // Called by 1 task
    public static void UpdateEntries(List<int> taskIds)
    {
        TempEntries = new Dictionary<int, string>(Entries);

        locked = true;
        Entries.Clear();

        try
        {
            // Simulates database access
            Thread.Sleep(3000);

            foreach (int taskId in taskIds)
            {
                Entries.Add(taskId, $"task {taskId} : {DateTime.Now}");
            }
        }
        catch (Exception ex)
        {
            Log(ex);
        }
        finally
        {
            locked = false;
        }
    }
}

There's my questions :

The program runs but I don't understand why the twice 'locked' bool assignments in the UpdateEntries methods doesn't generate a multithreading exception since it's read by the other thread "everytime"

Is there a more conventional way to handle this, I feel like it's a weird way to do it?

Upvotes: 0

Views: 703

Answers (2)

Theodor Zoulias
Theodor Zoulias

Reputation: 43384

Using a non-volatile bool flag for thread synchronization is not thread-safe, and makes your code susceptible to race conditions and haisenbugs. The correct way to do it is to replace atomically the old dictionary with the new one, after the new one has been fully constructed, using a cross-thread publishing mechanism like the Volatile.Write or the Interlocked.Exchange methods. Your case is simple enough that you could also use the volatile keyword for brevity, like in the example below:

public static class Cache
{
    private static volatile ReadOnlyDictionary<int, string> _entries
        = new ReadOnlyDictionary<int, string>(new Dictionary<int, string>());

    public static IReadOnlyDictionary<int, string> Entries => _entries;

    // Called by 1000+ Tasks
    public static string GetStringByTaskId(int taskId)
    {
        _entries.TryGetValue(taskId, out var result);
        return result;
    }

    // Called by 1 task
    public static void UpdateEntries(List<int> taskIds)
    {
        Thread.Sleep(3000); // Simulate database access

        var temp = new Dictionary<int, string>();
        foreach (int taskId in taskIds)
        {
            temp.Add(taskId, $"task {taskId} : {DateTime.Now}");
        }
        _entries = new ReadOnlyDictionary<int, string>(temp);
    }
}

With this approach every access to the _entries field will incur the cost of volatility, which is typically less than 10 nsec per operation, so it shouldn't be a problem. It's a cost worth paying, because it guarantees the correctness of your program.

Upvotes: 1

John Wu
John Wu

Reputation: 52210

The conventional way to handle this is to use a ConcurrentDictionary. This class is thread-safe and designed for multiple threads to read and write to it. You still need to be aware of potential logic problems (e.g. if two keys must be added at the same time before other threads can see either of them), but it is going to be fine for most operations without additional locking.

Another way to handle this for your specific situation is to use an ordinary dictionary, but treat it as immutable once it is available to reader threads. This will be more efficient as it avoids locks.

public static void UpdateEntries(List<int> taskIds)
{
    //Other threads can't see this dictionary
    var transientDictionary = new Dictionary<int, string>();  

    foreach (int taskId in taskIds)
    {
        transientDictionary.Add(taskId, $"task {taskId} : {DateTime.Now}");
    }

    //Publish the new dictionary so other threads can see it
    TempEntries = transientDictionary; 
}

Once the dictionary is assigned to TempEntries (the only place other threads can access it), it is never modified, so the threading concerns go away.

Upvotes: 1

Related Questions