Help me remove a Singleton: looking for an alternative

Question

Background: I have some classes implementing a subject/observer design pattern that I've made thread-safe. A subject will notify it's observers by a simple method call observer->Notified( this ) if the observer was constructed in the same thread as the notification is being made. But if the observer was constructed in a different thread, then the notification will be posted onto a queue to be processed later by the thread that constructed the observer and then the simple method call can be made when the notification event is processed.

So… I have a map associating threads and queues which gets updated when threads and queues are constructed and destroyed. This map itself uses a mutex to protect multi-threaded access to it.

The map is a singleton.

I've been guilty of using singletons in the past because "there will be only one in this application", and believe me - I have paid my penance!

One part of me can't help thinking that there really will be only one queue/thread map in an application. The other voice says that singletons are not good and you should avoid them.

I like the idea of removing the singleton and being able to stub it for my unit tests. Trouble is, I'm having a hard time trying to think of a good alternative solution.

The "usual" solution which has worked in the past is to pass in a pointer to the object to use instead of referencing the singleton. I think that would be tricky in this case, since observers and subjects are 10-a-penny in my application and it would very awkward to have to pass a queue/thread map object into the constructor of every single observer.

What I appreciate is that I may well have only one map in my application, but it shouldn't be in the bowels of the subject and observer class code where that decision is made.

Maybe this is a valid singleton, but I'd also appreciate any ideas on how I could remove it.

Thanks.

PS. I have read http://stackoverflow.com/questions/1300655/whats-alternative-to-singleton and this article mentioned in the accepted answer. I can't help thinking that the ApplicationFactory it just yet another singleton by another name. I really don't see the advantage.

Answer 1

Your observers may be cheap, but they depend on the notification-queue-thread map, right?

What's awkward about making that dependency explicit and taking control of it?

As for the application factory Miško Hevery describes in his article, the biggest advantages are that 1) the factory approach doesn't hide dependencies and 2) the single instances you depend on aren't globally available such that any other object can meddle with their state. So using this approach, in any given top-level application context, you know exactly what's using your map. With a globally accessible singleton, any class you use might be doing unsavory things with or to the map.

Answer 2

Some thoughts towards a solution:

Why do you need to enqueue notifications for observers that were created on a different thread? My preferred design would be to have the subject just notify the observers directly, and put the onus on the observers to implement themselves thread-safely, with the knowledge that Notified() might be called at any time from another thread. The observers know which parts of their state need to be protected with locks, and they can handle that better than the subject or the queue.

Assuming that you really have a good reason for keeping the queue, why not make it an instance? Just do queue = new Queue() somewhere in main, and then pass around that reference. There may only every be one, but you can still treat that as an instance and not a global static.

Answer 3

What's wrong with putting the queue inside the subject class? What do you need the map for?

You already have a thread reading from the singleton queue map. Instead of doing that simply make the map inside the subject class and provide two methods to subscribe an observer:

class Subject
{
  // Assume is threadsafe and all
  private QueueMap queue;
  void Subscribe(NotifyCallback, ThreadId)
  {
     // If it was created from another thread add to the map
     if (ThreadId != This.ThreadId)
       queue[ThreadId].Add(NotifyCallback);
  }

  public NotifyCallBack GetNext()
  {
     return queue[CallerThread.Id].Pop;
  }
}

Now any thread can call the GetNext method to start dispatching... of course it is all overly simplified, but it's just the idea.

Note: I'm working with the assumption that you already have an architecture around this model so that you already have a bunch of Observers, one or more subjects and that the threads already go to the map to do the notifications. This gets rid of the singleton but I'd suggest you notify from the same thread and let the observers handle concurrency issues.

Answer 4

My approach was to have the observers provide a queue when they registered with the subject; the observer's owner would be responsible for both the thread and the associated queue, and the subject would associate the observer with the queue, with no need for a central registry.

Thread-safe observers could register without a queue, and be called directly by the subject.

Answer 5

If the only purpose to trying to get rid of the singleton is from a unit test perspective, maybe replacing the singleton getter with something that you can swap in a stub for.

class QueueThreadMapBase
{
   //virtual functions
};

class QeueueThreadMap : public QueueThreadMapBase
{
   //your real implementation
};

class QeueueThreadMapTestStub : public QueueThreadMapBase
{
   //your test implementation
};

static QueueThreadMapBase* pGlobalInstance = new QeueueThreadMap;

QueueThreadMapBase* getInstance()
{
   return pGlobalInstance;
}

void setInstance(QueueThreadMapBase* pNew)
{
   pGlobalInstance = pNew
}

Then in your test just swap out the queue/thread map implementation. At the very least this exposes the singleton a little more.

Answer 6

What about adding a Reset method that returns a singleton to its initial state that you can call between tests? That might be simpler than a stub. It might be possible to add a generic Reset method to a Singleton template (deletes the internal singleton pimpl and resets the pointer). This could even include a registry of all singletons with a master ResetAll method to reset all of them!