patterncsharpMinor
What is the potential risk with this code?
Viewed 0 times
thistheriskwhatwithcodepotential
Problem
I am trying to implement a distinct counter to detect if all tasks that is added to the
I am still using .net 3.5, code has been updated as below.
ThreadPool has completed and want to use it as a barrier. So, when a function that is added to the ThreadPool is completed, workDone will be called, and count will be decreased. The main thread will be locked by calling waitForALL() method until the count is cleared to 0. Is this implementation reasonable and what risk does it have from the concurrency perspective?I am still using .net 3.5, code has been updated as below.
public class BarrierCount
{
private static volatile BarrierCount INSTANCE = new BarrierCount();
private int count = 0;
private static volatile Object o = new Object();
public static BarrierCount getInstance()
{
return INSTANCE;
}
private BarrierCount() { }
public void increaseCount()
{
lock (o)
{
count = count + 1;
}
}
public void workDone()
{
lock (o)
{
count = count - 1;
}
}
public void WaitForAll()
{
while(true)
{
lock (o)
{
if (count == 0)
{
break;
}
}
Thread.SpinWait(100);
}
}
}Solution
Update
OP has since updated question so this answer makes no sense any more WRT question asked!! Those edits have also been lost in migration from stack overflow it seems.
Purely from a concurrency perspective and not commenting on if the design is fit for purpose:
Newly added Volatile
You do not understand this, I am not going to go into why, just read up on stack overflow/MSDN or don't use it. As I remember you had a
In general don't
Instead add a specific lock object:
Note
In
I'm not sure how this is used, the
I would loose
But
You should look at Task.WaitAll before you do more on this.
OP has since updated question so this answer makes no sense any more WRT question asked!! Those edits have also been lost in migration from stack overflow it seems.
Purely from a concurrency perspective and not commenting on if the design is fit for purpose:
Newly added Volatile
You do not understand this, I am not going to go into why, just read up on stack overflow/MSDN or don't use it. As I remember you had a
readonly on the instance. That was correct.In general don't
lock(this), because (although not in this case... yet) this can also be locked from external code (lock(BarrierCount.getInstance())), and that allows deadlocks to occur. Although no deadlock is possible here as the methods are guaranteed to return.Instead add a specific lock object:
private readonly object _lock = new object();Note
readonly not volatile. readonly makes it clear to readers/modifiers that the reference is safe to use from multiple threads. It also stops modifiers from changing the reference (unless they remove the readonly, but they should make them think twice at least).In
WaitForAll you read count without a lock. Suggest you add a private GetCount() which locks.I'm not sure how this is used, the
setCount is presumably called once before the work commences. If the tasks have started and there is any risk in them calling workDone before setCount then it won't ever reach zero.I would loose
setCount in favour of an IncrementCount() method called before starting each task.But
You should look at Task.WaitAll before you do more on this.
Code Snippets
private readonly object _lock = new object();Context
StackExchange Code Review Q#32823, answer score: 4
Revisions (0)
No revisions yet.