Lock around dictionary access in a WPF application

Question

I'm working on an old and large WPF application. The customer reported a bug, which they were able to reproduce, but I can't. There is a class in the application that looks like this:

public static class PermissionProvider
{
    private static Dictionary<string, bool> Permissions;

    public static void Init()
    {
        Permissions = new Dictionary<string, bool>();
    }
    
    private static object _lock = new object();
    public static bool HasPermission(string permission)
    {
        if (string.IsNullOrEmpty(permission)) return false;

        lock (_lock)
        {
            if (Permissions.ContainsKey(permission)) return Permissions[permission];
            var hasPermission = true; // Expensive call a third party module to check user permissions.
            Permissions.Add(permission, hasPermission);
            return hasPermission;
        }
    }
}

According to the log files provided by the customer, the line Permissions.Add(permission, hasPermission) threw an ArgumentException (key already exists). This doesn't make sense to me; the code checks for the key inside the same lock.

Based on a test run, all calls to HasPermission seem to be made from the main thread. The program uses Dispatcher.BeginInvoke at places, but my understanding is that locking is not even necessary for that. The dictionary is private and not accessed from anywhere else.

In what situation could this exception happen?

---

My first thought was that the customer was running an old version of the application, but it turns out that this class was only added in the latest one.

This particular exception should be easy enough to avoid by just changing the Permissions.Add(permission, hasPermission) to Permissions[permission] = hasPermission, but I would prefer to understand why it happened first.

Answer 1

It is possible, but hard to tell without the whole source code.

The expensive call

var hasPermission = true; // Expensive call a third party module to check user permissions.

could do something that calls HasPermission() again. Thus, the same thread would enter

lock (_lock) { ... }

again (which is allowed), possibly adding the the permissing, leaving the lock, leaving the method and returning into HasPermission() where it came from, adding the same key again.

This might either require production debugging at your customer. If you're not familiar with that and you can convince your customer to replace the affected DLL for a moment (let him create a backup copy), you could try the following:

lock (_lock) 
{
    var stack = Environment.StackTrace;
    if (stack.Split(new []{nameof(HasPermission)}, StringSplitOptions.None).Length> 2) throw new Exception("I should not be in here twice");
    ...
}

This should crash the application (unless general catch block somewhere) with a call stack that has the affected method twice, thus you can analyze where the second call comes from. Do whatever you would do in such a case: generate a crash dump, analyze your logs, ...

Generating a stack trace is considerably expensive, so this will change timing and thus potentially make the problem disappear. A disappeared problem is not a fixed problem, though.

Answer 2

I agree with Thomas Weller that the most likely reason is that the same thread reenter the lock for some reason. But i wanted to suggest another approach to these kinds of problems.

Holding a lock while calling arbitrary code can be dangerous, it may lead to deadlocks and various other issues. To limit such risks it is a good idea to only hold locks for short sections of code, only call code you know is safe, and does not raise events or can run arbitrary code some other way.

One option would be to switch to a 'publication only' model for thread safety that releases the lock while calling the 'expensive method'. This might allow multiple threads to call the expensive method at the same time, and this might or might not be an issue in your particular case. Something like:

lock (_lock)
{
     if (Permissions.ContainsKey(permission)) return Permissions[permission];
}
var hasPermission = true; // Expensive call a third party module to check user permissions.
lock (_lock)
{
     if (Permissions.ContainsKey(permission)) return Permissions[permission];
     Permissions.Add(permission, hasPermission);
     return hasPermission;
}

Or use ConcurrentDictionary.GetOrAdd that does more or less the same thing.

I would also caution against mutable global state in general since this can also make code hard to read and predict.

Answer 3

As pointed out by JonasH in a comment, the Init method looks highly suspicious. Your program could crash if this method is not called exactly once. If you are not sure how many times it's called, at least protect the code it contains with the same lock.

public static void Init()
{
    lock (_lock)
    {
        Permissions = new Dictionary<string, bool>();
    }
}