Is this use of Parallel.ForEach() thread safe?

Question

Essentially, I am working with this:

var data = input.AsParallel(); List<String> output = new List<String>();  Parallel.ForEach<String>(data, line => {     String outputLine = "";      // ** Do something with "line" and store result in "outputLine" **      // Additionally, there are some this.Invoke statements for updating UI      output.Add(outputLine); }); 

Input is a List<String> object. The ForEach() statement does some processing on each value, updates the UI, and adds the result to the output List. Is there anything inherently wrong with this?

Notes:

• Output order is unimportant

Update:

Based on feedback I've gotten, I've added a manual lock to the output.Add statement, as well as to the UI updating code.

Answer 1

Yes; List<T> is not thread safe, so adding to it ad-hoc from arbitrary threads (quite possibly at the same time) is doomed. You should use a thread-safe list instead, or add locking manually. Or maybe there is a Parallel.ToList.

Also, if it matters: insertion order will not be guaranteed.

This version is safe, though:

var output = new string[data.Count];  Parallel.ForEach<String>(data, (line,state,index) => {     String outputLine = index.ToString();     // ** Do something with "line" and store result in "outputLine" **      // Additionally, there are some this.Invoke statements for updating UI     output[index] = outputLine; }); 

here we are using index to update a different array index per parallel call.

Answer 2

Is there anything inherently wrong with this?

Yes, everything. None of this is safe. Lists are not safe for updating on multiple threads concurrently, and you can't update the UI from any thread other than the UI thread.