How is a null value getting into my concurrent dictionary?

Question

I have a simple connection manager service in my web app that keeps track of websocket connections against a user GUID they are associated with. These are stored in a dictionary with the key being a user GUID and the value being a list of connection IDs (both strings).

ConnectionsByUser = new ConcurrentDictionary<string, List<string>>();

I have a register connection function as below:

public void RegisterConnection(string userGuid, string connectionId)
{
    if (connectionId == null)
    {
        logger.LogError($"Null connection id found for user {userGuid}");
        throw new ArgumentNullException(nameof(connectionId));
    }

    ConnectionsByUser.AddOrUpdate(userGuid, new List<string>() { connectionId }, (key, value) =>
    {
        value.Add(connectionId);
        return value;
    });
}

I also have an unregister function:

public void UnregisterConnection(string connectionId)
{
    foreach (var user in ConnectionsByUser)
    {
        if (user.Value.Contains(connectionId))
        {
            user.Value.Remove(connectionId);
        }

        if (!user.Value.Any())
        {
            ConnectionsByUser.TryRemove(user.Key, out var _);
        }
    } 
}

Finally I have a Get function:

public List<string> GetConnectionIdsByUserGuid(string userGuid)
{
    var hasValue = ConnectionsByUser.TryGetValue(userGuid, out var connectionIds);

    if (!hasValue)
    {
        return new List<string>();
    }

    return connectionIds;
}

These are the only references to the ConnectionsByUser dictionary, yet somehow we have instances where GetConnectionIdsByUserGuid returns a list containing a null value mixed in with other actual valid connection IDs.

Considering that register function has a null check, I am clueless to how a null value gets into the list, at this point I assume that there is something about the way concurrent dictionary is implemented that I do not understand.

The `ConcurrentDictionary` is threadsafe, but the list you are updating in it IS NOT. — Matthew Watson, Jul 12 '22 at 10:40
Related: [Is AddOrUpdate thread safe in ConcurrentDictionary?](https://stackoverflow.com/questions/39156752/is-addorupdate-thread-safe-in-concurrentdictionary) — Theodor Zoulias, Jul 12 '22 at 12:30

score 0 · Answer 1 · answered Jul 12 '22 at 12:22

The ConcurrentDictionary<K,V> collection is safe when both the keys and the values are immutable. When the values are mutable, trying to make it safe becomes a pain in the neck, assuming that it's possible to do it at all.

My suggestion is to switch from a mutable List<T> to an immutable ImmutableHashSet<T>, as the container of each user's connections. Here is an example of how you could do it:

ConcurrentDictionary<string, ImmutableHashSet<string>> ConnectionsByUser = new();

public void RegisterConnection(string userGuid, string connectionId)
{
    ArgumentNullException.ThrowIfNull(userGuid);
    ArgumentNullException.ThrowIfNull(connectionId);

    ConnectionsByUser.AddOrUpdate(userGuid,
        (_, value) => ImmutableHashSet<string>.Empty.Add(value),
        (_, existingConnections, value) => existingConnections.Add(value),
        connectionId);
}

public bool UnregisterConnection(string userGuid, string connectionId)
{
    while (ConnectionsByUser.TryGetValue(userGuid, out var existingConnections))
    {
        var updatedConnections = existingConnections.Remove(connectionId);
        if (ReferenceEquals(updatedConnections, existingConnections))
        {
            return false; // Connection not found
        }
        else if (updatedConnections.IsEmpty)
        {
            if (ConnectionsByUser.TryRemove(
                KeyValuePair.Create(userGuid, existingConnections)))
                    return true; // Connection found and removed. User removed.
        }
        else
        {
            if (ConnectionsByUser.TryUpdate(
                userGuid, updatedConnections, existingConnections))
                    return true; // Connection found and removed.
        }
        // We lost the race to update the dictionary. Try again.
    }
    return false; // User not found
}

public IReadOnlyCollection<string> GetConnectionIdsByUserGuid(string userGuid)
{
    return ConnectionsByUser.TryGetValue(userGuid, out var connectionIds) ?
        connectionIds : ImmutableHashSet<string>.Empty;
}

I changed the signature of the UnregisterConnection method so that it has a userGuid parameter, otherwise searching blindly for a connectionId to remove will be very inefficient.

In the above example, all three methods (RegisterConnection, UnregisterConnection and GetConnectionIdsByUserGuid) are atomic. No possible sequence of events exists that could bring the dictionary in a corrupted state.

The GetConnectionIdsByUserGuid returns a snapshot of the user's connections that were stored in the dictionary, at the time that the method was called. The returned object does not represent a dynamically updated list of the connections, as they evolve over time.

How is a null value getting into my concurrent dictionary?

1 Answers1