How to synchronize a shared IProgress<int>

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);
}