Putting your synchronization at the correct level -- solution
Of course when I say "solution", I really mean "my solution" because there's not really just one way to do this. But this is an approach I like and I'll go over why I like it.
Recall the code from the original problem: (there was some very interesting discussion so it's worth clicking to see what people thought)
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);
}
}
}
Point #1
I think it's a very bad idea to introduce locking into a low level class like this counter. It has no awareness of what is going on in the overall scheme of things. Generally classes like this with synchronization end up having redundant features that add complexity and cost for no benefit. This is exactly what is happening here. We have every expectation of having to use several of these counters in tandem and therefore synchronization within any one counter is worthless.
Point #2
The lock here is inescapable. Indeed it is fundamentally the purpose of this class to coordinate three resources -- the two counters and the output stream. Removing this lock would put the onus on some higher level code which might otherwise remain blissfully ignorant of the coordination. Now the opposite could be true: if the higher level code has to do a variety of its own synchronization then perhaps even this level is "too low" but it seems likely that this is a good fit. Note the static entry point and the presence of high level actions such as logging.
Point #3
As it is written the code is trying to coordinate three resources. Without changing the format we would be unable to decode the output in any meaningful way. Furthermore we need some form of synchronization on the output because the API itself is not threadsafe. We could lament that the writing might be slow, but since we are fundamentally required to write and it must be coordinated, just splitting the lock won't help us -- we'd simply move the blocking elsewhere. However perhaps there something that can be done to mitigate the cost. I offer one such suggestion in my solution below.
Point #4
I certainly do not like the Writing functionality at this level -- it seems rather orthogonal to the problem of keeping read and write counts. Does this really fit? See this other article for more thinking on these lines.
So what would I suggest instead? Perhaps something like this (though there are other good options)
public class Counter
{
private string name;
private int reads;
private int writes;
public Counter(string _name)
{
name = _name;
reads = writes = 0;
}
public void AddRead()
{
reads++;
}
public void AddWrite()
{
writes++;
}
public void FormatCounts(StringBuilder sb)
{
sb.AppendFormat("{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();
private static Object myOutputLock = new Object();
private static StringBuilder sb = new StringBuilder(4096);
private static long sequenceNumber = 0;
// this method is called by many threads
public static void DoWork(bool br1, bool bw1, bool br2, bool bw2)
{
StringBuilder sbOut = null;
lock (myLock)
{
if (br1) c1.AddRead();
if (br2) c2.AddRead();
if (bw1) c1.AddWrite();
if (bw2) c2.AddWrite();
sb.AppendFormat("{0} ", sequenceNumber++);
c1.FormatCounts(sb);
sb.Append(" ");
c2.FormatCounts(sb);
sb.Append("\r\n");
if (sb.Length >= 4000)
{
sbOut = sb;
sb = new StringBuilder(4096);
}
}
// note other threads can keep doing the main job
// this thread becomes the output worker temporarily
// you could do real async i/o here if you wanted
// output could be out of order but sequence number allows decoding
if (sbOut != null)
{
lock(myOutputLock)
{
Console.WriteLine(sbOut.ToString());
}
}
}
}
And can you think of three reasons why I like that better? :)
(Also it needs a simple flush method to handle shutdown -- left as an exercise for the reader :))