Is this the correct usage of ConcurrentDictionary<TKey, TValue>?

Question

I need to make a simple class that tracks how many clients are listening to a specific item, will be referred to as building hence forth. I will have a timer running that will check which buildings are active and send updates to them. While this is occurring, the client will be able connect and disconnect. This means they will also be modifying the building list as time goes on.

To make sure that I don't have any concurrency issues, I decided to make a simple class that uses a ConcurrencyDictionary<TKey, TValue>. I don't have a need of the class in the past and want to make sure it is being used correctly as the documentation seems to lack a bit for this class, mostly when it comes to updating values and the like.

/// <summary>
/// Tracks the buildings that are actively being listened to and the number of clients subscribed to the building
/// </summary>
public static class BuildingSubscriptionTracker
{
    /// <summary>
    /// A dictionary of building name and listening client count
    /// </summary>
    private static readonly ConcurrentDictionary<string, int> _buildingsSubscribedTo = new ConcurrentDictionary<string, int>();

    /// <summary>
    /// Get all the buildings that are actively being listened to by clients
    /// </summary>
    /// <returns>An <see cref="System.Collections.Generic.IEnumerable{T}"/> of string</returns>
    public static IEnumerable<string> GetBuildings()
    {
        return _buildingsSubscribedTo.Keys;
    }

    /// <summary>
    /// Get how many clients are listening to updates for this building
    /// </summary>
    /// <param name="buildingName">Name of building to get client listening count for</param>
    /// <returns>A count of how many clients are listening to updates for this building.  If the building is no longer being tracked, this will return 0</returns>
    public static int GetActiveClientCountFor(string buildingName)
    {
        int clientCount;

        if (_buildingsSubscribedTo.ContainsKey(buildingName))
            if (_buildingsSubscribedTo.TryGetValue(buildingName, out clientCount))
                return clientCount;

        return 0;
    }

    /// <summary>
    /// Increment the client count of the given building
    /// </summary>
    /// <param name="buildingName">Name of the building the client wants updates for</param>
    public static void IncrementClientCountFor(string buildingName)
    {
        //If the building doesn't exist, add it and +1 the count
        if (!_buildingsSubscribedTo.ContainsKey(buildingName))
            _buildingsSubscribedTo.TryAdd(buildingName, 0);

        int clientCount;

        _buildingsSubscribedTo.TryGetValue(buildingName, out clientCount);

        _buildingsSubscribedTo.TryUpdate(buildingName, clientCount++, clientCount);
    }

    /// <summary>
    /// Decrements the client count of the given building
    /// </summary>
    /// <param name="buildingName">Name of the building the client wants updates for</param>
    public static void DecrementClientCountFor(string buildingName)
    {
        //If the building doesn't exist, there isn't anything to do
        if (_buildingsSubscribedTo.ContainsKey(buildingName))
        {
            int clientCount;

            //Get how many clients are listening to building so we can properly update it
            if (_buildingsSubscribedTo.TryGetValue(buildingName, out clientCount))
            {
                var newClientCount = clientCount--;

                if (newClientCount < 1)
                {
                    int unusedValue;
                    _buildingsSubscribedTo.TryRemove(buildingName, out unusedValue);
                }
                else
                {
                    _buildingsSubscribedTo.TryUpdate(buildingName, newClientCount, clientCount);
                }
            }
        }
    }
}

Answer 1

There isn't a lot of point of checking ContainsKey. A thread could sneak in between the ContainsKey and Try* methods you are calling. The TryGetValue out parameter will be initialized to the default value if it can't find the key.

The GetActiveClientCountFor could become this.

public static int GetActiveClientCountFor(string buildingName)
{
    int clientCount;
    _buildingsSubscribedTo.TryGetValue(buildingName, out clientCount);
    return clientCount;
}

In the IncrementClientCountFor code the value is retrieved and then added to. If someone came in and updated the value the TryUpdate would fail as the count changed.

What should be used is the AddOrUpdate method. If the key doesn't exist it will be created and set to one. If it does exist it will call the update method. The update method could be called multiple times if the value changed before it could be set and will be set to the last good value.

public static void IncrementClientCountFor(string buildingName)
{
    _buildingsSubscribedTo.AddOrUpdate(buildingName, 1, (s, i) => i++);
}

Same problem with DecrementClientCountFo. The value is retrieved and checked if less than one. But again a thread could add to dictionary and the code would remove it from the dictionary with a positive count. To get an Atomic TryRemove I would create an extension method called TryRemove taken from http://blogs.msdn.com/b/pfxteam/archive/2011/04/02/10149222.aspx

public static class ConcurrentDictionaryExtensions
{
    //http://blogs.msdn.com/b/pfxteam/archive/2011/04/02/10149222.aspx
    public static bool TryRemove<TKey, TValue>(
        this ConcurrentDictionary<TKey, TValue> dictionary, TKey key, TValue value)
    {
        if (dictionary == null) throw new ArgumentNullException("dictionary");
        return ((ICollection<KeyValuePair<TKey, TValue>>) dictionary).Remove(
            new KeyValuePair<TKey, TValue>(key, value));
    }
}

Now change DecrementClientCountFor to use AddOrUpdate then the Atomic TryRemove

public static void DecrementClientCountFor(string buildingName)
{
    var buildings = _buildingsSubscribedTo.AddOrUpdate(buildingName, s => 0, (s, i) => i--);
    if (buildings < 1)
    {
        _buildingsSubscribedTo.TryRemove(buildingName, buildings);
    }
}

If the value was changed from underneath us after getting the value the TryRemove will fail. Which is ok since it could be positive or another thread could have removed the key already.