C# Is locking within getters and setters necessary?

Question

Is it necessary to lock getter and setters in general when multiple threads will access a property/field through get/set-functions.

For instance:

You have a timer which regular gets the value of an object. And a process which regular updates this value.

public class Task : IProgressProvider
{
    ...
    public Progress GetProgress()
    {
        // should i lock here?
        return _progress;
    }

    public SetProgress(Progress progress)
    {
        // and lock here?
        _progress = progress;
    }

    public void Execute()
    {
        // ...
        SetProgress(new Progress(10));
        // ...
        SetProgress(new Progress(50));
        // ...
        SetProgress(new Progress(100));
    }
}

public class ProgressReporter
{
    public ProgressReporter()
    {
        _timer = new Timer();
        _timer.Elapsed += Elapsed;
        // ...
    }

    // ...

    public void Elapsed(...)
    {
        var progress = _task.GetProgress();
        // ...
    }
}

The question again:

• Should i lock in the GetProgress and SetProgress function.
• If yes, why?
• Is volatile necessary in this case (assuming one or multiple processor cores)
• To keep it "simple" lets assume the properties/fields from Progress are readonly. EDIT: I know that += and so on are not atomic operations and would require proper handling (f.e. locking)

I would think that the setting and reading from the variable _progress is an atomic operation. It is not important that the GetProgress will get the very very current value. If not this time it will get it on the next call of GetProgress.

Answer 1

If _progress is a reference type, then reading and writing its value (i.e. changing the reference) is indeed an atomic operation, so you wouldn't need to lock for the specific code in your example. But if you ever changed more than one field in the setter or getter, or if the field wasn't a type with atomic reads/writes (e.g. double) then you'd need to lock.

If you want several threads to observe the same value at any instant, then indeed you will need additional locking. If you don't care if one thread could read one value and another thread another value (because it changed inbetween) then locking doesn't really seem necessary.

I would make it volatile for sure. volatile is for exactly that purpose. It will at least prevent the optimising compiler from caching the value when it shouldn't (in case it would ever do such a thing).

So to summarise: For your required usage, you should make _progress volatile, but you don't need to lock access to it.

Answer 2

Based on your method signatures, it looks like you only have one thread that will be updating the status. If so, you don't need any locks on the SetProgress.

If you do have multiple threads updating the Progress variable, you still wouldn't need to have a lock in the set function, as it's atomic (assuming it's a reference variable, which it looks like).

However, if you read the value and then add a number to it (e.g. take the current progress and increase it by 10) then you will need a lock around that call. If this is your intent, since your calling objects shouldn't really be responsible for the integrity of this object, I would suggest creating a method that handles the update.

    public Progress IncrProgress(int incr)
    {
        lock (_progressLock)
        {
            // Get the current progress
            int current = _progress.GetPercentage();
            current += incr;
            _progress = new Progress(current);
        }
        return _progress;
    }

As for your other question, _progress should be flagged as volatile, as it's updated by another thread.

http://msdn.microsoft.com/en-us/library/x13ttww7(v=vs.71).aspx