Singletons in agent++

[Philippe Roger]

Frank,

Thank you very much for your quick reply.  I have a few
additional comments.

The DefaultLog class could be replaced easily by a user of
the agent++/agentX++ code, and so could the AgentLogImpl
class.  True, the use of a static member prevents the
DefaultLog class derivation, no matter how you look at it.
But, using an object factory member is no worse than having a
static data member hard-coded as: log = new AgentLogImpl();  :-)

Given that you spent some time abstracting the log concept
into an AgentLog abstract class and then provided a real
implementation with AgentLogImpl, I would argue that if
someone needs a different log implementation, they SHOULD
provide a different implementation of AgentLogImpl. Isn't
it what it's for?  I'd say: problem solved.

On the second and more important point, I see where you are
coming from: "helping" the user code, without creating too
many constraints.  However, I would rather have an extra test
for a NULL pointer rather than an unsightly crash.  It's not
like this code is _really_ optimized for speed, with the
numerous conversions to and from strings, and all the temporary
objects.  What I am saying here is that agent++/agentX++ may
not be suitable for very small/slow embedded systems anyhow,
starting with the C++ requirements, of course, including
templates. The users of agent++/agentX++ are very likely
running on a "decent" size machine (in RAM and CPU power).

[I hope I am not offending anyone there, but I have been
working with small systems too, and there is nothing wrong
with them: they are just "different"... :-)]

-- Philippe

-----Original Message-----
From: Frank Fock [mailto:Frank.Fock____t-online.de]
Sent: Tuesday, June 04, 2002 3:42 PM
To: Philippe Roger
Subject: Re: Singletons in agent++


Hi Philippe,

Regarding the clumsy usage of the singleton pattern in log.*
you are right. I will rework it, however there are also other
problems to solve. For example, one would like to replace
the default logging by another implementation without changing
the LOG_* macros. So an object factory is needed with which
one could create a derived DefaultLog too.

Providing a MibEntry::instance() factory is not appropriate,
because there might be use cases (an object available in more
than one context) where in an object is added to more than
one MibContext. Enforcing the singleton pattern would then
cause many additional objects.

AGENT++, internally, does not rely on the singleton MibEntry
classes. However the public instance member is provided as a
shortcut for implementations that are supposed to be "singleton".

Checking the instance member could be helpful, but checking
any pointer could also decrease performance/readability...

Any further comments/suggestions are welcome!

Best regards,
Frank


Philippe Roger wrote:

> 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

--
Frank Fock - AGENT++
Email: fock____agentpp.com
Fax: +49 7195 177108