Why does simple Parallel.ForEach sometimes fail to add value to a string variable?

Question

I am trying to speed up a foreach loop which appends a string result from a method to a variable. It doesn't matter what order the result strings get appended to the variable so I thought about changing the foreach to a Parallel.ForEach. Running a simple test I found that this may cause one or more of the result strings to not get appended to the variable.

Here is the test I created.

private string GetMessageString()
{
    string message = string.Empty;
    List<int> listOfInts = new List<int>();
    listOfInts.Add(1);
    listOfInts.Add(2);
    listOfInts.Add(3);
    listOfInts.Add(4);
    listOfInts.Add(5);
    listOfInts.Add(6);
    listOfInts.Add(7);
    listOfInts.Add(8);
    listOfInts.Add(9);
    Parallel.ForEach(listOfInts, currentInt =>
    {
        message += currentInt.ToString();
    });
    return message;
}

Now if I run this multiple times the message variable will have all 9 numbers in the string most of the time, but occasionally the message variable will be missing one or more of the numbers.

For example message = "12436789" where the 5 is missing.

Is this a race condition? What is the best way to assure all numbers get appended before the Parallel.ForEach loop exits?

atomic operations are not guaranteed. "the best way to assure all numbers get appended" is not to use `Parallel.ForEach` here. there is no point to use concurrency here IMO . — xZ6a33YaYEfmv, Oct 02 '15 at 19:33
also, you'd better use `StringBuilder`. `string` in .net is immutable type. that means that every concatenation creates new instance. — xZ6a33YaYEfmv, Oct 02 '15 at 19:38
This is just a very simplified example to demonstrate the problem. Change the code to message += DoLongRunningProcess() and then there is an argument for using concurrency. — user588582, Oct 02 '15 at 19:42
okay, if it's a long running tasks then you'd better add results to `List` with `Add` wrapped in `lock`. then concatenate resulting list. more info [here](http://stackoverflow.com/a/6601832/183267) — xZ6a33YaYEfmv, Oct 02 '15 at 19:50
You cannot modify the object from different threads simultaneously. — Andrey Nasonov, Oct 02 '15 at 20:30

score 1 · Answer 1 · answered Oct 02 '15 at 22:24

Is this a race condition?

Yes, this line is not atomic. By default pretty much nothing is atomic or thread-safe. It is important to understand the rules before you start playing with fire (threads).

This particular code is not going to receive any speedup due to per-iteration overhead. Even if that overhead was zero this would not speed up because we would need to synchronize on message. So this is pointless and there is no simple way to make this go faster than serial.

Maybe you could make it faster by running some kind of aggregation tree... This is a bad example for you to start learning threads basically.

So a fix would be to use a lock:

object @lock = new object();
...
lock (@lock)
      message += currentInt.ToString();

Why does simple Parallel.ForEach sometimes fail to add value to a string variable?

1 Answers1