Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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);
        }
    }        
}
share|improve this question
    
Can you show a use case when getIsReady is actually used? Why would you try to acquire the lock when business logic is not ready for it? –  almaz Dec 2 '12 at 9:43
    
@almaz, Perfectly valid question. When about to write to the object, I might want to check that the source data is ready before I take the lock for a 'long time', and so, of I'm not ready to write, I might as well not dwell on it. That's when I would provide different logic for 'read' and 'write'. It is still important that this test be done while the lock is in place. Does that makes sense? –  Lockszmith Dec 2 '12 at 10:58
    
well... not really :)... the common pattern is: you prepare the data to write, acquire the lock, and write to shared resource as quickly as possible. Another common pattern is double-checked locking where you do prepare the data to be written while you are in lock, but that is done in order not to prepare the same data twice in different threads (often used in singletons) –  almaz Dec 2 '12 at 11:11
    
But what if the subject object itself has a 'readiness' state that needs to be verified. especially prior to writing, it still needs to be read while locked. I do agree, however, that getIsReady MUST be a very quick test, and not a complex/long operation. –  Lockszmith Dec 2 '12 at 11:26
1  
Can you wait for subject object to be ready, capture its state if necessary, and only after that try to acquire the lock? Please provide example, as it still sounds like unnecessary complication. –  almaz Dec 2 '12 at 11:40
show 1 more comment

2 Answers 2

up vote 1 down vote accepted

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.

share|improve this answer
    
These is not a bad idea, but it looses some of the abstraction I had in mind. I'll need to sleep on this. As before you might be right though. Simple is usually better, and maybe this is one of those examples where it is. –  Lockszmith Dec 3 '12 at 18:51
    
Did you plan to pass wait as a variable and not explicit true or false? I can't imagine a case where you would need that. And if you do, your code will grow 10x bigger because each time you'll have to deal with "what if my code in action didn't run?". The simpler, the better :) –  almaz Dec 3 '12 at 19:02
    
you are so right, I reviewed this code today again, given all the comments, and I agree with you completely. The simplest approach is usually the best, and that is exactly what you suggested. Thanks! –  Lockszmith Jan 7 '13 at 11:28
add comment

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.

share|improve this answer
    
Thanks for pointing out the public InnerObject, I think that hiding it by making it private would resolve the issue though - would you agree? I'll change to the SpinWait. As for the coding style, thanks for the suggestion, though I always feel that an action should be as far to the right as possible, since it is very likely to be a multi line entry, would it be better if it was always the last param? –  Lockszmith Dec 1 '12 at 18:40
    
@Trever, thanks again, I've edited the code above to include your suggestions, I would appreciate a 2nd look by you if possible. –  Lockszmith Dec 1 '12 at 19:52
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.