How to update my model counter value using Interlocked.Add

Question

So I have this model:

public class Container : INotifyPropertyChanged
{
    private int _total;
    private static InjectionContainer _mainContainer = new InjectionContainer();
    private static InjectionContainer _secondContainer = new InjectionContainer();
    private ObservableCollection<MyData> _files = new ObservableCollection<MyData>();

    public int TotalPackets
    {
        get { return _total; }
        set
        {
            _total = value;
            OnPropertyChanged("Total");
        }
    }

    public ObservableCollection<MyData> List
    {
        get { return _files; }
        set { _files = value; }
    }
}

And outside of this Container class I want to update my class Total property but I need it to be thread safe cause many thread do it at the same time:

public static void UpdateTotal(Container container, int value)
{
    Interlocked.Add(ref container.Total, value);
}

And got this error:

A property or indexer may not be passed as an out or ref parameter

Why not make it part of `Container` and supply an `Add` method? — Jamiec, Feb 08 '18 at 10:01
@Matt the first parameter of `Interlocked.Add` must be `ref` — Jamiec, Feb 08 '18 at 10:02
I assume you mean `ref container.TotalPackets`? As the error states, you cannot pass a property reference because (just as in your case), it's not a primitive value you're passing but a reference to the setter (that is actually a method) that might incur other side effects. You should pass a reference to the `_total` field instead (you'll have to adjust your code a bit to expose it, though, or simply move the `Interlocked.Add` call into the setter) — haim770, Feb 08 '18 at 10:03
Can you update your code to be consistent? At the moment you are referring to `container.Total` when your container doesn't have a member called `Total`. This makes it very hard to understand what you are actually doing. We can make some guesses but those guesses may not actually be what your code has... Additionally you should clarify what part of the error message you don't understand. It is a pretty clear statement and I'm personally unsure what scope there is for not understanding it... — Chris, Feb 08 '18 at 10:12

Christoph Fink · Accepted Answer · 2018-02-08T10:30:03.960

4

You should create an Add method inside the Container:

public class Container : INotifyPropertyChanged
{
    private int _total;
    private static InjectionContainer _mainContainer = new InjectionContainer();
    private static InjectionContainer _secondContainer = new InjectionContainer();
    private ObservableCollection<MyData> _files = new ObservableCollection<MyData>();

    public int TotalPackets
    {
        get { return _total; }
    }

    public ObservableCollection<MyData> List
    {
        get { return _files; }
        set { _files = value; }
    }

    public void AddTotal(int value)
    {
        Interlocked.Add(ref _total, value);
        OnPropertyChanged("TotalPackets");
    }
}

You can't add the Interlocked.Add(ref _total, value); into the setter because the needed usage pattern would then still be non-thread-save:

var total = container.TotalPackets; // #1
total += 10; // #2
container.TotalPackets = total; // #3

There the setting of the new total value in #3 on its own would be thread-safe, but between #1 and #3 some other thread could already have changed the total value. If we think of two threads and a start total of 10 the following execution order could happen:

Thread 1 - #1 ==> read 10
Thread 1 - #2 ==> total is set to 20
Thread 2 - #1 ==> read 10, as thread 1 has not run #3 yet
Thread 1 - #3 ==> TotalPackets is set to 20 (initial 10 + 10 from #2/2.)
Thread 2 - #2 ==> total is set to 20 as at 3. it was still 10
Thread 2 - #3 ==> TotalPackets is set to 20 again => boom ;-)

edited Feb 08 '18 at 10:30

answered Feb 08 '18 at 10:05

Christoph Fink

22,727
9
68
113

Why not simply move the call to `Interlocked.Add()` into the setter? – haim770 Feb 08 '18 at 10:06
@haim770 Because that would still not be thread-safe. If you read the value, then increment it and set it back it could already have changed... – Christoph Fink Feb 08 '18 at 10:07
1

Plus... the setter `value` is an absolute value, and if you now had `myContainer.TotalPackages = 2` to mean "Add 2" that would be *weird* – Jamiec Feb 08 '18 at 10:10
I failed to notice that it's `AddTotal()` and not `SetTotal()`. Obviously, they're not the same – haim770 Feb 08 '18 at 10:10
@ChrFin I can see how putting `Interlocked.Add()` in the setter would cause what @Jamiec said, but I don't understand how `Interlocked.Add(ref _total, value)` in the setter isn't thread safe. **EDIT**: the assignment part is done with the call to `Interlocked.Add()`, right? – cosh Feb 08 '18 at 10:21
@ChrFin, I see, so it's not that the setter isn't thread-safe, but the **usage** of the setter is. So for example, `container.TotalPackets = 10` would add 10 to _total as atomically as calling `AddTotal(10)`, right? And another nitpick, `container.TotalPackets = total;` would set total to 30 (10 start, 10 incremented, then another 10 on account to the call to `Interlocked.Add()` inside the setter), not 20. Other than that, +1, great explanation, and thanks. :) – cosh Feb 08 '18 at 10:42

How to update my model counter value using Interlocked.Add

1 Answers1