How to synchronize a shared IProgress

Question

I have an asynchronous method DoStuffAsync that spawns two tasks with Task.Run, and both tasks report their progress using a single IProgress<int> object. From the user's perspective there is only one operation, so showing two progress bars (one for each Task) wouldn't make any sense. This is why the IProgress<int> is shared. The problem is that sometimes the UI receives the progress notifications in incorrect order. Here is my code:

private async void Button1_Click(object sender, EventArgs e)
{
    TextBox1.Clear();
    var progress = new Progress<int>(x => TextBox1.AppendText($"Progress: {x}\r\n"));
    await DoStuffAsync(progress);
}

async Task DoStuffAsync(IProgress<int> progress)
{
    int totalPercentDone = 0;
    Task[] tasks = Enumerable.Range(1, 2).Select(n => Task.Run(async () =>
    {
        for (int i = 0; i < 5; i++)
        {
            await Task.Delay(100); // Simulate an I/O operation
            var localPercentDone = Interlocked.Add(ref totalPercentDone, 10);
            progress.Report(localPercentDone);
        }
    })).ToArray();
    await Task.WhenAll(tasks);
}

Most of the time the notifications are in the correct order, but sometimes they are not:

This causes the ProgressBar control (not shown in the above screenshot) to jump awkwardly back and forth.

As a temporary solution I have added a lock inside the DoStuffAsync method, that includes the invocation of the IProgress.Report method:

async Task DoStuffAsync(IProgress<int> progress)
{
    int totalPercentDone = 0;
    object locker = new object();
    Task[] tasks = Enumerable.Range(1, 2).Select(n => Task.Run(async () =>
    {
        for (int i = 0; i < 5; i++)
        {
            await Task.Delay(100); // Simulate an I/O operation
            lock (locker)
            {
                totalPercentDone += 10;
                progress.Report(totalPercentDone);
            };
        }
    })).ToArray();
    await Task.WhenAll(tasks);
}

Although this solves the problem, it causes me anxiety because I invoke arbitrary code while holding a lock. The DoStuffAsync method is actually part of a library, and could be called with a whatever IProgress<int> implementation as argument. This opens the possibility for deadlock scenarios. Is there a better way to implement the DoStuffAsync method, without using a lock, but with the desired behavior regarding the ordering of the notifications?

Answer 1

Your problem is that you need the increment of totalPercentDone AND the call to Report to be atomic.

There's nothing wrong with using a lock here. After all, you need some way to make the two operations atomic. If you really don't want to use lock then you could use a SemaphoireSlim:

async Task DoStuffAsync(IProgress<int> progress)
{
    int totalPercentDone = 0;
    var semaphore =  new SemaphoreSlim(1,1);

    Task[] tasks = Enumerable.Range(1, 2).Select(n => Task.Run(async () =>
    {
        for (int i = 0; i < 5; i++)
        {
            await Task.Delay(100); // Simulate an I/O operation
            await semaphore.WaitAsync();

            try
            {
                totalPercentDone += 10;
                progress.Report(totalPercentDone);
            }
            finally
            {
                semaphore.Release();
            }
        }
    })).ToArray();

    await Task.WhenAll(tasks);
}

Answer 2

You could just report the deltas and let the handling handle them:

private async void Button1_Click(object sender, EventArgs e)
{
    TextBox1.Clear();
    var totalPercentDone = 0;
    var progress = new Progress<int>(x =>
        {
            totalPercentDone += x;
            TextBox1.AppendText($"Progress: {totalPercentDone}\r\n"));
        }
    await DoStuffAsync(progress);
}

async Task DoStuffAsync(IProgress<int> progress)
{
    await Task.WhenAll(Enumerable.Range(1, 2).Select(n => Task.Run(async () =>
    {
        for (int i = 0; i < 5; i++)
        {
            await Task.Delay(100); // Simulate an I/O operation
            progress.Report(10);
        }
    })));
}

Answer 3

Instead of using one int for both tasks you could use two individual ints, and take the smallest of them. Each Task need to report to 100, and not 50.

async Task DoStuffAsync(IProgress<int> progress)
{
    int[] totalPercentDone = new int[2];
    Task[] tasks = Enumerable.Range(1, 2).Select(n => Task.Run(async () =>
    {
        for (int i = 0; i < 5; i++)
        {
            await Task.Delay(100); // Simulate an I/O operation
            totalPercentDone[n - 1] += 10;

            progress.Report(totalPercentDone.Min());
        }
    })).ToArray();
    await Task.WhenAll(tasks);
}

Answer 4

This is to extend my comments under question

Basically, progress is usually a forward-only value. With regards to reporting progress, it is likely that you never need to report a progress made in the past. Even you do, in most cases the client / event handler side would still drop such values received.

The problem here / why you need to synchronize reporting is mainly because you are reporting a progress of value type, whose value got copied when Report(T) is called.

You can simply avoid locking by reporting a reference type instance with the latest progress made:

public class DoStuffProgress
{
    private volatile int _percentage;

    public int Percentage => _percentage;

    internal void IncrementBy(int increment)
    {
        Interlocked.Add(ref _percentage, increment);
    }
}

Now your code looks like:

async Task DoStuffAsync(IProgress<DoStuffProgress> progress)
{
    DoStuffProgress totalPercentDone = new DoStuffProgress();

    Task[] tasks = Enumerable.Range(1, 2).Select(n => Task.Run(async () =>
    {
        for (int i = 0; i < 5; i++)
        {
            await Task.Delay(100); // Simulate an I/O operation

            totalPercentDone.IncrementBy(10);

            // Report reference type object
            progress.Report(totalPercentDone);
        }
    })).ToArray();
    await Task.WhenAll(tasks);
}

The client, however, may receive notification with duplicate value:

Progress: 20
Progress: 20
Progress: 40
Progress: 40
Progress: 60
Progress: 60
Progress: 80
Progress: 80
Progress: 90
Progress: 100

But, the values should never be out of order.