Am I implementing this buffer using a C# Timer correctly?

Question

I have a service that subscribes to updates to a repository. When an update message is received, the service needs to reload some data from the repository.

However many update messages can be received in a short period of time. So I want to create a buffer / time window, that will mean only one reload will happen for that period were many update messages arrived.

I've created a very rough outline:

class TestService
{
    private Timer scheduledReloadTimer;

    public void AttemptReload()
    {
        if (scheduledReloadTimer == null)
        {
            Console.WriteLine("Scheduling reload...");

            scheduledReloadTimer = new Timer(Reload, null, 10000, Timeout.Infinite);
        }
        else
        {
            Console.WriteLine("Reload already scheduled for this period...");
        }
    }

    private void Reload(object stateInfo)
    {
        scheduledReloadTimer.Dispose();
        scheduledReloadTimer = null;

        Console.WriteLine("Doing reload..");
    }
}

Is using the null check on the Timer good enough to see if a reload has already been scheduled?

Am I disposing the Timer correctly?

Is there anything else I am missing here, especially around thread safety?

I've seen another stackoverflow answer that suggests using the Reactive Extensions to achieve this: https://stackoverflow.com/a/42887221/67357 but is that overkill?

Answer 1

You do have a potential thread-safety issue here. A quick fix would be to create a thread lock scope around the critical parts of your code, to ensure that while you're inspecting/creating and setting the timer variable, no other thread can get in there and start the same process in parallel:

class TestService
{
    private Timer scheduledReloadTimer;
    private object timerLock = new object();

    public void AttemptReload()
    {
        lock (timerLock)
        {
            if (scheduledReloadTimer == null)
            {
                Console.WriteLine("Scheduling reload...");

                scheduledReloadTimer = new Timer(Reload, null, 10000, Timeout.Infinite);
            }
            else
            {
                Console.WriteLine("Reload already scheduled for this period...");
            } 
        }
    }

    private void Reload(object stateInfo)
    {
        lock (timerLock)
        {
            scheduledReloadTimer.Dispose();
            scheduledReloadTimer = null; 
        }

        Console.WriteLine("Doing reload..");
    }
}

Reactive Extensions are a good way to deal with this throttling issue - as the code is already written for you.

Another approach might be to modify the AttemptReload call to simply reset the interval on the timer (if the reloadTimer != null), essentially pushing back the invocation of the timer event with each subsequent call to AttemptReload.

That way, the timer will definitely not fire until after the last call to AttemptReload + 10,000 milliseconds.