C# list concurrency

Question

I'm working on a simple irc chat bot (specifically for twitch.tv streams), and I'm using a List to keep a list of all the users in the channel. When someone leaves or joins, I add or remove them from the list. Then I have a thread that runs every minute that checks if the stream is online, and if it is, it hands out "currency" to all the people in my user list.

I'm sure you can already see where my problem is. If someone leaves or joins while my program is looping through the users in my list, then I get a Collection Modified exception. Currently, as a workaround, I just make a temp list and copy the real list into it, then loop through the temp list instead, but I was just curious if there was a "better" way to do it?

Quick psuedocode:

private List<string> users = new List<string>();

private void IrcInitialize(){
    //connect to irc stuff
    //blah
    //blah
    //blah
    Thread workThread = new Thread(new ThreadStart(doWork());
    workThread.Start();
}

private void ircListener(){
    parseIRCMessage(StreamReader.ReadLine());
}

private void parseIRCMessage(msg){
    if (msgType == "JOIN"){
        users.Add(user);
    }
    else if (msgType == "PART"){
        users.Remove(user);
    }
}

private void doWork(){
    while (true) {
        if (streamOnline() && handOutTime()){
            handOutCurrency();
        }
        Thread.Sleep(60000);
    }
}

private void handOutCurrency(){
    List<string> temp = users; //This is what I'm currently doing
    foreach (String user in temp) {
        database.AddCurrency(user, 1);
    }
}

Any other suggestions?

Answer 1

I suggest using a ConcurrentBag<string> for the users.

This allows multi-threaded access to the users even while it is being enumerated.

The big plus is that you do not have to worry about locking.

Answer 2

There are two ways to solve this problem:

• Using a lock to synchronize access between two threads, or
• Doing all access from a single thread.

The first way is simple: add lock(users) {...} block around the code that reads or modifies the users list.

The second way is slightly more involved: define two concurrent queues, toAdd and toRemove in your class. Instead of adding or removing users directly from the users list, add them to the toAdd and toRemove queues. When the sleeping thread wakes up, it should first empty both queues, performing the modifications as necessary. Only then it should hand out the currency.

ConcurrentQueue<string> toAdd = new ConcurrentQueue<string>();
ConcurrentQueue<string> toRemove = new ConcurrentQueue<string>();

private void parseIRCMessage(msg){
    if (msgType == "JOIN"){
        toAdd.Enqueue(user);
    }
    else if (msgType == "PART"){
        toRemove.Enqueue(user);
    }
}
private void doWork(){
    while (true) {
        string user;
        while (toAdd.TryDequeue(out user)) {
            users.Add(user);
        }
        while (toRemove.TryDequeue(out user)) {
            users.Remove(user);
        }
        if (streamOnline() && handOutTime()){
            handOutCurrency();
        }
        Thread.Sleep(60000);
    }
}

Answer 3

The suggestions from dasblinkenlight's answer are good. Another option is to do something similar to what you suggested: work with a immutable copy of the list. Except with normal List, you would need to make sure that it's not changed while you're copying it (and you would actually need to copy the list, not just a reference to it, like your code suggested).

A better version of this approach would be to use ImmutableList from the immutable collections library. With that, each modification creates a new collection (but sharing most parts with the previous version to improve efficiency). This way, you could have one thread that modifies the list (actually, creates new lists based on the old one) and you could also read the list from another thread at the same time. This will work, because new changes won't be reflected in an old copy of the list.

With that, your code would look something like this:

private ImmutableList<string> users = ImmutableList<string>.Empty;

private void ParseIRCMessage(string msg)
{
    if (msgType == "JOIN")
    {
        users = users.Add(user);
    }
    else if (msgType == "PART")
    {
        users = users.Remove(user);
    }
}

private void HandOutCurrency()
{
    foreach (String user in users)
    {
        database.AddCurrency(user, 1);
    }
}

Answer 4

You need to lock on the list during all reads, writes, and iterations of the list.

private void parseIRCMessage(msg){
  lock(users)
  {
    if (msgType == "JOIN"){
        users.Add(user);
    }
    else if (msgType == "PART"){
        users.Remove(user);
    }
  }
}

private void doWork(){
    while (true) {
        if (streamOnline() && handOutTime()){
            handOutCurrency();
        }
    Thread.Sleep(60000);
    }
}

private void handOutCurrency(){
  lock(users)
  {
    foreach (String user in users) {
        database.AddCurrency(user, 1);
    }
  }
}

etc...