Can locking a List fail

Question

I have a class that inherits from MemoryStream in order to provide some buffering. The class works exactly as expected but every now and then I get an InvalidOperationException during a Read with the error message being

Collection was modified; enumeration operation may not execute.

My code is below and the only line that enumerates a collection would seem to be:

m_buffer = m_buffer.Skip(count).ToList();

However I have that and all other operations that can modify the m_buffer object within locks so I'm mystified as to how a Write operation could interfere with a Read to cause that exception?

public class MyMemoryStream : MemoryStream
{
    private ManualResetEvent m_dataReady = new ManualResetEvent(false);
    private List<byte> m_buffer = new List<byte>();

    public override void Write(byte[] buffer, int offset, int count)
    {
        lock (m_buffer)
        {
            m_buffer.AddRange(buffer.ToList().Skip(offset).Take(count));
        }
        m_dataReady.Set();
    }

    public override int Read(byte[] buffer, int offset, int count)
    {
        if (m_buffer.Count == 0)
        {
            // Block until the stream has some more data.
            m_dataReady.Reset();
            m_dataReady.WaitOne();
        }

        lock (m_buffer)
        {
            if (m_buffer.Count >= count)
            {
                // More bytes available than were requested.
                Array.Copy(m_buffer.ToArray(), 0, buffer, offset, count);
                m_buffer = m_buffer.Skip(count).ToList();
                return count;
            }
            else
            {
                int length = m_buffer.Count;
                Array.Copy(m_buffer.ToArray(), 0, buffer, offset, length);
                m_buffer.Clear();
                return length;
            }
        }
    }
}

Answer 1

I cannot say exactly what's going wrong from the code you posted, but a bit of an oddity is that you lock on m_buffer, but replace the buffer, so that the collection locked is not always the collection that is being read and modified.

It is good practice to use a dedicated private readonly object for the locking:

private readonly object locker = new object();

    // ...
    lock(locker)
    {
         // ...
    }

Answer 2

You have at least one data race there: on the Read method , if you're pre-empted after the if(m_buffer.Count == 0) block and before the lock, Count can be 0 again. You should check the count inside the lock, and use Monitor.Wait, Monitor.Pulse and/or Monitor.PulseAll for the wait/signal coordination, like this:

// On Write
lock(m_buffer)
{
    // ...
    Monitor.PulseAll();
}

// On Read
lock(m_buffer)
{
    while(m_buffer.Count == 0)
        Monitor.Wait(m_buffer);
// ...

You have to protect all accesses to m_buffer, and calling m_buffer.Count is not special in that regard.

Answer 3

Do you modify the content of buffer in another thread somewhere, I suspect that may be the enumeration giving error rather than m_buffer.