HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

What is the potential risk with this code?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thistheriskwhatwithcodepotential

Problem

I am trying to implement a distinct counter to detect if all tasks that is added to the 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 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.