Alfred Myers

Last month I was working on a bug in a service I developed a couple of months ago.

The service consists of a Windows Service that coordinates an unspecified amount of secondary Windows Services that are in charge of collecting web site metrics.

Metrics collection is made at intervals specified in a configuration file. During development and stabilization I used very short time intervals from one second to one minute to stress garbage collection and resource consumption. With the intervals tested, the services were working properly.

Earlier this week, the service was being deployed and the guy in charge reported that above two and half minutes, the services stopped collecting metrics.

I did some tests with a five minutes interval and everything worked fine, but when I increased the interval to ten minutes I started to get SynchronizationLockException.

The weird part was that the exception was being thrown by the closing curly brace of a lock statement:

public IEnumerator<INavigator> GetEnumerator() {

    while (!this.disposing) {

        if (this.availableNavigators.Count == 0) {

            this.synchronizer.WaitOne();

        }

        lock (this.availableNavigators) {

            if (this.availableNavigators.Count != 0) {

                yield return this.availableNavigators.Dequeue();

            }

        } // The exception was being throw here.

    }

}

I went on to investigate the situations in which a SynchronizationLockExceptions could be thrown only to learn that:

“The exception that is thrown when a method requires the caller to own the lock on a given Monitor, and the method is invoked by a caller that does not own that lock.

…

SynchronizationLockException is thrown by calling the Exit, Pulse, PulseAll, and Wait methods of the Monitor class from an unsynchronized block of code.”

Putting it another way: The thread that calls Monitor.Exit has to be the same that called Monitor.Enter in the first place.

Well… What does this has to do with our case? Being a smart reader as you are, you may already know that a lock statement gets translated into calls to Monitor.Enter and Monitor.Exit by the C# compiler. From the C# spec:

“A lock statement of the form

lock (x) …

where x is an expression of a reference-type, is precisely equivalent to

System.Threading.Monitor.Enter(x);
try {
      …
}
finally {
      System.Threading.Monitor.Exit(x);
}

except that x is only evaluated once.”

Well… Even then the code should be running on the same thread between the Enter and the Exit.

Let’s take another look to our code – this time with calls to Monitor instead of using lock directly.

public IEnumerator<INavigator> GetEnumerator() {

    while (!this.disposing) {

        if (this.availableNavigators.Count == 0) {

            this.synchronizer.WaitOne();

        }

        object localReference = this.availableNavigators;

        Monitor.Enter(localReference);

        try {

            if (this.availableNavigators.Count != 0) {

                yield return this.availableNavigators.Dequeue();

            }

        }

        finally {

            Monitor.Exit(localReference);

        }

    }

}

This code transformation ringed a bell. Besides lock, “yield return” also gets translated into a set of other instructions. The generated code is indeed much more sophisticated than the lock case since it involves generating a state machine for managing the return point during subsequent calls of the method.

If you haven’t had a chance to use yield return or even study it, it’s a very interesting construct that permits you return a value and the next time you enter the method, code starts executing at the line immediately after the last line executed the last time you were in the method (the yield return line).

Under the covers, the C# compiler creates a new class that implements a state machine that keeps track of the line to be executed the next time the code enters the method.

So in our example, the code exits the method before passing through Monitor.Exit (it’s already starting to smell bad), but when coming back it is still the same thread. Or isn’t it?

Looking through the stack I find that this code is being called by a System.Threading.Timer which uses the thread pool to do its work.

Now a little parenthesis about threads and the thread pool: When you need to do work asynchronously it’s common to spawn a thread to do the work and eventually get the results later.

Creating thread is an expensive operation. You have to create kernel objects, allocate some space for the stack and some other stuff so it has become widespread use creating threads and instead of destroying it after it completes its job, we return it to a thread pool so it can be used later for accomplishing another task.

The .Net’s thread pool is smart enough to create new threads when needed and also to destroy them when they are no longer needed.

When you have little bursts of work at small intervals between them, the chances are that each job will be accomplished by the same thread.

On the other hand, if there is a long interval between job batches, the chances are that the thread becomes idle for too long and then the CLR gets rid of it.

So what was happening in this case was that once somebody configured a long enough interval the code was being executed by another thread causing a SynchronizationLockException to be thrown.

To get rid of the exception, I had to move the “yield return” out of the lock block so the code became like:

public IEnumerator<INavigator> GetEnumerator() {

    while (!this.disposing) {

        INavigator navigator = null;

        if (this.availableNavigators.Count == 0) {

            this.synchronizer.WaitOne();

        }

        lock (this.availableNavigators) {

            if (this.availableNavigators.Count != 0) {

                navigator = this.availableNavigators.Dequeue();

            }

        }

  yield return navigator;

    }

}

The lesson to be learned is that “yield return” should be used cautiously, even more when used inside constructs that get translated into other code as is the case with “lock” and “using” since the result may not be exactly what we are expecting.