Concurrent acces to a static member in .NET

Question

I've a class that contains a static collection to store the logged-in users in an ASP.NET MVC application. I just want to know about the below code is thread-safe or not. Do I need to lock the code whenever I add or remove item to the onlineUsers collection.

public class OnlineUsers
{
    private static List<string> onlineUsers = new List<string>();
    public static EventHandler<string> OnUserAdded;
    public static EventHandler<string> OnUserRemoved;

    private OnlineUsers()
    {
    }

    static OnlineUsers()
    {
    }

    public static int NoOfOnlineUsers
    {
        get
        {
            return onlineUsers.Count;
        }
    }

    public static List<string> GetUsers()
    {
        return onlineUsers;
    }

    public static void AddUser(string userName)
    {
        if (!onlineUsers.Contains(userName))
        {
            onlineUsers.Add(userName);

            if (OnUserAdded != null)
                OnUserAdded(null, userName);
        }
    }

    public static void RemoveUser(string userName)
    {
        if (onlineUsers.Contains(userName))
        {
            onlineUsers.Remove(userName);

            if (OnUserRemoved != null)
                OnUserRemoved(null, userName);
        }
    }
}

Answer 1

That is absolutely not thread safe. Any time 2 threads are doing something (very common in a web application), chaos is possible - exceptions, or silent data loss.

Yes you need some kind of synchronization such as lock; and static is usually a very bad idea for data storage, IMO (unless treated very carefully and limited to things like configuration data).

Also - static events are notorious for a good way to keep object graphs alive unexpectedly. Treat those with caution too; if you subscribe once only, fine - but don't subscribe etc per request.

Also - it isn't just locking the operations, since this line:

return onlineUsers;

returns your list, now unprotected. all access to an item must be synchronized. Personally I'd return a copy, i.e.

lock(syncObj) {
    return onlineUsers.ToArray();
}

Finally, returning a .Count from such can be confusing - as it is not guaranteed to still be Count at any point. It is informational at that point in time only.

Answer 2

Yes, you need to lock the onlineUsers to make that code threadsafe.

A few notes:

• Using a HashSet<string> instead of the List<string> may be a good idea, since it is much more efficient for operations like this (Contains and Remove especially). This does not change anything on the locking requirements though.

• You can declare a class as "static" if it has only static members.

Answer 3

Yes you do need to lock your code.

 object padlock = new object
 public bool Contains(T item)
 {
    lock (padlock)
    {
        return items.Contains(item);
    }
 }