Singletons in agent++

[Philippe Roger]

Hi Frank,

I have been studying and experimenting with agent++ and agentX++
for a little while now, and I have come across something which I
think could be implemented better.  There are many places where a
static member pointer to an object is used, called instance.  I
could be mistaken, but it looks to me like every Mib leaf node
generated by AgentGen has such a static data member.

The primary issue with the assignment "instance = this" in the
constructor of the Mib node is that if another such object were
ever to be created (a potential error, or say static objects
representing a notification erroneously defined in multiple files),
the _other_ object would indeed be created, and the single instance
pointer overwritten.  Similarly, since the instance pointer exists
without having any class object created, there can be cases where
the instance pointer may be NULL and still be referenced (not a
good thing!)...  In essence, the "instance" data member does not
guarantee the existence of an object, nor that this object is
indeed unique, which is what Singletons are supposed to do (see
Gamma et al, Design Patterns).

I am not sure of what the best solution there might be, in the
context of agent++/AgentX++, maybe throwing an exception if the
instance pointer is already set to a non-NULL value, indicating
that another object of that class already exists? Most people
would probably object as they wouldn't want to have to handle
exceptions in their project... However, since the code produced
by AgentGen for these classes appears to always include a default
constructor, it may be possible to do the following instead:

Make the T constructor private so it can't be called externally
to the class (i.e. by anything other than the static member functions),
and add such a new static member function:

class T {
public:
      virtual ~T();
      static T* instance() { if (zinstance == 0)
                                 zinstance = new T;
                             return zinstance }
...

private:
      T();
      static T* zinstance;
}

Then it's simply a matter of replacing any reference to the pointer
T::instance by the pointer T::instance().  It will always point to a
single object of class T (as long as new  does not throw, of course!),
whether just created or previously created, and unique.


One other place where an instance static pointer is used is in the
implementation of the DefaultLog class.  The issue there is again
that it does not make sure that a log instance actually exists, and
doesn't make sure that there is a single one.  The Default::log static
member is initialized to "new AgentLogImpl()" in log.cpp, but you can't
really be sure that this will have executed before DefaultLog::log
is used, e.g. in the agent++ examples.  In particular, this can be
true if dynamic libraries are involved (I tried it in Windows, with
C++Builder 5, turning all the agent++ classes into DLLOPT classes).
In general, as Stroustrup says (section 9.4.1, page 217, C++ Programming
Language, 3rd edition), "there is no guaranteed order of initialization
of global variables in different translation units" (this includes
static class members).  Again, the solution for this problem relies
on the Singleton pattern (Gamma et al). and can be implemented as
something like the following:

in agent++/include/log.h

/**
 * The DefaultLog class has a static Log member, that is used by the
 * AGENT++ API for logging.
 */

class DLLOPT DefaultLog {
public:
   static AgentLog *log();
   static LogEntry *log_entry();
   static void create_log_entry(unsigned char t);
   static void clear_log_entry();

protected:
   DefaultLog() { }
   virtual ~DefaultLog() { }

private:
   static AgentLog* instance;
   static LogEntry* entry;
};

with the DefaultLog class implementation in agent++/src/log.cpp, as:

AgentLog *DefaultLog::log()
{
    if (instance == 0)
        instance = new AgentLogImpl();
    return instance;
}

void DefaultLog::create_log_entry(unsigned char t)
{
    if (entry == 0) {
        entry = log()->create_log_entry(t);
        entry->init();
    }
}

LogEntry *DefaultLog::log_entry()
{
    if (entry == 0)
        create_log_entry(ERROR_LOG | 1);   // take your pick here...
    return entry;
}

void DefaultLog::clear_log_entry() {
    delete entry;
    entry = 0;
}

AgentLog* DefaultLog::instance = 0;
LogEntry* DefaultLog::entry = 0;

With the above, the logging macros can be rewritten as:

#ifdef _NO_LOGGING

#define LOG_BEGIN(x)
#define LOG(x)
#define LOG_END

#else

#define LOG_BEGIN(x)                      \
{                                         \
    if (DefaultLog::log()->log_needed(x)) \
    {                                     \
        DefaultLog::log()->lock();        \
        DefaultLog::create_log_entry(x);  \

#define LOG(x)  *DefaultLog::log_entry() += x

#define LOG_END                           \
        *DefaultLog::log() += DefaultLog::log_entry(); \
        DefaultLog::clear_log_entry();    \
        DefaultLog::log()->unlock();      \
    }                                     \
}

#endif _NO_LOGGING

Of course, the agent++/examples need to be changed from:
	DefaultLog::log->setfilter(ERROR_LOG, 5);
to:
	DefaultLog::log()->setfilter(ERROR_LOG, 5);

A few places in agent++/src/log.cpp also need a similar change,
i.e. DefaultLog::log()->now()...


I apologize for the length of this message, but as I provided a
possible solution to the problems, I hope this helps, and I
certainly welcome any comments. 

-- Philippe