C# Timed buffer

Question

Purpose of this code is to have buffer which will batch messages based on configured timeout. For example, one thread will continuously push data to this buffer, and when configured timeout pass, it will trigger event to process data (like batch and send to external source).

BufferReadyEventArgs.cs

public class BufferReadyEventArgs<T> : EventArgs
{
    public BufferReadyEventArgs(List<T> buffer)
    {
        Buffer = buffer ?? throw new ArgumentNullException(nameof(buffer));
    }

    public List<T> Buffer { get; }
}

TimedBuffer.cs

public class TimedBuffer<T>
{
    public event EventHandler<BufferReadyEventArgs<T>> BufferReady;

    private readonly object _bufferElapsedSyncRoot = new object();
    private readonly object _bufferSyncRoot = new object();
    private readonly Timer _processBufferTimer;
    private volatile List<T> _buffer = new List<T>();

    public TimedBuffer(double intervalInMsecs)
    {
        _processBufferTimer = new Timer(intervalInMsecs);
        _processBufferTimer.Elapsed += _processBufferTimer_Elapsed;
        _processBufferTimer.Enabled = true;
    }

    private void _processBufferTimer_Elapsed(object sender, ElapsedEventArgs e)
    {
        lock (_bufferElapsedSyncRoot)
        {
            List<T> readerList = _buffer;
            lock (_bufferSyncRoot)
            {
                _buffer = new List<T>();
            }

            BufferReady?.Invoke(this, new BufferReadyEventArgs<T>(readerList));
        }
    }

    public void Add(T item)
    {
        lock (_bufferSyncRoot)
        {
            _buffer.Add(item);
        }
    }
}

I would like to have code review for:

• General (like naming, design, code style and etc)
• Performance (Especially locking part)

Answer 1

An event-based design might cause some problems:

• It allows for multiple event handlers, which all get access to the same list. Lists are mutable, so depending on what these event handlers do they could end up with shared mutable state.
• It's possible to lose data by not registering an event handler, or by registering too late.

A more robust approach would be to replace that event with a callback that must be passed via the buffer's constructor.

---

Other issues:

• Raising an event while holding a lock is a bad idea. In this case, you end up blocking (wasting) threadpool threads if your event handlers take longer than the interval you specified.
• The only use of _buffer that's not protected by a lock on _bufferSyncRoot is fenced in between two lock statements, so if I understand things correctly then there's no point in making _buffer volatile. I would move that assignment inside of the lock just to be sure, and remove that volatile marker.