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);
        }
    }
}

score 1 · Accepted Answer · answered Jan 29 '15 at 14:23

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.

answered Jan 29 '15 at 14:23

usr

168,620
35
240
369

Do you mean I should remove the 'currentValue = getFlag()' and replace it with just '_theFlag'? There is a reason I used the getter/setter rather than accessing the _theFlag directly (which would have been simpler). The class in question has two flags, used for separate purposes. So I need to know which of the two flags to use depending on who called the method. This leaves two options - I can pass the correct flag into the method (by reference, hence the getter/setter) or I can duplicate the method entirely and have each method use the correct flag. – JMc Jan 29 '15 at 14:34
You could pass the right flag using `ref myFlag`. Or, use a wrapper class around the flag. The wrapper would expose a field. – usr Jan 29 '15 at 15:05
I've tried both of those solutions, but you can't pass by reference into a Task. Also, with the wrapper (like a flag object with an int property) still won't let you change the value without copying locally). I'm struggling to come up with a way to do that. I'd ideally not have to copy/paste the entire method and just access the correct flag in each. There must be a way to do this. – JMc Jan 29 '15 at 15:09
You can ref-reference a field. class c { int field; }. ref new c().field. – usr Jan 29 '15 at 15:15
1

Thats brilliant - solves my problem exactly. Thanks for your help. I learned quite a lot from that. – JMc Jan 29 '15 at 15:23

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

1 Answers1