Explain Strange Synchronization in WCF Source Code

Question

While looking at the source code of System.ServiceModel.Channels.BufferManager, I noticed this method:

void TuneQuotas()
{
    if (areQuotasBeingTuned)
        return;

    bool lockHeld = false;
    try
    {
        try { }
        finally
        {
            lockHeld = Monitor.TryEnter(tuningLock);
        }

        // Don't bother if another thread already has the lock
        if (!lockHeld || areQuotasBeingTuned)
            return;
        areQuotasBeingTuned = true;
    }
    finally
    {
        if (lockHeld)
        {
            Monitor.Exit(tuningLock);
        }
    }
    //
    // DO WORK... (code removed for brevity)
    //
    areQuotasBeingTuned = false;
}

Obviously, they want only one thread to run TuneQuotas(), and other threads to not wait if it is already being run by another thread. I should note that the code removed was not try protected.

I'm trying to understand the advantages of this method above over just doing this:

void TuneQuotas()
{
    if(!Monitor.TryEnter(tuningLock)) return;
    //
    // DO WORK...
    //
    Monitor.Exit(tuningLock);
}

Any ideas why they might have bothered with all that? I suspect the way they use the finally blocks is to guard against a thread abort scenario, but I still don't see the point because, even with all this code, TuneQuotas() would be locked for good if that one thread doesn't make it all the way to the end to set areQuotasBeingTunes=false, for one reason or another. So is there something cool about this pattern that I'm missing?

EDIT: As a side note, it seems the method exists in .NET 4.0, which I confirmed using this code running on framework 4 (although I cannot confirm that the content of the method hasn't changed from what I found on the web):

var buffMgr = BufferManager.CreateBufferManager(1, 1);
var pooledBuffMgrType = buffMgr.GetType()
    .GetProperty("InternalBufferManager")
    .GetValue(buffMgr, null)
    .GetType();

Debug.WriteLine(pooledBuffMgrType.Module.FullyQualifiedName);
foreach (var methodInfo in pooledBuffMgrType
    .GetMethods(BindingFlags.Instance | BindingFlags.NonPublic))
{
    Debug.WriteLine(methodInfo.Name);
}

which outputs:

C:\Windows\Microsoft.Net\assembly\GAC_MSIL\System.Runtime.DurableInstancing\v4.0_4.0.0.0__3    1bf3856ad364e35\System.Runtime.DurableInstancing.dll
ChangeQuota
DecreaseQuota
FindMostExcessivePool
FindMostStarvedPool
FindPool
IncreaseQuota
TuneQuotas
Finalize
MemberwiseClone

Answer 1

Any ideas why they might have bothered with all that?

After running some tests, I think see one reason (if not THE reason): They probably bothered with all that because it is MUCH faster!

It turns out Monitor.TryEnter is an expensive call IF the object is already locked (if it's not locked, TryEnter is still very fast -- no problems there). So all threads, except the first one, are going to experience the slowness.

I didn't think this would matter that much; since afterall, each thread is going to try taking the lock just once and then move on (not like they'd be sitting there, trying in a loop). However, I wrote some code for comparison and it showed that the cost of TryEnter (when already locked) is significant. In fact, on my system each call took about 0.3 ms without the debugger attached, which is several orders of magnitude slower than using a simple boolean check.

So I suspect, this probably showed up in Microsoft's test results, so they optimized the code as above, by adding the fast track boolean check. But that's just my guess..

Answer 2

Until .NET 4.0 there was essentially a bug in the code that was generated by a lock statment. It would generate something similar to the following:

Monitor.Enter(lockObject)
// see next paragraph
try
{
    // code that was in the lock block
}
finally
{
   Monitor.Exit(lockObject);
}

This means that if an exception occurred between Enter and try, the Exit would never be called. As usr alluded to, this could happen due to Thread.Abort.

Your example:

if(!Monitor.TryEnter(tuningLock)) return;
//
// DO WORK...
//
Monitor.Exit(tuningLock);

Suffers from this problem and more. The window in which this code and become interrupted and Exit not be called is basically the whole block of code--by any exception (not just one from Thread.Abort).

I have no idea why most code was written in .NET. But, I surmise that this code was written to avoid the problem of an exception between Enter and try. Let's look at some of the details:

try{}
finally
{
  lockHeld = Monitor.TryEnter(tuningLock);
}

Finally blocks basically generate a constrained execution region in IL. Constrained execution regions cannot be interrupted by anything. So, putting the TryEnter in the finally block above ensures that lockHeld reliably holds the state of the lock.

That block of code is contained in a try/finally block whose finally statement calls Monitor.Exit if tuningLock is true. This means that there is no point between the Enter and the try block that can be interrupted.

FWIW, this method was still in .NET 3.5 and is visible in the WCF 3.5 source code (not the .NET source code). I don't know yet what's in 4.0; but I would imagine it would be the same; there's no reason to change working code even if the impetus for part of its structure no longer exists.

For more details on what lock used to generate see http://blogs.msdn.com/b/ericlippert/archive/2007/08/17/subtleties-of-c-il-codegen.aspx

Answer 3

I'll add some comments:

void TuneQuotas()
{
    if (areQuotasBeingTuned)
        return; //fast-path, does not require locking

    bool lockHeld = false;
    try
    {
        try { }
        finally
        {
            //finally-blocks cannot be aborted by Thread.Abort
            //The thread could be aborted after getting the lock and before setting lockHeld
            lockHeld = Monitor.TryEnter(tuningLock);
        }

        // Don't bother if another thread already has the lock
        if (!lockHeld || areQuotasBeingTuned)
            return; //areQuotasBeingTuned could have switched to true in the mean-time
        areQuotasBeingTuned = true; //prevent others from needlessly trying to lock (trigger fast-path)
    }
    finally //ensure the lock being released
    {
        if (lockHeld)
        {
            Monitor.Exit(tuningLock);
        }
    }
    //
    // DO WORK... (code removed for brevity)
    //
    //this might be a bug. There should be a call to Thread.MemoryBarrier,
    //or areQuotasBeingTuned should be volatile
    //if not, the write might never reach other processor cores
    //maybe this doesn't matter for x86
    areQuotasBeingTuned = false;
}

The simple version you gave does not protect against some problems. At the very least it is not exception-safe (lock won't be released). Interestingly, the "sophisticated" version, doesn't either.

This method has been removed from .NET 4.