Putting your synchronization at the correct level


Continuing in what’s turned into my series on locking advice, I have a new example for you of a typical problem that happens in class design.  As usual I’ve simplified this almost to the point of being silly to illustrate the problem.  Hopefully the crux of what is going on still shows through.


The theme for today is that often synchronization is inserted into the wrong level of programming code.  Low level classes, like the Counter in this example, have very little idea what is going on in the world around them and so they can’t make especially good synchronization decisions. 


Often synchronization at the low level ends up doing nothing useful, but instead only creates complexity and wastes cycles.  My “favorite” example of this is the availablity of synchronized collection classes which, like the Counter example below, have difficulty meeting the synchronization needs of their clients because they have no understanding of the overall unit of work and are therefore unhelpful when composed with other resources.

    public class Counter
{
private string name;
private int reads;
private int writes;

public Counter(string _name)
{
name = _name;
reads = writes = 0;
}

public void AddRead()
{
lock(this) // point #1 — do we like the three lines marked point #1 ?
{
reads++;
}
}

public void AddWrite()
{
lock(this) // point #1 — do we like the three lines marked point #1 ?
{
writes++;
}
}

public void WriteCounts(TextWriter tw)
{
lock(this) // point #1 — do we like the three lines marked point #1 ?
{
// point #4 — do we even like this method for this class?
tw.WriteLine(“{0} {1} {2}”, name, reads, writes);
}
}
}

public class Updater
{
private static Counter c1 = new Counter(“Category 1”);
private static Counter c2 = new Counter(“Category 2”);
private static Object myLock = new Object();

// this method is called by many threads
public static void DoWork(bool br1, bool bw1, bool br2, bool bw2)
{
lock (myLock) // point #2, do we need this? does it even belong?
{
if (br1) c1.AddRead();
if (br2) c2.AddRead();
if (bw1) c1.AddWrite();
if (bw2) c2.AddWrite();

// point #3 could we change the lock so that it didn’t include these two slowish statements?

c1.WriteCounts(Console.Out);
c2.WriteCounts(Console.Out);
}
}
}


What are your thoughts on the marked points (1-4)?  What would a better Counter class look like to meet the needs of DoWork?


(my “solution” is now available here)

Comments (33)

  1. Rico,

    This is the classic example where "Design for future" fails.

    And even more is the classic example where a Decorator should be used.

    You would then have two classes: Counter with NO locks and SynchronizedCounter (using inheritance or composition as you find if fit) that adds synchronization to the Counter.  

    In your Updater you would then user the Counter, thus getting no performance hit.

    Also, using lock(this) is very dangerous as other people might lock your instance from outside, creating unexpected and hard to trace locks. I this you should always use a member object for locks.

    Corneliu.

  2. ricom says:

    >>Also, using lock(this) is very dangerous as other people might lock your instance from outside

    That sounds like a pretty strong vote for "don’t like" on point #1.

    See:

    http://msdn.microsoft.com/library/default.asp?url=/archive/en-us/dnaraskdr/html/askgui06032003.asp

    For more details on that which support your position.

    But I don’t agree that this is a "classic example where a Decorator should be used."  I think you can guess what my thoughts are on that from the original posting, but I’ll follow up after others have had a chance to comment too.

  3. IMHO, a better Counter class wouldn’t be thread safe. It would allow the client code to perform locking.

    Indeed, this is the exact problem that faced Java developers using the StringBuffer class. Java’s StringBuffer class performed locking – even though it was at too low a level to really understand the locking requirements of the application. I blogged about this a while ago here: http://kentb.blogspot.com/2005/08/i-hate-java-part-3.html

  4. ricom says:

    High 5 for Kent 🙂

  5. Conceptually, member modifications should be thread-safe, regardless of a client’s unit-of-work.  The lock used to synchronize members of the Counter class could be made public to allow clients the ability to optimize synchronization (see SyncRoot).

    Some points I’d make regarding your example:

    * While it’s necessary to synchronize access to "reads" and "writes", does accessing "reads" need to lock out accesses to "writes", and vice versa?  I think your example needlessly locks the entire Counter object for any access to any member; which could affect performance.

    * Conceptually, it’s safer to synchronize the invariant c1&c2, regardless of whether Counter is "thread-safe"; especially (not in this case) if the members of the invariant depend upon one-another.  You don’t always need to simply synchronize the access to the values of the invariant’s members; but the reference as well.  For example, if you had another thread that assigned another object to c1 within a lock(myLock) block, DoWork would be thread-safe in this case.  myLock doesn’t solely synchronize modification of "reads" and "writes" from outside DoWork, that may just be a side-effect…

    * If Counter does not synchronize its private members, and you expect a client class to perform synchronization in such a way to protect those private members; you’re coupling client class’s to the internal implementation of Counter.  Changing Counter could then impact the "correctness" of a client’s synchronization.  I don’t think that’s what you want.

  6. Brian Chiasson says:

    Peter,

    >>If Counter does not synchronize its private members, and you expect a client class to perform synchronization in such a way to protect those private members; you’re coupling client class’s to the internal implementation of Counter.

    By relying on the thread safety of Counter’s members aren’t you forcing this coupling anyway?

    Rico pointed out in the entry that this class is intended to be low-level, and as such, I agree with Kent’s recommendation.

    Nevermind the fact that simply locking the AddRead/AddWrite operations does not ensure that WriteCounts will be synchronized with the appropriately incremented values unless the client’s classes have implemented some sort of locking mechanism in the first place.

  7. Alois Kraus says:

    Is it better? I hope at least a little bit. I have made Counter a value type so I can copy it when still holding a lock to print the consistent state out at a later time. The Update does now also the printing with the copied value. As additional bonus I did make the atomicity of the DoWork function configurable depending on the client needs.

    public struct Counter
    {
    private string name;
    private int reads;
    private int writes;

    public string Name
    {
    get { return name; }
    }
    public int Reads
    {
    get { return reads; }
    }
    public int Writes
    {
    get { return writes; }
    }

    public Counter(string _name)
    {
    name = _name;
    }

    public void AddRead()
    {
    reads++;
    }

    public void AddWrite()
    {
    writes++;
    }
    }

    public class Updater
    {
    private static Counter c1 = new Counter(“Category 1”);
    private static Counter c2 = new Counter(“Category 2”);
    private static Object myLock = new Object();

    private static void WriteCounts(ref Counter copy)
    {
    Console.WriteLine(“{0} {1} {2}”, copy.Name, copy.Reads, copy.Writes);
    }

    static bool myAtomicUpdateEnabled = false;
    public static bool AtomicUpdateEnabled
    {
    get
    {
    lock (myLock)
    {
    return myAtomicUpdateEnabled;
    }
    }
    set
    {
    lock (myLock)
    {
    myAtomicUpdateEnabled = value;
    }
    }
    }

    // this method is called by many threads
    public static void DoWork(bool br1, bool bw1, bool br2, bool bw2)
    {
    Counter c1Copy;
    Counter c2Copy;

    try // Do atomic updates only when the client does enable it.
    {
    if (myAtomicUpdateEnabled)
    System.Threading.Monitor.Enter(myLock);

    if (br1) c1.AddRead();
    if (br2) c2.AddRead();
    if (bw1) c1.AddWrite();
    if (bw2) c2.AddWrite();

    c1Copy = c1;
    c2Copy = c2;
    }
    finally
    {
    System.Threading.Monitor.Exit(myLock);
    }

    WriteCounts(ref c1Copy);
    WriteCounts(ref c2Copy);

    }
    }

  8. Alois Kraus says:

    Is it better? I hope at least a little bit. I have made Counter a value type so I can copy it when still holding a lock to print the consistent state out at a later time. The Update does now also the printing with the copied value. As additional bonus I did make the atomicity of the DoWork function configurable depending on the client needs.

    public struct Counter
    {
    private string name;
    private int reads;
    private int writes;

    public string Name
    { get { return name; } }

     public int Reads
     { get { return reads; } }

     public int Writes
     { get { return writes; } }

     public Counter(string _name)
     {   name = _name;
     }

     public void AddRead()
     {   reads++;  }

     public void AddWrite()
     {  writes++;     }
    }

    public class Updater
    {
    private static Counter c1 = new Counter(“Category 1”);
    private static Counter c2 = new Counter(“Category 2”);
    private static Object myLock = new Object();

    private static void WriteCounts(ref Counter copy)
    {
      Console.WriteLine(“{0} {1} {2}”, copy.Name, copy.Reads, copy.Writes);
     }

    static bool myAtomicUpdateEnabled = false;
    public static bool AtomicUpdateEnabled
    { get
      {
        lock (myLock) {
        return myAtomicUpdateEnabled;
         }  
      }
      set
      {
        lock (myLock){                    myAtomicUpdateEnabled = value;                
        }
      }
    }

    // this method is called by many threads
    public static void DoWork(bool br1, bool bw1, bool br2, bool bw2)
    {
     Counter c1Copy;
     Counter c2Copy;

     try // Do atomic updates only when the client does enable it.
     {
       if (myAtomicUpdateEnabled)               System.Threading.Monitor.Enter(myLock);

       if (br1) c1.AddRead();
       if (br2) c2.AddRead();
       if (bw1) c1.AddWrite();
       if (bw2) c2.AddWrite();

       c1Copy = c1;
       c2Copy = c2;
     }
     finally
     {
      System.Threading.Monitor.Exit(myLock);
     }
     
     WriteCounts(ref c1Copy);
     WriteCounts(ref c2Copy);
             
    }
    }

     

    [Alois discusses and refines this approach further below, this comment was inadvertently censored by the blog software -Rico]

  9. Maurits says:

    The first two #1s are silly because ++ is an atomic operation already.

    The third #1 is (potentially) reasonable because "reads" or "writes" or both could change while the function call is being built.

    The lock at point #2 is necessary because multiple threads call DoWork and you don’t want crossed calls to AddRead or AddWrite on the same objects.  Alas, the locks #1 will not help with these crosses.  Note if calls to AddRead() and/or AddWrite() exist in other functions that don’t lock myObject, there may still be crossed calls.

    #3 — alas, the lock around WriteCounts is necessary for the same reason.

    If the atomicity of the AddRead/AddWrite is important, it should be a single member function.

    Writing to a TextWriter can be slow, the more so if it’s not Console.Out.  A better solution is to write a function that expresses the counter object as a string, and let the caller write that to the stream.

    So here’s a potential new member function:

    public string Counter::AddReadsAndWrites(int morereads, int morewrites)

    {

    StringBuilder sb = name;

    int newreads, int newwrites;

    lock (*this)

    {

    newreads = (reads += morereads);

    newwrites = (writes += morewrites);

    };

    sb += " "; sb += newreads; sb += " "; sb += newwrites;

    return sb.ToString();

    }

    DoWork becomes:

    lock (myLock) // still necessary

    {

    string s1 = c1.AddReadsAndWrites((br1 ? 1 : 0), (bw1 ? 1 : 0));

    string s2 = c2.AddReadsAndWrites((br2 ? 1 : 0), (bw2 ? 1 : 0));

    }

    // note these are now safely outside the lock

    Console.WriteLine(s1);

    Console.WriteLine(s2);

  10. ricom says:

    >>The first two #1s are silly because ++ is an atomic operation already.

    ++ is not atomic

    AddRead/AddWrite do not have to be atomic operations however writing a consistent pair of the numbers is to be atomic.  Which means *some* locking is necessary.  But is it the locking marked #1?

    Can you really call Console.WriteLine outside of the lock and get consistent output?  A leading question…

  11. Maurits says:

    > ++ is not atomic

    ! Really? I learn something new every day. So crossed AddRead()s could lead to lost information.

    AddRead() { reads = read + 1; }

    Start: this.reads == 0

    Thread A: read value of "reads" (0)

    Thread B: read value of "reads" (0)

    Thread B: add one (1)

    Thread B: write to "reads" (1)

    Thread A: add one (1)

    Thread A: write to "reads" (1)

    End: this.reads == 1

    Two AddRead() calls, only one takes effect.

    > is it the locking marked #1?

    Well, that would solve that consistency problem; but #2 would equally well, and perhaps the Counter object should leave synchronicity to the caller.

    > Can you really call Console.WriteLine outside of the lock and get consistent output?

    eh… no, I suppose the threads could cross between Console.WriteLine statements, so you get one s1 and a different s2.

  12. ricom says:

    I don’t think Console.WriteLine is itself atomic… so it’s worse than line interleaves… who knows what happens.

  13. Maurits says:

    There’s a thread-safe wrapper function to solve the WriteLine issue.

    TextWriter.Synchronized(Console.Out).WriteLine(s)

    That takes care of the thread-safety issues incumbent in Console.WriteLine.

    The thread-interleaving issue between s1 and s2 can be taken care of by using a single string s and locking it:

    if (cr1 || cw1)

    {

    lock(c1)

    {

    if (cr1) { c1.AddRead(); }

    if (cw1) { c1.AddWrite(); }

    }

    }

    if (cr2 || cw2)

    {

    lock(c2)

    {

    if (cr2) { c2.AddRead(); }

    if (cw2) { c2.AddWrite(); }

    }

    }

    string s;

    lock(s)

    lock(c1) // if it’s not important that c1 and c2 be read at the same time these locks can be wrapped around one line below

    lock(c2)

    {

    s = c1.Name + " " + c1.Reads + " " + c2.Writes + "n";

    s += c2.Name + " " + c2.Reads + c2.Writes + "n";

    TextWriter.Synchronized(Console.Out).Write(s);

    }

  14. ricom says:

    yikes… more complexity

  15. Alois Kraus says:

    Lets make things easier:
    1. Change the “class Counter” to “struct Counter”
    2. Add some getter to its members
    3. Remove all locks in Counter.
    4. Remove the WriteCounts in Counter.
    5. Add static Print to Updater.

    This enables us to lock in the higher class Updater and make a copy inside the lock of DoWork to print out an consistent state later.

    5. Make the locking strategry of Updater configurable for external clients so they can decide if they need atomicity of DoWork or not.
    6. The code for DoWork would look then like this:

    try
    {
     Counter ?c1Copy;
     Counter ?c2Copy;

     if( myEnableLocking )
       Monitor.Enter(myLock)

       if (br1) c1.AddRead();
       if (br2) c2.AddRead();
       if (bw1) c1.AddWrite();
       if (bw2) c2.AddWrite();

    }
    finally
    {
     if( myEnableLocking)
     {
       c1Copy = c1;
       c2Copy = c2;
       Monitor.Leave(myLock)
     }
    }
     
    // Now print out copied values which were catched in an consistent state
    if( c1Copy != null && c2Copy != null )
      Print(c1Copy,c2Copy);
    else
      Print(c1,c2); // No locking done ok then our client have done proper locking for us.

    Yours,
     Alois Kraus

  16. ricom says:

    Interesting use of Monitor.Enter and Monitor.Leave to show that you can get functionality identical to “lock” with more complicated flow control and still have good code sharing.

    For myself I don’t believe in the hybrid approach — “I can be locked, or not locked”.  It results in code that is more complex than either alternative.

    You could imagine a version of DoWork that relies on client synchronization.  Or you can imagine that DoWork is high enough level to do its own synchronization.  I think either choice is ok there.

    I’m not sure what sort of exception we’re protecting ourselves from with the try block.  Though I suppose it pays to be careful in this case because we did manually acquire the Monitor.

    But can we really move the Print out of the lock?  I think not because then we will print out of order.  If we wanted to do that we’d need some way to at least recover the order (such as writing a sequence number).  Otherwise we have arbitrarily ordered output.

    Alois: you had two other comments posted earlier that are currently suppressed.  My blog normally allows comments without my having to approve them.  Did you self-suppress those comments or is that some kind of bug and you would like me to enable the comments for you?  I have no objection to what you posted of course.  Perhaps long comments are set to require approval.

  17. Alois Kraus says:

    Hi Rico,

    I think my post was too long but there was no error shown. After doing a repost and making it shorter I did have still no success. But my third comment did make it finally ;-). The user feedback that the post is too long is missing.

    As for the taking the print statement out of the lock: Console.WriteLine is damn slow. I did favor performance over guaranteed order of printout if the stream is a file I would include it inside the lock. It all depends …

    Yours,

     Alois Kraus

  18. ricom says:

    I enabled the second of your two postings (it should appear above shortly).  I will have to keep my eye out for long posts that need to be manually approved from now on.

    Thank you (all!) for the comments!

  19. Mike Hearn says:

    Does C# really not have any equivalent to InterlockedIncrement?

    In the WriteCounts method i’d be tempted to do something like:

    {

       // assuming it doesn’t matter if

       // these aren’t quite in sync

       int _reads = reads;

       int _writes = writes;

       ….

    }

    That is, copy them onto the stack … a word-sized read should be atomic on x86 right? I don’t know what guarantees .NET makes about this. Maybe to read a value safely you must lock. I do not know.

    If Console.WriteLine doesn’t do its own locking then I guess you would have to do

    lock(Console) Console.WriteLine(…)

  20. Joku says:

    Would using volatile be wrong here? I really have little clue when to use it and what are the pro/cons.

  21. ricom says:

    There’s no need.  Entering the lock must create a memory barrier or else the construct wouldn’t be very useful… you’d have to put volatile everywhere.  You typically use volatile if you are trying to avoid explicit locks with some trickiness.

  22. Of course when I say “solution”, I really mean “my solution” because there’s not really just one…

  23. Maurits says:

    OK, I have another idea… buffered output.  That should allow writing to the stream (the most expensive operation) without holding open a lock.

    I’ve posted the code sample out-of-band here

    http://www.geocities.com/mvaneerde/counter-concurrency.txt

    for the twin purposes of keeping the thread readable and allowing me to make in-place updates 😉

  24. Maurits says:

    Oops, too late… didn’t see the solution was posted already. 🙂

  25. ricom says:

    It’s never too late 🙂

  26. Maurits says:

    I see a potential weakness in my solution of a dedicated output thread.  If DoWork() is called faster than PrintOutput() can write to the stream, the buffer will build up and build up until it creates a resource problem.

  27. I get a lot of questions about why the new generic collection interfaces (IList<T> for example)…

  28. Peter Ritchie says:

    Brian,

    >>By relying on the thread safety of Counter’s

    >>members aren’t you forcing this coupling

    >>anyway?

    The extent of the coupling would only be to the interface, not the implementation.  You can’t use a class without at lease being coupled to the interface.  Part of my point was you can’t possibly expect to correctly syncrhonize an object’s internals if you don’t know what they are.

    >>Nevermind the fact that simply locking the

    >>AddRead/AddWrite operations does not ensure

    >>that WriteCounts will be synchronized with

    >>the appropriately incremented values unless

    >>the client’s classes have implemented some

    >>sort of locking mechanism in the first place.

    Why not?  If WriteCounts can’t proceed until it locks this, and AddRead and AddWrite do the same, how could WriteCounts not be syncrhonized with the incremented values?

  29. Peter Ritchie says:

    See http://blogs.msdn.com/ericgu/archive/2005/04/22/410809.aspx and http://www.thescripts.com/forum/thread243478.html regarding problems with "lock (this)" and http://msdn2.microsoft.com/en-us/library/c5kehkcz.aspx for locking publicly available references (including "lock(this)").

    I don’t think this is a well thought-out example in terms of design-correctness and a discussion on performance is therefore moot.

  30. Peter Ritchie says:

    May I suggest analyzing the performance of an example that follows Design Guidelines and best practices more closely, for example:

    public class Counter

    {

    private readonly string name;

    private int reads, writes;

    private object readsLock = new object(), writesLock = new object();

    public Counter(string _name)

    {

    name = _name;

    reads = writes = 0;

    }

    public void AddRead()

    {

    lock (readsLock) // make AddRead thread-safe

    {

    reads++;

    }

    }

    public void AddWrite()

    {

    lock (writesLock) // make AddRead thread-safe

    {

    writes++;

    }

    }

    public void WriteCounts(System.IO.TextWriter tw)

    {

    lock (writesLock)

    {

    // with above lock, make WriteCounts thread-safe

    // with respect to itself

    lock (readsLock)

    {

    System.IO.TextWriter.Synchronized(tw).WriteLine("{0} {1} {2}", name, reads, writes);

    }

    }

    }

    }

    public class Updater

    {

    private static Counter c1 = new Counter("Category 1");

    private static Counter c2 = new Counter("Category 2");

    private static Object myLock = new Object();

    // this method is called by many threads

    public static void DoWork(bool br1, bool bw1, bool br2, bool bw2)

    {

    // this lock is needed to make DoWork thread-safe,

    // because c1, and c2 could be accessed by multiple threds

    // using DoWork, it doesn’t matter what type of reference c1 and c2 are

    // This lock in no way attempts to ensure AddRead and AddWrite are thread

    // safe, that is a seperate issue.

    lock (myLock)

    {

    if (br1) c1.AddRead();

    if (br2) c2.AddRead();

    if (bw1) c1.AddWrite();

    if (bw2) c2.AddWrite();

    c1.WriteCounts(Console.Out);

    c2.WriteCounts(Console.Out);

    }

    }

    }

  31. In my last quiz I asked a few questions about a few hypothetical classes that might appear in a value-right…

  32. In my last quiz I asked a few questions about a few hypothetical classes that might appear in a value-rich