CODEWITHSUNDEEP
c#.netmultithreadinglocking
BetaKeja
BetaKeja

Reputation: 472

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

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?

Upvotes: 2

Views: 385

Answers (2)

Servy
Servy

Reputation: 203844

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.

Upvotes: 4

BetaKeja
BetaKeja

Reputation: 472

I realized that reading the lock object's field is done before we can lock on it; so this will never be correct. If another thread attempts to enter the lock before the value is changed it will end up entering its lock block with a lock on the old value but the field will have the new value. It will then be using the new value without having a lock on it.

Example:

  1. Thread A calls ReplaceList and enters the lock.
  2. Thread B calls Add. It reaches the lock and is blocked.
  3. Thread A replaces lockedList with a new value.
  4. Thread C calls Contains, it gets the new value of lockedList and takes out the lock.
  5. Thread A exits its lock, allowing thread B to resume.
  6. Thread B enters the lock using the old value of lockedList and adds an item to the new list without having a lock on the new list.
  7. Thread C throws an exception because the list was modified while it was enumerating it.

Upvotes: 2

Related Questions