What is wrong with this solution to locking and managing locked exceptions?

Question

My objective is a convention for thread-safe functionality and exception handling within my application. I'm relatively new to the concept of thread management/multithreading. I am using .NET 3.5

I wrote the following helper method to wrap all my locked actions after reading this article http://blogs.msdn.com/b/ericlippert/archive/2009/03/06/locks-and-exceptions-do-not-mix.aspx, which was linked in response to this question, Monitor vs lock.

My thought is that if I use this convention consistently in my application, it will be easier to write thread-safe code and to handle errors within thread safe code without corrupting the state.

public static class Locking
{

    private static readonly Dictionary<object,bool> CorruptionStateDictionary = new Dictionary<object, bool>(); 
    private static readonly object CorruptionLock = new object();

    public static bool TryLockedAction(object lockObject, Action action, out Exception exception)
    {
        if (IsCorrupt(lockObject))
        {
            exception = new LockingException("Cannot execute locked action on a corrupt object.");
            return false;
        }
        exception = null;
        Monitor.Enter(lockObject);
        try
        {
            action.Invoke();
        }
        catch (Exception ex)
        {
            exception = ex;
        }
        finally
        {
            lock (CorruptionLock)   // I don't want to release the lockObject until its corruption-state is updated.
                                    // As long as the calling class locks the lockObject via TryLockedAction(), this should work
            {
                Monitor.Exit(lockObject);
                if (exception != null)
                {   
                    if (CorruptionStateDictionary.ContainsKey(lockObject))
                    {
                        CorruptionStateDictionary[lockObject] = true;
                    }
                    else
                    {
                        CorruptionStateDictionary.Add(lockObject, true);
                    }
                }
            }
        }
        return exception == null;
    }

    public static void Uncorrupt(object corruptLockObject)
    {
        if (IsCorrupt(corruptLockObject))
        {
            lock (CorruptionLock)
            {
                CorruptionStateDictionary[corruptLockObject] = false;
            }
        }
        else
        {
            if(!CorruptionStateDictionary.ContainsKey(corruptLockObject))
            {
                throw new LockingException("Uncorrupt() is not valid on object that have not been corrupted."); 
            }
            else
            {
                //  The object has previously been uncorrupted.
                //  My thought is to ignore the call.
            }
        }
    }

    public static bool IsCorrupt(object lockObject)
    {
        lock(CorruptionLock)
        {
            return CorruptionStateDictionary.ContainsKey(lockObject) && CorruptionStateDictionary[lockObject];
        }
    }


}

I use a LockingException class for ease of debugging.

    public class LockingException : Exception
    {
        public LockingException(string message) : base(message) { }
    }

Here is an example usage class to show how I intend to use this.

public class ExampleUsage
{
    private readonly object ExampleLock = new object();

    public void ExecuteLockedMethod()
    {
        Exception exception;
        bool valid = Locking.TryLockedAction(ExampleLock, ExecuteMethod, out exception);
        if (!valid)
        {
            bool revalidated = EnsureValidState();
            if (revalidated)
            {
                Locking.Uncorrupt(ExampleLock);
            }
        }
    }

    private void ExecuteMethod()
    {
        //does something, maybe throws an exception

    }

    public bool EnsureValidState()
    {
        // code to make sure the state is valid
        // if there is an exception returns false,

        return true;
    }
}

Answer 1

Your solution seems to add nothing but complexity due to a race in the TryLockedAction:

        if (IsCorrupt(lockObject))
        {
            exception = new LockingException("Cannot execute locked action on a corrupt object.");
            return false;
        }
        exception = null;
        Monitor.Enter(lockObject);

The lockObject might become "corrupted" while we are still waiting on the Monitor.Enter, so there is no protection.

I'm not sure what behaviour you'd like to achieve, but probably it would help to separate locking and state managing:

class StateManager
{
    public bool IsCorrupted
    {
        get;
        set;
    }

    public void Execute(Action body, Func fixState)
    {
        if (this.IsCorrupted)
        {
            // use some Exception-derived class here.
            throw new Exception("Cannot execute action on a corrupted object.");
        }

        try
        {
            body();
        }
        catch (Exception)
        {
            this.IsCorrupted = true;
            if (fixState())
            {
                this.IsCorrupted = false;
            }

            throw;
        }
    }
}

public class ExampleUsage
{
    private readonly object ExampleLock = new object();
    private readonly StateManager stateManager = new StateManager();

    public void ExecuteLockedMethod()
    {
        lock (ExampleLock)
        {
            stateManager.Execute(ExecuteMethod, EnsureValidState);
        }
    }

    private void ExecuteMethod()
    {
        //does something, maybe throws an exception

    }

    public bool EnsureValidState()
    {
        // code to make sure the state is valid
        // if there is an exception returns false,

        return true;
    }
}

Also, as far as I understand, the point of the article is that state management is harder in presence of concurrency. However, it's still just your object state correctness issue which is orthogonal to the locking and probably you need to use completely different approach to ensuring correctness. E.g. instead of changing some complex state withing locked code region, create a new one and if it succeeded, just switch to the new state in a single and simple reference assignment:

public class ExampleUsage
{
    private ExampleUsageState state = new ExampleUsageState();

    public void ExecuteLockedMethod()
    {
        var newState = this.state.ExecuteMethod();
        this.state = newState;
    }
}

public class ExampleUsageState
{
    public ExampleUsageState ExecuteMethod()
    {
        //does something, maybe throws an exception
    }
}

Personally, I always tend to think that manual locking is hard-enough to treat each case when you need it individually (so there is no much need in generic state-management solutions) and low-lelvel-enough tool to use it really sparingly.