Call Action every X cycles

Question

I want to call a Action every X cycles (in my case: to a transaction commit every 100 records). So i do:

public class CyclicActionCaller
{
    private int _counter;
    private int _cycleValue;
    private Action _methodToCall;

    public CyclicActionCaller()
    {
        _counter = 0;            
    }

    public CyclicActionCaller Every(int cycleValue)
    {
        _cycleValue = cycleValue;
        return this;
    }

    public CyclicActionCaller Call(Action methodToCall)
    {
        _methodToCall = methodToCall;
        return this;
    }

    public void PerformCall()
    {
        Interlocked.Increment(ref _counter);
        // IS THIS THREAD SAFE - SHOULD BE CALLED EVERY _counter % _cycleValue - EVEN IN MULTITHREADING ENVIRONMENT
        if (_counter % _cycleValue == 0)
        {
            _methodToCall();                
        }
    }
}

CyclicActionCaller cyclicActionCaller = new CyclicActionCaller();
using (ModelContainer container = new ModelContainer())
{
   cyclicActionCaller.Every(100).Call(() => { container.SaveChanges(); });

   while(...) { // Import Loop - assume multiple workers inside
       cyclicActionCaller.PerformCall(); // every worker calls this after inserting a row
   }
}

Is this function thread safe? I want that - even if multiple threads call PerformCall - that it is just called once every 100 calls

Best regards

Answer 1

No, it's not thread-safe. Between the call to Interlocked.Increment() and the if check, another thread could call Interlocked.Increment(). That means both of them could see that _counter is 101, which means a call to _methodToCall would be skipped.

But this is exactly why Interlocked.Increment() returns the incremented value. So the thread-safe version would be:

public void PerformCall()
{
    if (Interlocked.Increment(ref _counter) % _cycleValue == 0)
        _methodToCall();                
}

This is exactly why writing code like this is hard (although this is a simple situation, so it's not that hard here). It's much easier to write correct multi-threaded code, if you lock all shared values.

Also, if you do something like this, you need to be sure that inserting a row in your code is also thread-safe. And the same applies to SaveChanges(). For example, if you used something like ConcurrentQueue in a straightforward way, it would be possible for SaveChanges() to save more than 100 items at a time, because new items could have been inserted between the call to Interlocked.Increment() and the final dequeue from the queue. This may not be a problem for you, but you should think about that.