Is it bad to overwrite a lock object if it is the last statement in the lock?

Question

I've seen this a couple times now but I'm not sure if it is actually incorrect.

Consider the following example class:

class Foo
{
    List<string> lockedList = new List<string>();

    public void ReplaceList(IEnumerable<string> items)
    {
        var newList = new List<string>(items);

        lock (lockedList) 
        {
            lockedList = newList;
        }
    }

    public void Add(string newItem)
    {
        lock (lockedList)
        {
            lockedList.Add(newItem);
        }
    }

    public void Contains(string item)
    {
        lock (lockedList)
        {
            lockedList.Contains(item);
        }
    }
}

ReplaceList overwrites lockedList while it has a lock on it. As soon as it does all subsequent callers will actually be locking on the new value. They can enter their lock before ReplaceList exits its lock.

Despite raising flags by replacing a lock object, this code might actually work correctly. As long as the assignment is the last statement of the lock there is no more synchronized code to run.

Besides the increased maintenance cost of ensuring the assignment remains at the end of lock block, is there another reason to avoid this?

I can't think of any reason why it would fail, but it still makes me uncomfortable. — Sam I am says Reinstate Monica
Depends what you mean by "correctly". Within the lock block you have a lock on the object that lockedList initially points to. You're not transferring the lock to newList, you never have a lock on that object. So as long as your code doesn't assume otherwise, it's fine. — Blorgbeard
Whether or not this is correct is going to vary wildly based on information not shown. It will depend on what all is shared across threads, and how all of the various code uses those shared objects. This could be fine, or it could not be, there's no possible way of knowing. — Servy
You get a warning from the C# compiler for this, CS0728, be sure to try it. To remind you that you are doing it wrong. But does point out that the compiler is smart enough to not let this go wrong, it makes a copy of lockedList to an otherwise invisible variable. — Hans Passant
While not the same question, Eric Lippert answered a similar question asked at almost exactly the same time and went on to explain why this construction is a bad idea and what the proper way to use locks with mutable variables is: stackoverflow.com/a/46854307/517852 — Mike Zboray

Servy Servy · Accepted Answer · 2017-10-20T21:20:03

So, to start with, no the specific solution that you have provided is not safe, due to the specifics of how you're accessing the field.

Getting a working solution is easy enough. Just don't create a new list; instead, clear it out and add the new items:

class Foo
{
    private List<string> lockedList = new List<string>();

    public void ReplaceList(IEnumerable<string> items)
    {
        lock (lockedList)
        {
            lockedList.Clear();
            lockedList.AddRange(items);
        }
    }

    public void Add(string newItem)
    {
        lock (lockedList)
        {
            lockedList.Add(newItem);
        }
    }

    public void Contains(string item)
    {
        lock (lockedList)
        {
            lockedList.Contains(item);
        }
    }
}

Now your field isn't actually changing, and you don't need to worry about all of the problems that that can cause.

As for how the code in the question can break, all it takes is a call to Add or Contains to read the field, get the list, lock the list, and then have another thread replace the field. The when you read the field for a second time, after already getting the value to lock on, the value may have changed, so you'll end up mutating or reading from a list that another caller won't be restricted from accessing.

All that said, while mutating the lockedList variable is a really bad idea, and you should unquestionably avoid mutating it, as shown above, you can also ensure that you only actually read the field once, rather than reading from it repeatedly, and you'll still ensure that each list is only ever accessed from a single thread at any one time:

class Foo
{
    private volatile List<string> lockedList = new List<string>();

    public void ReplaceList(IEnumerable<string> items)
    {
        lockedList = new List<string>(items);
    }

    public void Add(string newItem)
    {
        var localList = lockedList;
        lock (localList)
        {
            localList.Add(newItem);
        }
    }

    public void Contains(string item)
    {
        var localList = lockedList;
        lock (localList)
        {
            localList.Contains(item);
        }
    }
}

Notice here that the problem that this fixes isn't mutating the field that the object to lock on was fetched from (that's not inherently the problem, although is a very bad practice), but rather constantly getting new values from the field in all usages of it from inside of the lock statements and expecting that value to never change, when it can.

This is going to be much harder to maintain, is very fragile, and is much more difficult to understand or ensure the correctness of though, so again, do do things like this.

Is it bad to overwrite a lock object if it is the last statement in the lock?

2 Answers