Is this Dictionary-based UserCache implementation thread-safe?

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 thread-safe, 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 it's still not thread-safe (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'm not 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.)