Does using ConcurrentDictionary TryGetValue within an if statement make the if contents thread-safe?

Question

If I have a ConcurrentDictionary and use the TryGetValue within an if statement, does this make the if statement's contents thread safe? Or must you lock still within the if statement?

Example:

        ConcurrentDictionary<Guid, Client> m_Clients;

        Client client;
        //Does this if make the contents within it thread-safe? 
        if (m_Clients.TryGetValue(clientGUID, out client))
        {
            //Users is a list.
            client.Users.Add(item);
        }

or do I have to do:

        ConcurrentDictionary<Guid, Client> m_Clients;

        Client client;
        //Does this if make the contents within it thread-safe? 
        if (m_Clients.TryGetValue(clientGUID, out client))
        {
            lock (client)
            {
                //Users is a list.
                client.Users.Add(item);
            }
        }

Answer 1

Yes you have to lock inside the if statement the only guarantee you get from concurrent dictionary is that its methods are thread save.

Answer 2

The accepted answer could be misleading, depending on your point of view and the scope of thread safety you are trying to achieve. This answer is aimed at people who stumble on this question while learning about threading and concurrency:

It's true that locking on the output of the dictionary retrieval (the Client object) makes some of the code thread safe, but only the code that is accessing that retrieved object within the lock. In the example, it's possible that another thread removes that object from the dictionary after the current thread retrieves it. (Even though there are no statements between the retrieval and the lock, other threads can still execute in between.) Then, this code would add the Client object to the Users list even though it is no longer in the concurrent dictionary. That could cause an exception, synchronization, or race condition.

It depends on what the rest of the program is doing. But in the scenario I'm describing, it would be safer to put the lock around the entire dictionary retrieval. And then a regular dictionary might be faster and simpler than a concurrent dictionary, as long as you always lock on it while using it!

Answer 3

While both of the current answers are technically true I think that the potential exists for them to be a little misleading and they don't express ConcurrentDictionary's big strengths. Maybe the OP's original way of solving the problem with locks worked in that specific circumstance but this answer is aimed more generally towards people learning about ConcurrentDictionary for the first time.

Concurrent Dictionary is designed so that you don't have to use locks. It has several specialty methods designed around the idea that some other thread could modify the object in the dictionary while you're currently working on it. For a simple example, the TryUpdate method lets you check to see if a key's value has changed between when you got it and the moment that you're trying to update it. If the value that you've got matches the value currently in the ConcurrentDictionary you can update it and TryUpdate returns true. If not, TryUpdate returns false. The documentation for the TryUpdate method can make this a little confusing because it doesn't make it explicitly clear why there is a comparison value but that's the idea behind the comparison value. If you wanted to have a little more control around adding or updating, you could use one of the overloads of the AddOrUpdate method to either add a value for a key if it doesn't exist at the moment that you're trying to add it or update the value if some other thread has already added a value for the key that is specified. The context of whatever you're trying to do will dictate the appropriate method to use. The point is that, rather than locking, try taking a look at the specialty methods that ConcurrentDictionary provides and prefer those over trying to come up with your own locking solution.

In the case of OP's original question, I would suggest that instead of this:

    ConcurrentDictionary<Guid, Client> m_Clients;

    Client client;
    //Does this if make the contents within it thread-safe? 
    if (m_Clients.TryGetValue(clientGUID, out client))
    {
        //Users is a list.
        client.Users.Add(item);
    }

One might try the following instead*:

    ConcurrentDictionary<Guid, Client> m_Clients;

    Client originalClient;
    if(m_Clients.TryGetValue(clientGUID, out originalClient)
    {
        //The Client object will need to implement IEquatable if more
        //than an object instance comparison needs to be done.  This
        //sample code assumes that Client implements IEquatable.

        //If copying a Client is not trivial, you'll probably want to
        //also implement a simple type of copy in a method of the Client
        //object.  This sample code assumes that the Client object has
        //a ShallowCopy method to do this copy for simplicity's sake.
        Client modifiedClient = originalClient.ShallowCopy();

        //Make whatever modifications to modifiedClient that need to get
        //made...
        modifiedClient.Users.Add(item);
        
        //Now update the value in the ConcurrentDictionary
        if(!m_Clients.TryUpdate(clientGuid, modifiedClient, originalClient))
        {
            //Do something if the Client object was updated in between
            //when it was retrieved and when the code here tries to
            //modify it.
        }
    }

*Note in the example above, I'm using TryUpate for ease of demonstrating the concept. In practice, if you need to make sure that an object gets added if it doesn't exist or updated if it does, the AddOrUpdate method would be the ideal option because the method handles all of the looping required to check for add vs update and take the appropriate action.

It might seem like it's a little harder at first because it may be necessary to implement IEquatable and, depending on how instances of Client need to be copied, some sort of copying functionality but it pays off in the long run if you're working with ConcurrentDictionary and objects within it in any serious way.