We found something interesting happening recently at work when reviewing the web exception logs on our servers.  There seemed to be an inordinate number of “Index out of range” errors occurring, and they were all coming from one class.

Digging into the code, we found that the error was occurring on a ArrayList.Contains(item) method, which seemed very odd. As I understood it, the whole point of the Contains() method is to check for the item before you try and access it, thereby avoiding the index out of range error.

A workmate thought that it may be a race condition problem, as this class is called on pretty much every page load (it sits in the page header) and the ArrayList itself was a static variable.  We weren’t sure how this could happen, so he cracked out Reflector and disassembled the Contains() method on the ArrayList object. And we found something interesting.

The Contains() method is nothing more than two for-loops:

public virtual bool Contains(object item)
{
      if (item == null)
      {
            for (int num1 = 0; num1 < this._size; num1++)
            {
                  if (this._items[num1] == null)
                  {
                        return true;
                  }
            }
            return false;
      }
      for (int num2 = 0; num2 < this._size; num2++)
      {
            if (item.Equals(this._items[num2]))
            {
                  return true;
            }
      }
      return false;
}

This is .NET 1.1 code. .NET 2.0 is almost identical, except the equality test inside the second for loop is a bit more comprehensive:
if ((this._items[num2] != null) && this._items[num2].Equals(item))

This code has absolutely no protection against any sort of race condition, and neither should it.  It’s not the framework’s job to protect you from this sort of thing, that’s the point of the lock statement.  If you call Contains() on a static ArrayList variable, and then another thread comes in and empties this list, one of the this._items[numX] calls will throw an index out of range error.

The best way to avoid this problem is to wrap any code accessing the static ArrayList in a lock block, which locks on a local static object variable.  From the MSDN reference on the lock statement:

In general, avoid locking on a public type, or instances beyond your code’s control. The common constructs lock (this), lock (typeof (MyType)), and lock ("myLock") violate this guideline:

• lock (this) is a problem if the instance can be accessed publicly.
• lock (typeof (MyType)) is a problem if MyType is publicly accessible.
• lock(“myLock”) is a problem since any other code in the process using the same string, will share the same lock.

Best practice is to define a private object to lock on, or a private static object variable to protect data common to all instances.

In the code causing this error, there was as lock statement surrounding the Contains() call. It was locking on a local private variable, which is in accordance with this spec.  However the class itself was designed to be accessed only in a singleton pattern, but in this case it was being instantiated directly through the constructor.  It would most likely have never even become an issue if it hadn’t been implemented on a control that’s used on every page, thus enabling the race condition to occur.

It’s been an interesting little exercise in debugging.  I think I learn more about .NET when the bizarre bugs occur than any amount of time spent on normal coding.  These stranger bugs make you dig into how the framework actually handles memory and other resources.