Acceptable method for modifying an item in a ConcurrentBag?

Question

Consider the following function, which iterates over the generic List<T>: Items, and changes a matching item if found:

void UpdateList(ref List<clsMyClass> Items, int idToFind) {
    foreach(var Item in Items) {
        if (Item.ID == idToFind)
        {   
            // modify the item
            Item.SomeIntCounter++;
            return;
        }
    }
}

Now, if I wanted to do the same thing, but this time using a thread-safe ConcurrentBag<T>, is this an acceptable method?...

void UpdateList(ref ConcurrentBag<clsMyClass> Items, int idToFind) {
    clsMyClass Item;
    bool found = false;
    ConcurrentBag<clsMyClass> tempItems = new ConcurrentBag<clsMyClass>();
    while(Items.Count > 0) {
        if (Items.TryTake(out Item))
        {
            if (Item.ID == idToFind)
            {
                //modify the item
                Item.SomeIntCounter++;
                found = true;
            }
            tempItems.Add(Item);
            if (found) break;
        }
    }
    foreach(var tempItem in tempItems) Items.Add(tempItem);
}

The idea here is that every item is removed from the ConcurrentBag and added to a temporary one until the matching item is found and changed, after this all the removed items are re-added to the ConcurrentBag.

Is this a sensible way to modify the collection in a thread safe way?

If your `ID` is unique, I would say a `ConcurrentDictionary` is more appropriate. And no, your second method is not thread-safe, since you are not using any locks, and are taking items, creating a temp bag, etc, all which can cause problems if it happens concurrently. — Maarten, Oct 01 '14 at 09:55
@Maarten Thanks - the ID is not always unique as unsaved entries have an id of 0, according to the classes used in the project. Also, I thought the Concurrent namespace used its own internal locking methods. My function also uses what I thought was the thread-safe method of removing an item from a ConcurrentBag: TryTake()? — Panjo, Oct 01 '14 at 10:14
If you want something to be called *thread-safe*, than **your operation as a whole** must be thread-safe, and the fact that you use a thread-safe method to remove an item is not enough. — Maarten, Oct 01 '14 at 10:34
Even if your manipulation of the collection itself is altered such that other threads using the collection won't break it you'll still have to deal with the fact that you're mutating the items that are accessed from multiple threads, and that's not safe as it sits. — Servy, Oct 01 '14 at 16:54

score -1 · Answer 1 · edited May 23 '17 at 12:05

Your concurrent version of UpdateList is not thread-safe because it introduces a race condition.

Your 1st version of UpdateList is not equivalent to your 2nd version in case of multi-threading. Do you see why? If you start two threads executing UpdateList one with idToFind_1 and the other with idToFind_2 working on the same ConcurrentBag<T> Items. Then the first thread might take out an item which the second one needs to update. Therefore there is a fair chance that an item with idToFind_2 will miss an update and vice versa. And here we have the race condition: If thread1 puts the item back timely it will get an update otherwise it won't.

Additionally, you will still have to deal with the fact that you're mutating the items that are accessed from multiple threads, and that's not safe as it is (Servy's comment).

Since the implementation is to some extent inefficient. Did you think about using another more suitable data structure and perhaps take care about synchronization by utilizing a lock which is used by any other block of code to provide exclusive access to the same instance of the data structure.

Moreover, since tempItems is local to UpdateList you don't need a thread-safe collection since there is no point in synchronization. So a simple List<T> would suffice as well.

You don't need the ref keyword for the parameters, see When to use ref and when it is not necessary in C#.

The OP's code is *not* safe. A `lock` is only helpful if every other block of code accessing this data also locks on the same instance. The object would *not* be passed by reference without the `ref` keyword. It would be passed by value, but that value would be a reference. This is a very important distinction. — Servy, Oct 01 '14 at 16:49

Acceptable method for modifying an item in a ConcurrentBag?

1 Answers1