0

Apologies for the slightly speculative nature of this question.

I am writing a Cms using an in-memory static object to hold the current state of the system, and am using Event Sourcing to create an event log which I can use to rebuild the object state, and to provide roll back etc. (see http://martinfowler.com/articles/lmax.html if you have no idea what I am on about) 

public class Cms
{
    private static object WriteLock = new object();
    public static Cms Read { get; set; }
    static Cms Write { get; set; }

    static Cms()
    {
        Read = RebuildFromActionLog();
        Write = RebuildFromActionLog();
    }

    public static void Update(Action action)
    {

        lock (WriteLock)
        {                
            try
            {
                action.Apply(Write);
            }
            catch(Exception ex)
            {
                Write = RebuildFromActionLog(); //ditch the potentially messed up Write model
                throw;
            }
            LogAction(action); //the action was a keeper, so keep it
            Read = Write; //ditch the current read only model - it will continue to be used by any requests that have grabbed it
            Write = RebuildFromActionLog(); //get a new model ready for the next write
        }
    }
...
}

Aside from the potential for lots of writes to stack up if execution or rebuilding takes a long time, and the potential for lots of memory to be used, are there any bugs in this, particularly concurrency related bugs?

mcintyre321
  • 12,996
  • 8
  • 66
  • 103
  • Are you sure no thread goes for starvetion mode? Becase the lock{} block should not take up much time consuming work. Also, if one thread has aquired alock and its catch block has thrown an exception, your throwing it in catch{} block. So do you think the lock will be ever released?? – Zenwalker Oct 13 '11 at 18:09
  • hmm maybe this would be better as a concurrent list or something, and let the actions queue up in there... good point! I think the exceptions are safe: http://stackoverflow.com/questions/639493/in-c-how-can-i-safely-exit-a-lock-with-a-try-catch-block-inside – mcintyre321 Oct 13 '11 at 18:39
  • Since he is throwing from catch, the lock is never released i guess. Correct me if i am wrong :) Also if a thread throws an exception, the app will crash. – Zenwalker Oct 14 '11 at 03:15

1 Answers1

1

You may have a problem if someone uses the public getter or setter of Read, as you have no synchronization there. If someone uses that property at the same time as another thread modifies it in Update the read could fetch a stale value, or a change made by writing could be silently lost.

Does that property really need to be public? And publicly writeable?

Mark Byers
  • 811,555
  • 193
  • 1,581
  • 1,452
  • defs not writeable - though I am wondering if it should be volatile. It needs to be public so the rest of the app can get it's hands on it, right? maybe it should be a volatile private field with a public getter... – mcintyre321 Oct 13 '11 at 18:34
  • @mcintye321: If you want to be able to read/write to it from different threads without synchronization then yes it should be marked as volatile. See the docs for `volatile`: http://msdn.microsoft.com/en-us/library/x13ttww7.aspx But even with `volatile` you still have some possibly unwanted race conditions. – Mark Byers Oct 13 '11 at 19:51