Synced/Atomic access

Question

Forward

I would love any comments you have, any ideas, any flaws you can find, and any suggestions you might have regarding the code below. If this is similar to other implementations, I would love to know about them, since this is something I came up with by myself, after not finding any solution 'out there', but I know there is nothing new here, just my way of doing things.

Also, this is my first time posting here, so please go easy on me if I did it wrong.

Description

Writing code for windows WinForms application, usually entails working asynchronously, and sometimes throwing some threads in the air, so that the UI will still be responsive, IO will happen in the background, data will stay fresh, and your users will love instead of hate you for the bugs you throw at them.

C# has the 'lock' keyword to help with creating Atomic operations, but what I find frustrating, is that most often then not, the data itself need to be guarded, but the sync syntax can be easily forgotten.

So I've created the following code for myself to manage object locking, via generics and extension methods, which allow me to easily wrap any data type I need to be kept safe, and use it only via the Synced context.

First the extension methods (or rather a single extension method with overloaded parameter signature to make it's use more natural) DoLocked.

DoLocked will perform a locked action, after verifying that the object is 'ready for work', with the option to 'wait' (or not) on the lock, will release the lock when the action has completed (or in case of an exception), and will let you know if the action was triggered or not.

So basically any object, for ex. ComplexClass complexObject, you want to performed an operation that is locked (with all the features mentioned above), the code will look something similar to:

complexObject.DoLocked(syncObj,
() => { if( isReady ) return true; },
true,
obj => {
    ...
    obj.Method();
    ...
    obj.Property = value;
    ...
};

the getIsReady is a Func which tests readiness to enter the read (before even attempting to wait for the lock), when null, it is 'always ready'.

Next step is the Synced generic class, which will hold the syncObject along with the content (now locked) object. The idea is not to abstract the internal object, but wrap it in a package that makes it clear how to use it right.

The Code:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading;

namespace IdleSharp.Sys.ThreadSafe
{
    public static class SyncedDoExt
    {
        private static readonly Random random = new Random();
        private static bool alwaysReady() { return true; }

        // A note about the parameter order.
        // Usually, it is preferred to have the most common param in the first (left most) postion.
        // However, since the common argument is an action, which will likely be an in line lambda
        // expression more often then not, I have decided to place the action always last.
        // This is to make calls to DoLocked with multi-lines lambda inline methods more readable.

        public static bool DoLocked<T>(
            this T subject, object syncObj,
            Func<bool> getIsReady, bool wait, Action<T> action
            ) where T : class
        {
            System.Diagnostics.Trace.Assert(
                null != syncObj, "SyncObject must not be null!!!");
            bool lockWasTaken = false;
            try
            {
                Monitor.TryEnter(syncObj,
                wait ? Timeout.Infinite : 0, ref lockWasTaken);
                if (!lockWasTaken) return false;

                getIsReady = getIsReady ?? alwaysReady;

                // My original version
                //while (lockWasTaken && wait && !getIsReady())
                //{
                //    Monitor.Exit(syncObj);
                //    System.Threading.Thread.Sleep(random.Next(20, 70));
                //    lockWasTaken = false;
                //    Monitor.TryEnter(syncObj, Timeout.Infinite, ref lockWasTaken);
                //}
                //
                // Code implementing Trevor Pilley's suggestion to use SpinWait instead
                // After better understanding it with the help of:
                //   http://www.albahari.com/threading/part5.aspx#_SpinLock_and_SpinWait
                if (!getIsReady()) System.Threading.SpinWait.SpinUntil(() =>
                {
                    Monitor.Exit(syncObj);

                    lockWasTaken = false;
                    Monitor.TryEnter(syncObj, Timeout.Infinite, ref lockWasTaken);
                    return lockWasTaken && getIsReady();
                });

                // I now see that testing getIsReady here again is unnecessary, commenting it out.
                // if (getIsReady())
                // {
                    action(subject);
                    return true;
                // }
                return false;
            }
            finally
            {
                if (lockWasTaken) Monitor.Exit(syncObj);
            }
        }

        // Alternative parameters overloading
        public static bool DoLocked<T>(
            this T subject, object syncObj, Func<bool> getIsReady, Action<T> action
            ) where T : class
        {
            return DoLocked(subject, syncObj, getIsReady, false, action);
        }
        public static bool DoLocked<T>(
            this T subject, object syncObj, bool wait, Action<T> action
            ) where T : class
        {
            return DoLocked(subject, syncObj, (Func<bool>)null, wait, action);
        }
        public static bool DoLocked<T>(
            this T subject, object syncObj, Action<T> action
            ) where T : class
        {
            return DoLocked(subject, syncObj, (Func<bool>)null, false, action);
        }

        // The method signatures below are the same as above, except that the subject object
        // itself is also the lock object
        public static bool DoLocked<T>(
            this T subject, Func<bool> getIsReady, Action<T> action
            ) where T : class
        {
            return DoLocked(subject, subject, getIsReady, false, action);
        }
        public static bool DoLocked<T>(
            this T subject, bool wait, Action<T> action
            ) where T : class
        {
            return DoLocked(subject, subject, (Func<bool>)null, wait, action);
        }
        public static bool DoLocked<T>(
            this T subject, Action<T> action
            ) where T : class
        {
            return DoLocked(subject, subject,(Func<bool>)null, false, action);
        }
    }

    // Easy container to hold lock objects with content objects, allowing a uniform
    // access abstraction.
    public class Synced<T> where T: class
    {
        public bool AutoWait { get; set; }
        object _syncLock;
        public T InnerObject { get; private set; }

        public Synced(T source, object syncLock, bool autoWait)
        {
            AutoWait = AutoWait;
            _syncLock = syncLock;
            InnerObject = source;
        }

        public bool DoLocked(Action<T> action)
        {
            return InnerObject.DoLocked(_syncLock, null, AutoWait, action);
        }
        public bool DoLocked(bool wait, Action<T> action)
        {
            return InnerObject.DoLocked(_syncLock, null, wait, action);
        }
        public bool DoLocked(Func<bool> getIsReady, Action<T> action)
        {
            return InnerObject.DoLocked(_syncLock, getIsReady, AutoWait, action);
        }
    }

    // Non-generic static class, for easy creation of Synced<T> objects.
    public static class Synced
    {
        public static Synced<T> MakeSynched<T>(
            T source, object syncLock, bool autoWait
            ) where T : class
        {
            return new Synced<T>(source, syncLock, autoWait);
        }
        public static Synced<T> MakeSynched<T>(
            T source, object syncLock
            ) where T : class
        {
            return new Synced<T>(source, syncLock, false);
        }
        public static Synced<T> MakeSynched<T>(
            T source, bool autoWait
            ) where T : class
        {
            return new Synced<T>(source, source, autoWait);
        }
        public static Synced<T> MakeSynched<T>(T source) where T : class
        {
            return new Synced<T>(source, source, false);
        }
    }        
}

Answer 1

Based on discussion in question comments it sounds like getIsReady might be unnecessary. Then, wait parameter can also be dropped in favor of 2 different methods. So, in case of wait == true it gets simplified to:

public static void DoLocked<T>(this T subject, object syncObj, Action<T> action)
{
    //no need to check syncObject for null since lock will throw exception in this case anyway
    lock (syncLock)
    {
        action(subject);
    }
}

or, put simply we just don't need this method at all.

In case of wait == false we get the following code:

public static bool TryDoLocked<T>(this T subject, object syncObj, Action<T> action)
{
    bool lockWasTaken = false;
    try
    {
        Monitor.TryEnter(syncObj, ref lockWasTaken);

        if (lockWasTaken) 
            action(subject);

        return lockWasTaken;
    }
    finally
    {
        if (lockWasTaken)
            Monitor.Exit(syncObj);
    }
}

This method can be useful, but I think situations where I would need to attempt non-blocking operation are quite rare, especially given .NET 4.5 asynchronous API. I think some of these cases can also be rewritten in a completely non-blocking fashion using atomic operations from Interlocked class.

Answer 2

While this may work in isolation, the problem here is that it only works if every caller uses your Synced<T> class with the same syncObj as every other caller.

Also, the InnerObject exposes the actual class outside the synchronization wrapper so there is nothing which prevents someone modifying the instance separately.

A few other points around the actual "code style", you should alter your overloads for the DoLocked method, it's a good idea to add additional options after the common options. Since every method requires an action so that should be the first parameter, it helps keep the calling code consistent.

    public bool DoLocked(Action<T> action)
    public bool DoLocked(Action<T> action, bool wait)
    public bool DoLocked(Action<T> action, Func<bool> getIsReady)

If you are using .NET 4/4.5 you should use the System.Threading.SpinWait struct instead of the Thread.Sleep call, alternatively Thread.SpinWait is available in all versions of .NET.

Your parameters should be camel cased and not named after keywords - this T instance instead of this T This.