calling Interlocked after Semaphore WaitOne

Question

Came across the following code which blocks on a Semaphore when GenerateLabel is called more than 4 times concurrently. After the WaitOne a member mCurrentScanner is used to get access to a scanner. The question is if the Interlocked functions are needed after the WaitOne? I'd say no as the thread starts fresh when the WaitHandle is released, but not 100% sure.

  mConcurrentLabels = new Semaphore(4, 4);

  public string GenerateLabel()
  {
    mConcurrentLabels.WaitOne();

    int current = 0;

    Interlocked.Exchange(ref current, mCurrentScanner);

    (scanner, dir) = ScanMappings[current];

    Interlocked.Increment(ref mCurrentScanner);
    mCurrentScanner %= 4;
    DoLongRunningTask();
    mConcurrentLabels.Release();
  }

Answer 1

Like you said; The semaphore is used to limit the concurrent threads. But the body is still executed concurrently. So locks/interlocked is required.

The bigger problem is: Using Interlocked.Exchange(ref current, mCurrentScanner); to read the value safely and using the Interlocked.Increment(ref mCurrentScanner);.

It might be possible to concurrent read the same value Exchange() and increment it twice. So you'll select one value twice and skip the next one.

I also advice to use try/finallies when using Semaphores.

mConcurrentLabels = new Semaphore(4, 4);

public string GenerateLabel()
{
    mConcurrentLabels.WaitOne();
    try
    {
        int current = Interlocked.Increment(ref mCurrentScanner);
        
        (scanner, dir) = ScanMappings[current];
        
        // mCurrentScanner %= 4;   <------ ?
        DoLongRunningTask();
    }
    finally
    {
        mConcurrentLabels.Release();
    }
}

---

But if you need to mod the mCurrentScanner, I wouldn't use Interlocked.

mConcurrentLabels = new Semaphore(4, 4);
object mSyncRoot = new object();

public string GenerateLabel()
{
    mConcurrentLabels.WaitOne();
    try
    {
        int current;
        
        lock(mSyncRoot)
        {
            current = mCurrentScanner++;
            mCurrentScanner %= 4;
        }
        
        (scanner, dir) = ScanMappings[current];
        
        // mCurrentScanner %= 4;   <------ ?
        DoLongRunningTask();
    }
    finally
    {
        mConcurrentLabels.Release();
    }
}