Putting your synchronization at the correct level

아티클
05/01/2006

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

Anonymous
May 01, 2006
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.
Anonymous
May 01, 2006
>>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.
Anonymous
May 01, 2006
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
Anonymous
May 01, 2006
The comment has been removed
Anonymous
May 02, 2006
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.
Anonymous
May 02, 2006
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.
Anonymous
May 02, 2006
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); } }
Anonymous
May 02, 2006
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]
Anonymous
May 02, 2006
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);
Anonymous
May 02, 2006
>>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...
Anonymous
May 02, 2006
> ++ 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.
Anonymous
May 02, 2006
The comment has been removed
Anonymous
May 02, 2006
The comment has been removed
Anonymous
May 02, 2006
yikes... more complexity
Anonymous
May 02, 2006
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
Anonymous
May 02, 2006
The comment has been removed
Anonymous
May 02, 2006
The comment has been removed
Anonymous
May 02, 2006
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!
Anonymous
May 03, 2006
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(...)
Anonymous
May 03, 2006
Would using volatile be wrong here? I really have little clue when to use it and what are the pro/cons.
Anonymous
May 03, 2006
The comment has been removed
Anonymous
May 03, 2006
Of course when I say "solution", I really mean "my solution" because there's not really just one...
Anonymous
May 03, 2006
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 ;)
Anonymous
May 03, 2006
Oops, too late... didn't see the solution was posted already. :)
Anonymous
May 03, 2006
It's never too late :)
Anonymous
May 03, 2006
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.
Anonymous
May 05, 2006
I get a lot of questions about why the new generic collection interfaces (IList<T> for example)...
Anonymous
May 05, 2006
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?
Anonymous
May 05, 2006
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.
Anonymous
May 05, 2006
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);
}
}
}
Anonymous
September 07, 2006
In my last quiz I asked a few questions about a few hypothetical classes that might appear in a value-right...
Anonymous
January 23, 2007
In my last quiz I asked a few questions about a few hypothetical classes that might appear in a value-rich

다음을 통해 공유

Putting your synchronization at the correct level

Comments

추가 리소스