Threading and Singletons

Question

Before anyone flips out on me about singletons, I will say that in this instance it makes sense for me to have a singleton given the wide use of this object throughout my code and if someone has a better way short of DI I would like to hear but I would hope that this would not be the focus of this post, moreso helping solve it would be.

That being said here's the issue: It seems after a given amount of time that I am losing a reference to my class scheduler and inside there is a timer tick that no longer fires. Is this because it is being used in a singleton fashion and once it loses a reference it is GC'd?

using System;
using System.Collections.Generic;
using System.Linq;
using System.Windows.Forms;

namespace MessageQueueInterface
{
    public class Scheduler
    {
        private const int _interval = 1000;
        private readonly Dictionary<DateTime, Action> _scheduledTasks = new Dictionary<DateTime, Action>();
        private readonly Timer _mainTimer;

        public Scheduler()
        {
            _mainTimer = new Timer();
            _mainTimer.Interval = _interval;
            _mainTimer.Tick += MainTimer_Tick;
            _mainTimer.Start();
        }

        void MainTimer_Tick(object sender, EventArgs e)
        {
            CheckStatus();
        }

        public void Add(DateTime timeToFire, Action action)
        {
            lock (_scheduledTasks)
            {
                if (!_scheduledTasks.Keys.Contains(timeToFire))
                {
                    _scheduledTasks.Add(timeToFire, action);
                }
            }
        }

        public void CheckStatus()
        {
            Dictionary<DateTime, Action> scheduledTasksToRemove = new Dictionary<DateTime, Action>();
            lock (_scheduledTasks)
            {
                foreach (KeyValuePair<DateTime, Action> scheduledTask in _scheduledTasks)
                {
                    if (DateTime.Now >= scheduledTask.Key)
                    {
                        scheduledTask.Value.Invoke();
                        scheduledTasksToRemove.Add(scheduledTask.Key, scheduledTask.Value);
                    }
                }
            }
            foreach (KeyValuePair<DateTime, Action> pair in scheduledTasksToRemove)
            {
                _scheduledTasks.Remove(pair.Key);
            }
        }
    }
}

it is accessed in the following way in other classes

ApplicationContext.Current.Scheduler.Add(DateTime.Now.AddSeconds(1), ResetBackColor);

ApplicationContext is my singleton

also i am aware that a datetime object is not the best KEY for a dictionary, but it suits my purposes here

here's the singleton

public class ApplicationContext
{
    private static ApplicationContext _context;
    private Scheduler _scheduler;

    public Scheduler Scheduler
    {
        get { return _scheduler; }
    }

    private void SetProperties()
    {
        _scheduler = new Scheduler();
    }

    public static ApplicationContext Current
    {
        get
        {
            if (_context == null)
            {
                _context = new ApplicationContext();
                _context.SetProperties();

            }
            return _context;
        }
    }
}

Answer 1

Since you haven't posted the code for the singleton itself, it's hard to say if there are any problems in that. But no, as long as the object is reachable from a static variable (or anything on the stack), it should not get GC'ed. Which means your problem is... something else.

I'd suspect some synchronization issue assuming multiple threads access the object.

Answer 2

The class that you posted doesn't appear to be your singleton class. That code would be more helpful.

In any event, yes, if the timer were to fall out of scope and be GC'ed, then it would stop firing the event. What is more likely is that the scheduler is immediately falling out of scope and there is just a delay between when that happens and when GC occurs.

Posting your singleton code would allow me or anyone else to give a more specific answer.

Edit

Given the simplicity of the singleton class, the only potential issue that jumps out at me is a race condition on the Current property. Given that you don't lock anything, then two threads accessing Current property at the same time when it's null could potentially end up with different references, and the last one that gets set would be the only one that has a reference whose scope would extend beyond the scope of the property getter itself.

I would recommend creating a simple sync object instance as a static member, then locking it in the getter. This should prevent that condition from cropping up.

As an aside, what's the purpose for the SetProperties() method rather than initializing the variable in a parameterless constructor or even at the point of declaration? Having a function like this allows for the possibility of creating a new Scheduler object and abandoning the existing one.

Answer 3

Are you saying the issue is that your global/static pointer to Scheduler() becomes null? If thats the case we'd need to see code that manipulates that reference to understand while it fails.

I do have some feedback though... Your calls to _scheduledTasks.Remove() should happen within the lock, to prevent race conditions changing the list. Also your calls to scheduledTask.Value.Invoke() should happen outside the lock, to prevent someone from queueing another workitem within that lock when they implement Invoke. I would do this by moving due tasks to a list local to the stack before leaving the lock, then executing tasks from that list.

Then consider what happens if an Invoke() call throws an exception, if there are remaining tasks in the local list they may be leaked. The can be handled by catching/swallowing the exceptions, or only pulling 1 task out of the queue each time the lock is picked up.

Answer 4

System.Windows.Forms.Timer isn't very reliable in that it likes to get GC'd at odd times.

Try using System.Threading.Timer instead.

Answer 5

Is your application a regular WinForms program?

System.Windows.Forms.Timer listens for timer messages on your program's message loop. If you're program isn't WinForms, or if you do a lot of work on the UI thread (which is never a good idea), you should probably use System.Timers.Timer. Please note that it can fire events on multiple threads; see here