Is the following code reentrant and thread-safe?

Question

Is the following code is reentrant?

Is it thread-safe, if this.NextToExecuteIndex is declared private int NextToExecuteIndex = 0; and not computed anywhere else?

    protected override void Poll(object sender, System.Timers.ElapsedEventArgs e)
    {
        int index;

        object locker = new object();

        lock (locker)
        {
            Interlocked.Increment(ref this.NextToExecuteIndex);

            if (this.NextToExecuteIndex >= this.ReportingAgentsTypes.Count())
            {
                this.NextToExecuteIndex = 0;
            }

            index = this.NextToExecuteIndex;
        }

        var t = this.ReportingAgentsTypes[index];

        Console.WriteLine(t.ToString());
    }

Answer 1

No this is not thread-safe at all. The lock has no effect since the object is local to the thread. It needs to be shared by all calling threads. Once you fix that you don't need to use interlocked increment because the lock serialises execution.

As a general rule you should place locker at the same level as the resource you are protecting. If the resource is owned by the instance then so should be locker. Similarly if the resource is owned by the class.

As for re-entrancy, the lock keyword uses a re-entrant lock, i.e. one that lets the same thread in if the lock is held by that thread. That's probably not what you want. But if you had a non re-entrant lock then you would just deadlock yourself with a re-entrant call. And I don't think you'd want that either.

You look like you want a wrap around increment. So long as the collection is not being modified, this can be achieved with interlocked operations, i.e. lock free. If so then it can be written like this:

do
{
    int original = this.NextToExecuteIndex;
    int next = original+1;
    if (next == this.ReportingAgentsTypes.Count())
        next = 0;
}
while (Interlocked.CompareExchange(ref this.NextToExecuteIndex, next, original) != original);

Note: You should declare NextToExecuteIndex as volatile.

Answer 2

Absolutely not. This is your problem object locker = new object();. You will create a new object and lock on it every time.