Modifying ConcurrentBag objects from another thread

Question

I have a thread #1 which is looping continuously with foreach through a ConcurrentBag list and modifying the items, but at some time another thread #2 needs to modify an item from the list. Is it safe if I take out all the objects from thread #2 (while the other one is looping with foreach) and modify the object then add all items back in thread #2 ? if it's not then is there a workaround ?

ConcurrentBag<MyObject> list;

thread 1

while (1) // continuous loop
{
  foreach(MyObject obj in list)
  {
     obj.RunMethod();
     Thread.Sleep(500);
  }
  Thread.Sleep(1000);
}

thread 2

(at some point a callback is called on thread #2 and need to modify an object)

List<MyObject> temp;
while (list.TryTake(out obj)
{
   MyObject obj2=obj.Clone(); // make a copy of the object
   if (obj2.Id==4) obj2.variable=10;
   temp.Add(obj2);
}
// Add objects back to concurrentbag
foreach(MyObject obj in temp) list.Add(obj);

What problem are you trying to solve by doing so? Without more context it's hard to know whether it's safe/meaningful for two threads to attempt modifying the same object at the same time (but if you use ConcurrentBag you won't get any undefined behavior). — Dylan Nicholson, Nov 17 '17 at 08:43
ConcurrentBag internally creates a collection per thread, so that when you add items, you add to the collection belonging to the current thread. When you take objects out, it first empties your collection and then starts stealing objects from other threads' collections. It does the stealing in a thread-safe way, so yes, this should be safe. I don't think this is the solution you're looking for whatever problem you're solving: there will be better ways of doing it. Give us more information and we can advise what you should do. — ProgrammingLlama, Nov 17 '17 at 08:44
No, this isn't safe because the thread doing `foreach` may have retrieved a reference to an object and be working with that object while the other thread removes it from the bag, modifies it and puts it back. This could result in the object's state changing while the `foreach` thread is using it. You can avoid this by creating a copy of the object to be mutated, and change that and put it back into the bag. Also, have you considered making your object immutable? — Matthew Watson, Nov 17 '17 at 08:52
Given the comments, is there anything else you're wondering about now? It seems @MatthewWatson has answered (though in a comment) in that this is not safe to do, and since your question was explicitly "is it safe" then if Matthew posts an answer, is that enough? — Lasse V. Karlsen, Nov 17 '17 at 09:05
ok, but is there a workaround for this? to change the value of the object from another thread? — Mario, Nov 17 '17 at 09:07
@MarioM I put my comment into an answer - note that it does contain a workaround; you must make a copy of the object you removed from the bag, modify the copy and put the copy back into the bag. — Matthew Watson, Nov 17 '17 at 09:08
but that's what I did, a made a copy of the list in thread 2 then put it back in the bag. — Mario, Nov 17 '17 at 09:09
@MarioM Where are you making a copy of the object? I don't see any `obj.Clone()` or `obj.Copy()` or `new MyObjectj(obj)` sort of code. — Matthew Watson, Nov 17 '17 at 09:11
I made the copy in the while loop, take out each object from the bag and add to a temp list, then add it back — Mario, Nov 17 '17 at 09:13
@MarioM That's not copying the object, it's just copying a REFERENCE to the object. The copy of the reference still references the original object, so changing via the reference also changes the original. — Matthew Watson, Nov 17 '17 at 09:13

Matthew Watson · Accepted Answer · 2017-11-17T09:16:20.200

No, this isn't safe because the thread doing foreach may have retrieved a reference to an object and be working with that object while the other thread removes it from the bag, modifies it and puts it back.

This could result in the object's state changing while the foreach thread is using it. You can avoid this by creating a copy of the object to be mutated, and then change the copy and put the copy back into the bag.

Have you considered making your object immutable? Doing so will really simplify things, and actually makes it impossible for two threads to mutate it at the same time.

NOTE: Copying a reference to an object does NOT copy the contents of the object, so when you do list.TakeOut(out obj) you are receiving a reference to an existing object, not creating a new copy of it.

[EDIT] So you edited your question to use obj.Clone(). If that does a proper deep clone of your object, then the code should be OK, but that's only if you're careful to copy the object everywhere it is changed. If you make the class immutable, then you GUARANTEE this behaviour,so that's the best solution if you can use it.

Modifying ConcurrentBag objects from another thread

thread 1

thread 2

1 Answers1