Handling a save on a separate thread

Question

I've got a class that contains (among many other things) a list of items that needs to be saved to disk as often as realistically possible. In order to allow this, whenever any change is made, it uses a timer to delay saving to disk by half a second, and if any further changes happen within that half second, it delays the timer; in other words, it'll save to disk whenever there's been no changes within half a second.

However, I'm hitting threading issues where the list could be modified in the middle of the save. I've come up with a way of fixing it, but it feels like a bit of a hack, and I'm fairly sure there has to be a better way.

The save method locks the thread on a private instance of 'object' during a save operation, but this doesn't stop the list being updated, as it's a separate class, and doesn't have access to that private lock.

So, to resolve this what I've done is added an 'about to change' event to the list class (not a bad feature for it to have anyway), and the containing class listens to this event, and locks on the same private lock as the save method, but takes no action. This forces the thread to wait until the save is complete before allowing the list to be updated, even though the list itself has no knowledge of it's containing object, or what it's doing.

Here's the (seriously simplified) source:

public class ItemContainer
{
  private ItemList _itemList = new ItemList();
  private object _padlock = new object();

  public ItemContainer()
  {
    _itemList.ItemAdded += StartTimedSave;
    _itemList.ItemAdding += (sender, e) => { lock(_padlock) { } };
  }

  private StartTimedSave()
  {
    // starts or resets a half second timer to call Save();
  }

  private void Save()
  {
    lock(_padlock)
    {
      foreach (object obj in _itemList)
      {
        // save it.
      }
    }
  }
}

public class ItemList
{
  public event EventHandler<CancelEventArgs> ItemAdding;
  public event EventHandler ItemAdded;

  public List<object> _list;

  public void Add(object obj)
  {
    if (ItemAdding.RaiseWithCancel(this)) // extension method
      return;
    _list.Add(obj);
    ItemAdded.Raise(this); // another extension method
  }
}

I can't realistically have the list hold a reference to it's containing object, as the list is a fairly generalized class that may or may not be contained by such an object.

Is an event handler with an empty lock { } block really the way to go with this? I'm uncomfortable with the notion of allowing an event handler to interfere with the operation of the object that raised it, but I can't see a better way.

Answer 1

this does nothing:

lock(_padlock) { }

Lock holds while block is executing - no point to lock empty block.

If all problem is with that private lock object, just make public getter for it. Make sure that anything that modifies or reads collection locks it.

Answer 2

When saving, make a copy of the list in memory and save that -- this will reduce the exposure of the data to updates in other threads.

To be thread-safe, ask the ItemList class to create and return the list copy and the ItemList class can lock the internal list long enough to make the copy:

public List<object> CreateListCopy()
{
    List<object> result = new List<object>();
    lock(_list)
    {
        foreach(object o in _list)
        {
            result.Add(o.Clone());
        }
    }

    return result;
}

Then you save the result of CreateListCopy() in your save thread.

EDIT: When copying the list of items, don't forget to ensure you are using deep copy semantics as you don't want to just be copying the references; you want full, completely separate copies of the data.