Why is this code not threadsafe?

Question

I have a piece of code like this:

public class UserCache
{
    private Dictionary<int, User> _users = new Dictionary<int, User>();

    public User GetUser(int id)
    {
        User u = null;

        lock (_users)
        {
            if (_users.containsKey(id))
                return _users[id];
        }

        //The below line is threadsafe, so no worries on that.
        u = RetrieveUser(id); // Method to retrieve from database;

        lock (_users)
        {
            _users.Add(id, u);
        }

        return u;
    }
}

I'm locking the access to dictionary, however someone in my team told that its still not threadsafe(without an explanation). Question is - do you think this is thread safe at all?

Edit: Forgot to ask,what a solution would look like.Please note that I'mnot looking to lock the whole method, as the retrieve user is a time consuming operation.

Answer 1

No, it's not thread-safe. Imagine it's called twice at the same time with the same ID, which isn't previously present.

Both threads would get as far as RetrieveUser, and they'd both call _users.Add(id, u). The second call would fail because the key would already exist in the dictionary.

(As an aside, I'd strongly recommend using braces for locks, if statements etc, for the sake of readability.)

Answer 2

It is threadsafe in the sense that it will not corrupt any data structures. It is not thread-safe in the sense that the entire method behaves atomically. Two threads might find the item to be missing, then create it then add it. One of the adders will fail.

Answer 3

Idon't think you have more choice here because the result of _users.Add(id,u) depend on RetrieveUser you have to lock all method to make it thread safe. may be john skeet can confirm this
the solution may look like this

public class UserCache
 {
  private Dictionary<int, User> _users = new Dictionary<int, User>();
 private readonly object _syncLock = new object(); 
 public User GetUser(int id)
{
   User u = null;

    lock (_syncLock)
    { 

        if (_users.containsKey(id))
            return _users[id];


    //The below line is threadsafe, so no worries on that.
    u = RetrieveUser(id); // Method to retrieve from database;


        _users.Add(id, u);
    }

    return u;
}

}

hope this help

Answer 4

I think you must apply the singleton pattern to have a really thread safe code. In this moment you can have 2 instances of your class.