CODEWITHSUNDEEP
c#multithreading
user3230660
user3230660

Reputation:

How do I use Interlocked.Add() correctly?

From the documentation here: "The methods of this class help protect against errors that can occur when the scheduler switches contexts while a thread is updating a variable that can be accessed by other threads..."

Also, an answer to this question states "INTERLOCKED METHODS ARE CONCURRENTLY SAFE ON ANY NUMBER OF COREs OR CPUs" which seems pretty clear.

Based on the above I thought Interlocked.Add() would be sufficient for multiple threads to do addition on a variable. Apparently I'm wrong or I'm using the method incorrectly. In the runnable code below I expect Downloader.ActiveRequestCount to be zero when Run() completes. If I do not lock around the call to Interlocked.Add I get a random non-zero result. What is the correct usage of Interlocked.Add()?

class Program
{
    private Downloader downloader { get; set; }

    static void Main(string[] args)
    {
        new Program().Run().Wait();
    }


    public async Task Run()
    {
        downloader = new Downloader();
        List<Task> tasks = new List<Task>(100);

        for (int i = 0; i < 100; i++)
            tasks.Add(Task.Run(Download));

        await Task.WhenAll(tasks);

        Console.Clear();
        //expected:0, actual when lock is not used:random number i.e. 51,115
        Console.WriteLine($"ActiveRequestCount is : {downloader.ActiveRequestCount}"); 
        Console.ReadLine();


    }

    private async Task Download()
    {
        for (int i = 0; i < 100; i++)
            await downloader.Download();
    }
}

public class Downloader :INotifyPropertyChanged
{
    private object locker = new object();
    private int _ActiveRequestCount;
    public int ActiveRequestCount { get => _ActiveRequestCount; private set => _ActiveRequestCount = value; }

    public async Task<string> Download()
    {
        string result = string.Empty;

        try
        {
            IncrementActiveRequestCount(1);
            result = await Task.FromResult("boo");
        }
        catch (Exception ex)
        {
            Console.WriteLine("oops");
        }
        finally
        {
            IncrementActiveRequestCount(-1);
        }

        return result;
    }


    public void IncrementActiveRequestCount(int value)
    {
        //lock (locker)       // is this redundant
        //{
            _ActiveRequestCount = Interlocked.Add(ref _ActiveRequestCount, value);
        //}

        RaisePropertyChanged(nameof(ActiveRequestCount));
    }

    #region INotifyPropertyChanged implementation
    public event PropertyChangedEventHandler PropertyChanged;
    public void RaisePropertyChanged([CallerMemberNameAttribute] string propertyName = "") => PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName));
    #endregion
}

Upvotes: 1

Views: 1350

Answers (1)

Heinzi
Heinzi

Reputation: 172230

Replace

_ActiveRequestCount = Interlocked.Add(ref _ActiveRequestCount, value);

with

Interlocked.Add(ref _ActiveRequestCount, value);

Interlocked.Add is thread-safe and takes a ref parameter, so that it can do the assignment safely. You additionally perform an (unnecessary) unsafe assignment (=). Just remove it.

Upvotes: 3

Related Questions