It seems ConcurrentBag is not Thread-Safe

Question

I had written a program that a list-builder method returns IEnumerable of string including lots of strings (1milion items) and I stored it in List of string , then it append all the items in a StringBuilder instance in a Parallel.Foreach. Then I printed stringBuilderInstance.Length .

The problem was that it was fewer than 1000000 . after some googling, I realized thet List collection is not Thread-Safe , and that causes this problem. So 2 solution crosses my mind :

1) Use lock

2) Use ConcurrentBag

When I am using lock , it is OK and the length is 1 milion , BUT :

when I am using ConcurrentBag of string , the length is fewer than I expected !

What is the underlying reason of this problem?

List-Creator method :

public static List<string> CreateList() { List<string> result = new List<string>(); for (int i = 0; i < 1000000; i++) { result.Add(1.ToString()); } return result; }

Using ConcurrentBag :

public static void DoWithParallel_ThreadSafe() { ConcurrentBag<string> listOfString = new ConcurrentBag<string>(CreateList()); StringBuilder a = new StringBuilder(); Action<string> appender = (number) => { a.Append(number); }; Parallel.ForEach(listOfString, appender); Console.WriteLine($"The string builder lenght : {a.Length}"); }

Using lock :

public static void DoWithParallel_UnsafeThread_Lock() { List<string> listOfString = CreateList(); StringBuilder a = new StringBuilder(); Action<string> appender = (number) => { lock (listOfString) { a.Append(number); } }; Parallel.ForEach(listOfString, appender); Console.WriteLine($"The string builder lenght : {a.Length}"); }

Main :

static void Main(string[] args) { DoWithParallel_UnsafeThread_Lock(); DoWithParallel_ThreadSafe(); Console.ReadKey(); }

Thank you in advance.

You need to learn more about threading before trying to write thread-safe code. — SLaks
– SLaks, Commented Jul 31, 2017 at 20:56
Just so you know Parallel.ForEach calls the IEnumerable you pass in to the first argument in a thread safe way, so it does not matter what collection you used to pass in for that argument. — Scott Chamberlain
– Scott Chamberlain, Commented Jul 31, 2017 at 21:17
@Scott Chamberlain : If Parallel.Foreach call the IEnumerable in a thread-safe way , why this problem happens ? — Parsa
– Parsa, Commented Aug 1, 2017 at 7:46
@Parsa Did you read the two answers at all? The problem is not caused by the IEnumerable, it is caused by StringBuilder. — Scott Chamberlain
– Scott Chamberlain, Commented Aug 1, 2017 at 12:56

Anil · Accepted Answer · 2018-06-26 08:29:25Z

StringBuilder cannot be mutated from multiple threads, hence why the code doesn't work when you try to do that. Note that locking is pointless; just don't create multiple threads to do the work in the first place since the work can't be done from multiple threads. Since you're never accessing the ConcurrentBag from multiple threads, so it's pointless to be using that instead of a List.

Aasmund Eldhuset · Accepted Answer · 2017-07-31 20:49:11Z

5

StringBuilder is not thread-safe, which is why some Append() calls are "lost". So you still need the lock, even if your collection is thread-safe.

(Also, see Servy's answer for why you don't need the collection to be thread-safe at all.)

answered Jul 31, 2017 at 20:49

Aasmund Eldhuset

38.1k4 gold badges74 silver badges85 bronze badges

3 Comments

Servy Over a year ago

You shouldn't be locking here. The correct solution is to just not create multiple threads, rather than to create a bunch of threads only to synchronize all of them.

Parsa Over a year ago

But when I use string instead of stringbuilder , it gives me the wrong answer too , the string is thread-safe. am I correct ?

SLaks Over a year ago

@Parsa: String is immutable; your variable is not thread-safe.

Collectives™ on Stack Overflow

It seems ConcurrentBag is not Thread-Safe

2 Answers 2

Comments

3 Comments

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

3 Comments

Linked

Related