What is a best practice for making a class's properties thread safe?

Question

[Edit: It looks like the original question involved a double and not an integer. So I think this question stands if we change the integer to a double.]

I have rare issue with reading integer properties from a class used in multiple threads that sometimes returns a zero value. The values are not changed after initialization.

This question addresses that. The consensus is that even though I'm accessing an integer I need to synchronize the properties. (Some of the original answers have been deleted). I haven't chosen an answer there because I have not resolved my issue yet.

So I’ve done some research on this and I’m not sure which of .Net 4’s locking mechanisms to use or if the locks should be outside the class itself.

This is what I thought about using:

  public class ConfigInfo
  {
    private readonly object TimerIntervalLocker = new object();
    private int _TimerInterval;
    public int TimerInterval
    {
      get
      {
        lock (TimerIntervalLocker) {
          return _TimerInterval;
        }
      }
    }

    private int _Factor1;
    public int Factor1
    {
      set
      {
        lock (TimerIntervalLocker) {
          _Factor1 = value;
          _TimerInterval = _Factor1 * _Factor2;
        }
      }
      get
      {
        lock (TimerIntervalLocker) {
          return _Factor1;
        }
      }
    }

    private int _Factor2;
    public int Factor2
    {
      set
      {
        lock (TimerIntervalLocker) {
          _Factor2 = value;
          _TimerInterval = _Factor1 * _Factor2;
        }
      }
      get
      {
        lock (TimerIntervalLocker) {
          return _Factor2;
        }
      }
    }
  }

But I’ve read that this is horribly slow.

Another alternative is to lock the instance of ConfigData on the user side but that seems to be a lot of work. Another alternative I’ve seen is Monitor.Enter and Monitor.Exit but I think Lock is the same thing with less syntax.

So what is a best practice for making a class's properties thread safe?

Answer 1

It is best, as mentioned in the comments, to make the properties readonly. I thought about the following possibility:

public class ConfigInfo
{
    private class IntervalHolder
    {
        public static readonly IntervalHolder Empty = new IntervalHolder();

        private readonly int _factor1;
        private readonly int _factor2;
        private readonly int _interval;

        private IntervalHolder()
        {
        }

        private IntervalHolder(int factor1, int factor2)
        {
            _factor1 = factor1;
            _factor2 = factor2;
            _interval = _factor1*_factor2;
        }

        public IntervalHolder WithFactor1(int factor1)
        {
            return new IntervalHolder(factor1, _factor2);
        }

        public IntervalHolder WithFactor2(int factor2)
        {
            return new IntervalHolder(_factor1, factor2);
        }

        public int Factor1
        {
            get { return _factor1; }
        }

        public int Factor2
        {
            get { return _factor2; }
        }

        public int Interval
        {
            get { return _interval; }
        }

        public override bool Equals(object obj)
        {
            var otherHolder = obj as IntervalHolder;
            return 
                otherHolder != null &&
                otherHolder._factor1 == _factor1 &&
                otherHolder._factor2 == _factor2;
        }
    }

    private IntervalHolder _intervalHolder = IntervalHolder.Empty;

    public int TimerInterval
    {
        get { return _intervalHolder.Interval; }
    }

    private void UpdateHolder(Func<IntervalHolder, IntervalHolder> update)
    {
        IntervalHolder oldValue, newValue;
        do
        {
            oldValue = _intervalHolder;
            newValue = update(oldValue);

        } while (!oldValue.Equals(Interlocked.CompareExchange(ref _intervalHolder, newValue, oldValue)));
    }

    public int Factor1
    {
        set { UpdateHolder(holder => holder.WithFactor1(value)); }
        get { return _intervalHolder.Factor1; }
    }

    public int Factor2
    {
        set { UpdateHolder(holder => holder.WithFactor2(value)); }
        get { return _intervalHolder.Factor2; }
    }
}

This way, your TimerInterval value is always in sync with its factors. The only problem is when some thread reads one of the properties while another writes them from outside the ConfigInfo. The first one could get wrong value and I don't see any way to solve this without introducing a single lock root. The question is whether read operations are critical.

Answer 2

a. Using lock can be slow since it uses operating system resources, if the properties' complexity is low, then spin lock (or interlocked.compareexchange) will be faster.

b. You have to make sure that a thread won't enter a lock and via a call from one property to another get locked out. - If this can happen (non currently an issue in your code), you'll need to make the lock thread or task sensitive.

Edit:

If the object is supposed to be set during initialization and never changed, make it immutable (like .NET strings are). Remove all the public setters and provide a constructor with parameters for defining the initial state and perhaps additional methods/operators for creating a new instance with a modified state (e.g. var newString = "Old string" + " was modified.";).

Answer 3

I think you should rewrite your ConfigInfo class to look like this; then you can't get overflow or threading problems:

public sealed class ConfigInfo
{
    public ConfigInfo(int factor1, int factor2)
    {
        if (factor1 <= 0)
            throw new ArgumentOutOfRangeException("factor1");

        if (factor2 <= 0)
            throw new ArgumentOutOfRangeException("factor2");

        _factor1 = factor1;
        _factor2 = factor2;

        checked
        {
            _timerInterval = _factor1*_factor2;
        }
    }

    public int TimerInterval
    {
        get
        {
            return _timerInterval;
        }
    }

    public int Factor1
    {
        get
        {
            return _factor1;
        }
    }

    public int Factor2
    {
        get
        {
            return _factor2;
        }
    }

    private readonly int _factor1;
    private readonly int _factor2;
    private readonly int _timerInterval;
}

Note that I'm using checked to detect overflow problems.

Otherwise some values will give incorrect results.

For example, 57344 * 524288 will give zero in unchecked integer arithmetic (and there's very many other pairs of values that will give zero, and even more that will give a negative result or a positive value that "seems" correct).

Answer 4

If the values never change, it would be easier to just make a copy of that instance and pass each thread an instance of it's own. No locking required at all.