Should I use Interlocked.Exchange here or is a standard write sufficient?

Question

The following method attempts to obtain a lock which is shared with some other thread. In the event that the lock can be obtained within a time period, some Action will be executed.

If the lock cannot be obtained within the specified time period, I want to try the whole process again on a separate thread by spawning a Task. I don't want my initial thread hanging around waiting to obtain a lock because performance is important in this scenario. However I do not want any more tasks to be created until this task completes. In other words there should never be more than a single task running at any given time. Any subsequent calls to 'ExecuteOrOffload' that are made while a Task is still running must not be allowed to create another Task.

I came up with the following solution and I would like to know if my setter method (setFlag) should use Interlocked.Exchange to change the flags value, or whether it makes any difference at all? The code runs on a machine with a large number of cores.

    void Main()
    {
        //I currently use this
        ExecuteOrOffload(_theLock, () => { }, () => _theFlag, newVal => _theFlag = newVal, 0);

        //Should I use this instead? i.e. - Does my setter need to use Interlocked.Exchange? 
        ExecuteOrOffload(_theLock, () => { }, () => _theFlag, newVal => Interlocked.Exchange(ref _theFlag, newVal));
    }

    private object _theLock = new Object();
    private int _theFlag = 0;


    //Flag = 0. Allow a new Task to start if needed
    //Flag = 1. A Task is already running. Don't allow new tasks to be created
    private void ExecuteOrOffload(object thelock, Action theAction, Func<int> getFlag, Action<int> setFlag, int timeout = 0)
    {
       bool acquiredLock = false;

       try
       {
        //If the lock can be obtained, execute the Action
        Monitor.TryEnter(thelock, TimeSpan.FromSeconds(timeout), ref acquiredLock);
        if (acquiredLock)
        {
            theAction();
        }
        //The lock was not obtained. Either offload to a new task or do nothing
        else
        {
            //Get the current value of the flag
            var currentValue = getFlag();
            //Try set the flag to 1. Only one thread should manage to do this.
            var originalValue = Interlocked.CompareExchange(ref currentValue, 1, 0); 

            //If this thread didn't change the flag then just return.
            if (originalValue == currentValue)
            {
                return;
            }

            //The thread that gets here changes the actual flag value
            setFlag(1);
            //...and starts a new task
            Task.Factory.StartNew(() =>
            {
                try
                {
                    //Retry the whole process from a Task. This time the flag is set to 1 and a new Task will not be created should the lock time out
                    ExecuteOrOffload(thelock, theAction, getFlag, setFlag, 2);
                }
                finally
                {
                    //Task completed (either timed out or executed the action, we don't care which). Reset the flag to allow future calls to start a new Task
                    setFlag(0);
                }
            });
        }
    }
    catch (Exception e)
    {
        //Log exception
    }
    finally
    {
        //If this thread took the lock, release it
        if (acquiredLock)
        {
            Monitor.Exit(thelock);
        }
    }
}

Answer 1

That get/setFlag pattern is completely racy. This is not safe. You're not doing any synchronization on the shared variable.

The Interlocked.CompareExchange you're doing is on an unshared local variable. That never helps. A smart JIT, which we don't have, would just optimize this interlocked operation out (or convert it to a fence).

Perform the Interlocked.CompareExchange on a shared variable. In that case there is no separate store and your question is resolved.