lock vs boolean

Question

Here the scenario :

A method is called each minute by a timer. This method could be call through UI (a button). I want that if my method is "in process", and is called, it does not execute the method twice.

In my method I use a simple boolean :

    private bool _isProcessing;
    public void JustDoIt(Action a, int interval, int times)
    {
        if (!_isProcessing)
        {
            _isProcessing = true;
            for (int i = 0; i < times; i++)
            {
                a();
                Thread.Sleep(interval);
            }
        }
        _isProcessing = false;
    }

It works fine. I test this functionality with this test :

    [Test]
    public void Should_Output_A_String_Only_3_Times()
    {
        var consoleMock = new Mock<IConsole>();
        IConsole console = consoleMock.Object;
        var doer = new Doer { Console = console };

        Action a = new Action(() => console.Writeline("TASK DONE !"));

        // Simulate a call by Timer
        var taskA = Task.Factory.StartNew(() => doer.JustDoIt(a, 1000, 3));

        // Simulate a call by UI
        var taskB = Task.Factory.StartNew(() => doer.JustDoIt(a));

        taskA.Wait();
        consoleMock.Verify(c => c.Writeline("TASK DONE !"), Times.Exactly(3));
    }

A developer reviews my code and says : "I replaced your boolean by a lock keyword. It's more Thread Safe. Frankly I'm not masterize multithreading so I answered him "OK Guy !"

few days later (today to be more precise), I want to test what if the difference between using lock or a simple boolean. So I was surprised to constate when I replace a boolean by the lock keyword like this :

    private object _locker = new Object();
    public void JustDoIt(Action a, int interval, int times)
    {
        lock (_locker)
        {
            //_isProcessing = true;
            for (int i = 0; i < times; i++)
            {
                a();
                Thread.Sleep(interval);
            }
        }
        //_isProcessing = false;
    }

The precedent test don't pass :

Message : Moq.MockException : Expected invocation on the mock exactly 3 times, but was 4 times: c=>c.Writeline("TASK DONE !")

So, do I use the lock keyword badly ? Should it be 'Static' ?

Thank you

Answer 1

Make _isProcessing volatile. And then do this:

public void JustDoIt(Action a, int interval, int times)
{
    if (_isProcessing) return
    _isProcessing = true;
    for (int i = 0; i < times; i++)
    {
        a();
        Thread.Sleep(interval);
    }
    _isProcessing = false;
}

This has a minor race condition, but since your code isn't synchronized to anything anyway, I don't believe it can possibly matter.