Reputation: 132568
I have some code that loops through a list of records, starts an export task for each one, and increases a progress counter by 1 each time a task finishes so the user knows how far along the process is.
But depending on the timing of my loops, I often see the output showing a higher number before a lower number.
For example, I would expect to see output like this:
Exporting A Exporting B Exporting C Exporting D Exporting E Finished 1 / 5 Finished 2 / 5 Finished 3 / 5 Finished 4 / 5 Finished 5 / 5
But instead I get output like this
Exporting A Exporting B Exporting C Exporting D Exporting E Finished 1 / 5 Finished 2 / 5 Finished 5 / 5 Finished 4 / 5 Finished 3 / 5
I don't expect the output to be exact since I'm not locking the value when I update/use it (sometimes it outputs the same number twice, or skips a number), however I wouldn't expect it to go backwards.
My test data set is 72 values, and the relevant code looks like this:
var tasks = new List<Task>();
int counter = 0;
StatusMessage = string.Format("Exporting 0 / {0}", count);
foreach (var value in myValues)
{
var valueParam = value;
// Create async task, start it, and store the task in a list
// so we can wait for all tasks to finish at the end
tasks.Add(
Task.Factory.StartNew(() =>
{
Debug.WriteLine("Exporting " + valueParam );
System.Threading.Thread.Sleep(500);
counter++;
StatusMessage = string.Format("Exporting {0} / {1}", counter, count);
Debug.WriteLine("Finished " + counter.ToString());
})
);
}
// Begin async task to wait for all tasks to finish and update output
Task.Factory.StartNew(() =>
{
Task.WaitAll(tasks.ToArray());
StatusMessage = "Finished";
});
The output can appear backwards in both the debug statements and the StatusMessage
output.
What's the correct way to keep count of how many async tasks in a loop are completed so that this problem doesn't occur?
Upvotes: 5
Views: 5137
Reputation: 754863
In this sample the counter
variable represents shared state among several threads. Using the ++
operator on shared state is simply unsafe and will give you incorrect results. It essentially boils down to the following instructions
Because multiple threads are executing this statement it's possible for one to interrupt the other partway through completing the above sequence. This would cause the incorrect value to end up in counter
.
Instead of ++
use the following statement
Interlocked.Increment(ref counter);
This operation is specifically designed to update state which may be shared among several threads. The interlocked will happen atomically and won't suffer from the race conditions I outlined
The actual out of order display of values suffers from a similar problem even after my suggested fix. The increment and display operation aren't atomic and hence one thread can interrupt the other in between the increment and display. If you want the operations to be un-interruptable by other threads then you will need to use a lock.
object lockTarget = new object();
int counter = 0;
...
lock (lockTarget) {
counter++;
StatusMessage = string.Format("Exporting {0} / {1}", counter, count);
Debug.WriteLine("Finished " + counter.ToString());
}
Note that because the increment of counter
now occurs inside the lock there is no longer a need to use Interlocked.Increment
Upvotes: 5
Reputation: 12654
You get mixed output, because counter is not incremented in the same order as Debug.WriteLine(...)
method is executed.
To get a consistent progress report, you can introduce a reporting lock into the task
tasks.Add(
Task.Factory.StartNew(() =>
{
Debug.WriteLine("Exporting " + valueParam );
System.Threading.Thread.Sleep(500);
lock(progressReportLock)
{
counter++;
StatusMessage = string.Format("Exporting {0} / {1}", counter, count);
Debug.WriteLine("Finished " + counter.ToString());
}
})
);
Upvotes: 7