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

4

6

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;
    }
}
Supramolecular answered 12/2, 2011 at 17:3 Comment(0)
N
8

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.

Novice answered 21/2, 2011 at 8:39 Comment(0)
M
1

Though it looks reliable, I have three concerns:

1) The performance cost of Invoke() on every locked action could be severe. 2) What if the action (the method) requires parameters? A more complex solution will be necessary. 3) Does the CorruptionStateDictionary grow endlessly? I think the uncorrupt() method should problem remove the object rather than set the data false.

Moving answered 12/2, 2011 at 17:33 Comment(5)
1 - Why?, 2 - My intention would be something like: ()=>{ CallMethod(localVariableAsParameter1,localVariableAsParameter2); }, 3- Good callSupramolecular
1 - see the bar chart about halfway down in this article: msdn.microsoft.com/en-us/magazine/cc163759.aspx#S3Moving
There is clearly a performance difference. The answer to this post seems to indicate that the performance cost would be negligible: #2083235 . What would you suggest as an alternative?Supramolecular
Perhaps it is ok. THe only alternative I can suggest is breaking it into two parts, lokc and unlock, and the caller would be responsible for calloing both parts, using try/finally. BTW are you concenred that the call cannot see the exception details? Perhaps your method should return the Exception object (or null) rather than bool.Moving
also btw, SO is mangling my text. It's not ALL my fault. <s>Moving
G
0
  1. Move the IsCorrupt test and the Monitor.Enter inside the Try
  2. Move the corruption set handling out of finally and into the Catch block (this should only execute if an exception has been thrown)
  3. Don't release the primary lock until after the corruption flag has been set (leave it in the finaly block)
  4. Don't restrict the execption to the calling thread; either rethow it or add it to the coruption dictionary by replacing the bool with the custom execption, and return it with the IsCorrupt Check
  5. For Uncorrupt simply remove the item
  6. There are some issues with the locking sequencing (see below)

That should cover all the bases

  public static class Locking
{
    private static readonly Dictionary<object, Exception> CorruptionStateDictionary = new Dictionary<object, Exception>();
    private static readonly object CorruptionLock = new object();
    public static bool TryLockedAction(object lockObject, Action action, out Exception exception)
    {
        var lockTaken = false;
        exception = null;
        try
        {
            Monitor.Enter(lockObject, ref lockTaken);

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

            action.Invoke();
        }
        catch (Exception ex)
        {
            var corruptionLockTaken = false;
            exception = ex;
            try
            {
                Monitor.Enter(CorruptionLock, ref corruptionLockTaken);

                if (CorruptionStateDictionary.ContainsKey(lockObject))
                {
                    CorruptionStateDictionary[lockObject] = ex;
                }
                else
                {
                    CorruptionStateDictionary.Add(lockObject, ex);
                }
            }

            finally
            {
                if (corruptionLockTaken)
                {
                    Monitor.Exit(CorruptionLock);
                }
            }
        }
        finally
        {
            if (lockTaken)
            {
                Monitor.Exit(lockObject);
            }
        }
        return exception == null;
    }
    public static void Uncorrupt(object corruptLockObject)
    {
        var lockTaken = false;
        try
        {
            Monitor.Enter(CorruptionLock, ref lockTaken);
            if (IsCorrupt(corruptLockObject))
            {
                { CorruptionStateDictionary.Remove(corruptLockObject); }
            }
        }
        finally
        {
            if (lockTaken)
            {
                Monitor.Exit(CorruptionLock);
            }
        }

    }
    public static bool IsCorrupt(object lockObject)
    {
        Exception ex = null;
        return IsCorrupt(lockObject, out ex);
    }
    public static bool IsCorrupt(object lockObject, out Exception ex)
    {
        var lockTaken = false;
        ex = null;
        try
        {
            Monitor.Enter(CorruptionLock, ref lockTaken);
            if (CorruptionStateDictionary.ContainsKey(lockObject))
            {
                ex = CorruptionStateDictionary[lockObject];
            }
            return CorruptionStateDictionary.ContainsKey(lockObject);
        }
        finally
        {
            if (lockTaken)
            {
                Monitor.Exit(CorruptionLock);
            }
        }           
    }
}
Greer answered 25/2, 2011 at 1:12 Comment(4)
Moving the Monitor.Enter inside the Try would cause the object to remain locked when an exception is thrown.Supramolecular
from Eric Lippert: In C# 4.0 we've changed lock so that it now generates code as if it were bool lockWasTaken = false; var temp = obj; try { Monitor.Enter(temp, ref lockWasTaken); { body } } finally { if (lockWasTaken) Monitor.Exit(temp); }Greer
Sorry, I should more clear. the Monitor.Enter has to go inside the Try to ensure that the exit inside the finally is executed. This is the problem in the 3.5 code that is resolved with the 4.0 code with the try inside. Eric then raises the larger issue about corruption.Greer
Are you gong to leave this down-graded?Greer
O
0

The approach I would suggest would be to have a lock-state-manager object, with an "inDangerState" field. An application that needs to access a protected resource starts by using the lock-manager-object to acquire the lock; the manager will acquire the lock on behalf of the application and check the inDangerState flag. If it's set, the manager will throw an exception and release the lock while unwinding the stack. Otherwise the manager will return an IDisposable to the application which will release the lock on Dispose, but which can also manipulate the danger state flag. Before putting the locked resource into a bad state, one should call a method on the IDisposable which will set inDangerState and return a token that can be used to re-clear it once the locked resource is restored to a safe state. If the IDisposable is Dispose'd before the inDangerState flag is re-cleared, the resource will be 'stuck' in 'danger' state.

An exception handler which can restore the locked resource to a safe state should use the token to clear the inDangerState flag before returning or propagating the exception. If the exception handler cannot restore the locked resource to a safe state, it should propagate the exception while inDangerState is set.

That pattern seems simpler than what you suggest, but seems much better than assuming either that all exceptions will corrupt the locked resource, or that none will.

Odilo answered 3/8, 2011 at 19:21 Comment(0)

© 2022 - 2024 — McMap. All rights reserved.