Concurrent acces to a static member in .NET

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)
               开发者_如何学JAVA OnUserAdded(null, userName);
        }
    }

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

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

---

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.

---

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.

---

Yes you do need to lock your code.

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

---

Yes. You need to lock the collection before you read or write to the collection, since multiple users are potentially being added from different threadpool workers. You should probably also do it on the count as well, though if you're not concerned with 100% accuracy that may not be an issue.

---

As per Lucero's answer, you need to lock onlineUsers. Also be careful what will clients of your class do with the onlineUsers returned from GetUsers(). I suggest you change your interface - for example use IEnumerable<string> GetUsers() and make sure the lock is used in its implementation. Something like this:

public static IEnumerable<string> GetUsers() {
    lock (...) {
        foreach (var element in onlineUsers)
            yield return element;
        // We need foreach, just "return onlineUsers" would release the lock too early!
    }
}

Note that this implementation can expose you to deadlocks if users try to call some other method of OnlineUsers that uses lock, while still iterating over the result of GetUsers().

---

That code it is not thread-safe per se.

I will not make any suggestions relative to your "design", since you didn't ask any. I'll assume you found good reasons for those static members and exposing your list's contents as you did.

However, if you want to make your code thread-safe, you should basically use a lock object to lock on, and wrap the contents of your methods with a lock statement:

    private readonly object syncObject = new object();

    void SomeMethod()
    {
        lock (this.syncObject)
        {
            // Work with your list here
        }
    }

Beware that those events being raised have the potential to hold the lock for an extended period of time, depending on what the delegates do. You could omit the lock from the NoOfOnlineUsers property while declaring your list as volatile. However, if you want the Count value to persist for as long as you are using it at a certain moment, use a lock there, as well.

As others suggested here, exposing your list directly, even with a lock, will still pose a "threat" on it's contents. I would go with returning a copy (and that should fit most purposes) as Mark Gravell advised.

Now, since you said you are using this in an ASP.NET environment, it is worth saying that all local and member variables, as well as their member variables, if any, are thread safe.