Again on using interlocked

I read a lot of answers on this point but I did not found the solution yet.

I have a class with a counter attribute having a problem with cached values. Even volatile don't seems to work:

public class MyClass {
    private Timer _timer;
    private int _threadsCounter = 0;
    public StreamWriter Tracer { get; set; }

    public MyClass() {
        _timer = new Timer(1000.0 * 10);
        _timer.AutoReset = true;
        _timer.Elapsed += new ElapsedEventHandler(OnTimer);
        _timer.Start();
    }

    private void OnTimer(object sender, ElapsedEventArgs e) {
        HashSet<Task> taskPool = new HashSet<Task>();
        try {
            if (Tracer != null) Tracer.WriteLine("[{0}] onTimer start. Current threads counter is {1}.", DateTime.Now, _threadsCounter);
            if (_threadsCounter >= 10) return;

            // create parallel tasks
            for (int i = 0; i < 8; i++) {
                // limit on the max num of parallel processing but the counter remains unchanged during this timer event!!!
                if (_threadsCounter >= 10) break;

                var timeout = (30 + i * 2);
                var task = Task.Run(() => {
                        var localCounter = System.Threading.Interlocked.Increment(ref _threadsCounter);
                        try {
                            System.Threading.Thread.Sleep(timeout * 1000);
                        }
                        finally {
                            System.Threading.Interlocked.Decrement(ref _threadsCounter);
                        }
                    });
                taskPool.Add(task);
            }

        }
        finally {
            if (Tracer != null) 
                Tracer.WriteLine("[{0}] onTimer end. Created {1} tasks. Current threads counter is {2}.", DateTime.Now, taskPool.Count, _threadsCounter);
        }
    }

Well, it seems that the onTimer caches the _threadsCounter variable as the output is:

[14:10:47] onTimer start. Current threads counter is 0.
[14:10:47] onTimer end. Created 8 tasks. Current threads counter is 0.

[14:10:57] onTimer start. Current threads counter is 8.
[14:10:57] onTimer end. Created 8 tasks. Current threads counter is 8.

[14:11:07] onTimer start. Current threads counter is 16.
[14:11:07] onTimer end. Created 0 tasks. Current threads counter is 16.

[14:11:17] onTimer start. Current threads counter is 15.
[14:11:17] onTimer end. Created 0 tasks. Current threads counter is 15.

[14:11:37] onTimer start. Current threads counter is 4.
[14:11:37] onTimer end. Created 8 tasks. Current threads counter is 4.

[14:11:47] onTimer start. Current threads counter is 8.
[14:11:47] onTimer end. Created 8 tasks. Current threads counter is 8.

[14:11:57] onTimer start. Current threads counter is 16.
[14:11:57] onTimer end. Created 0 tasks. Current threads counter is 16.

Why do I arrive to 16? I solved the problem by changing a bit the code:

var localCounter = _threadsCounter;
...
if ((localCounter + taskPool.Count) >= 10) break;

But why this behavior?

Jon Skeet
people
quotationmark

You're effectively testing for the number of tasks that have actually started and got as far as incrementing the counter. That takes a little while - so basically you're creating all 8 tasks and starting them, then they're incrementing the counter... by which time it's too late to notice that you've got more than 10.

The better solution would be to increment the counter before you start the task:

// Increment the counter in expectation of starting a task
var localCounter = Interlocked.Increment(ref _threadsCounter);
if (localCounter >= 10)
{
    // Ah, we're not going to start a task after all, so undo
    // the increment
    Interlocked.Decrement(ref _threadsCounter);
    break;
}
else 
{
    // Start a task, which will decrement the counter at the end.
    // (You could add the "decrement" bit as a continuation, even...)
}

people

See more on this question at Stackoverflow