My pluggable factory is broken! 8(

Started by
8 comments, last by Jan Wassenberg 16 years, 10 months ago
Here's an overview of my pluggable factory...

// EntityFactory.h
class EntityFactory
{
public:
    typedef std::map<OBJ_TYPE,EntityFactory*>	FactoryMap;

protected:

    static FactoryMap	m_registry;
    virtual BaseEntity*	Create()=0;

public:
    EntityFactory(OBJ_TYPE _type);
    static BaseEntity*  Construct(OBJ_TYPE _type);

};

// EntityFactory.cpp
EntityFactory::FactoryMap EntityFactory::m_registry;

EntityFactory(OBJ_TYPE _type){
    m_registry.insert(std::make_pair(_type,this));
}

BaseEntity*  Construct(OBJ_TYPE _type){
    EntityFactory::FactoryMap::iterator mapIter = m_registry.find(_type);
    if (mapIter!=EntityFactory::m_registry.end()){
        EntityFactory* pObjectFactory = (*mapIter).second;
        return pObjectFactory->Create();
    }
    else
        return NULL;
}


// Object.h
class Object : public BaseEntity{...};


class ObjectFactory : public EntityFactory
{
public:
    ObjectFactory():EntityFactory(IDC_OBJECT1){}
private:
    BaseEntity* Create() const{
        Object* pObj = NULL;
        try{
            pObj = new Object;
        }
        catch(...){};
        return pObj;
    }

    static const ObjectFactory register_this;
};



but it's not working. When I try to create an object using a call such as

Object* pObj = reinterpret_cast<Object*>(EntityFactory::Construct(IDC_OBJECT1));
I get back a NULL pointer... the reason being that EntityFactor::m_registry is empty and so the m_registry.find(_type) call is failing. So obviously the ObjectFactory is not creating its base type (EntityFactory) in its constructor call. I've taken this design from the article "Why Pluggable Factories Rock My World"... so what did I do wrong? Thanks for any help you can provide. Cheers, Timkin [Edited by - Timkin on June 23, 2007 3:17:36 AM]
Advertisement
You did remember to define the static member (as opposed to just declaring it) somewhere (presumably in Object.cpp), right?

const ObjectFactory ObjectFactory::register_this;

"Voilà! In view, a humble vaudevillian veteran, cast vicariously as both victim and villain by the vicissitudes of Fate. This visage, no mere veneer of vanity, is a vestige of the vox populi, now vacant, vanished. However, this valorous visitation of a bygone vexation stands vivified, and has vowed to vanquish these venal and virulent vermin vanguarding vice and vouchsafing the violently vicious and voracious violation of volition. The only verdict is vengeance; a vendetta held as a votive, not in vain, for the value and veracity of such shall one day vindicate the vigilant and the virtuous. Verily, this vichyssoise of verbiage veers most verbose, so let me simply add that it's my very good honor to meet you and you may call me V.".....V
Quote:Original post by joanusdmentia
You did remember to define the static member (as opposed to just declaring it) somewhere (presumably in Object.cpp), right?

const ObjectFactory ObjectFactory::register_this;


I had tried that, but it wasn't possible to compile this because ObjectFactory is an abstract class (I get a "ObjectFactory: Cannot instantiate abstract class" error).

Clearly the problem is one of no instantiation... otherwise the ObjectFactory constructor would be called and the registry entry would be made... so without the above instantiation doesn't occur... but it can't occur while the class is abstract... and I cannot see what is different between what I've done and what the article did.
ObjectFactory shouldn't be abstract. If the code you posted is the code you're actually using, the reason ObjectFactory is abstract is that Create() has a different signature to Create() in EntityFactory; it differs by const.
Thanks guys... it was the omission of 'const' (I must be blind) that was causing the compilation problem...now I just need to work out what's causing the fatal error on startup on the call
EntityFactory::m_registry.insert(std::make_pair(_type,this));

reporting an access violation. 8( Ah well, at least that's one problem out of the way... thanks...

Timkin

I use a very simple abstract factory which you can use and abuse if you so feel :)
//////////////////////////////////////////////////////////////////////////////////  abstract_factory.h///  Allows a user to register types and a key for the type(can be enums ints ///  strings). There are two register functions one which takes a create ///  function which must match the function signature:///  std::auto_ptr<Base> (*func)()///  this allows the user to be able to construct the object in the manner ///  which they require .///  If a create function is not supplied then the default will be used which ///  just returns an auto pointer to the base class./////////  @author Liam Devine, Dan Oppenheim and Richard  Smith (LDR) @date 08/03/2007///////////////////////////////////////////////////////////////////////////////#ifndef ABSTRCT_FACTORY_H_#	define ABSTRCT_FACTORY_H_#	include <map>#	include <string>#	include "types.h"#	include <algorithm>#	include <memory>namespace LDR{	template<typename Base, typename Class>		inline std::auto_ptr<Base> abstract_creater()	{		return std::auto_ptr<Base>(new Class);	}	template<typename Base,typename Id>	class Abstract_factory	{	public:		typedef std::auto_ptr<Base> (*func)();		Abstract_factory(){}		//search the map and look for the create function for this key, if found return the return value		//of the function otherwise return null pointer		std::auto_ptr<Base> create(Id key)		{			iter i;			if(( i = m_creater.find(key) ) != m_creater.end() ){ return  i->second(); }			else			{				throw std::runtime_error("could not find the key in the factory");			}		}		//register a key and a create function for this key		bool register_key(Id key, func function)		{			iter i;			if ( ( i = m_creater.find(key) ) == m_creater.end() )			{				m_creater[key] = function;				return true;			}			else return false;		}		//register a key and supplies the type of class which this key relates to 		//this is then used by the default creater to create the required class		//when called		template <typename Type> bool register_key(Id key)		{			iter i;			if ( ( i = m_creater.find(key) ) == m_creater.end() )			{				m_creater[key] = &abstract_creater<Base,Type>;				return true;			}			else return false;		}		//remove a key and create function from the map		bool unregister_key(Id key)		{			iter i;			if ( ( i = m_creater.find(key) ) == m_creater.end() )			{				m_creater.erase(i);				return true;			}			else return false;		}	private:		std::map< Id , func > m_creater;		typedef typename std::map< Id, func >::iterator iter;	};}#endif//ABSTRCT_FACTORY_H_
Ooh, this kind of code is unfortunately brittle. Sounds like an order-of-static-ctors issue: are you sure m_registry is initialized before insert is called? (use memory breakpoint)

BTW, (unrelated) hard-learned advice for any later readers pulling their hair out due to similar issues:
// IMPORTANT NOTE: if compiling this into a static lib and not using VC8's// "use library dependency inputs" linking mode, the object file will be// discarded because it does not contain any symbols that resolve another// module's undefined external(s). for a discussion of this topic, see:// http://forums.microsoft.com/MSDN/ShowPost.aspx?PostID=144087// workaround: in the main EXE project, reference a symbol from this module,// thus forcing it to be linked in. example:// #pragma comment(linker, "/include:_wstartup_InitAndRegisterShutdown")// (doing that in this module isn't sufficient, because it would only// affect the librarian and its generation of the static lib that holds// this file. instead, the process of linking the main EXE must be fixed.)
E8 17 00 42 CE DC D2 DC E4 EA C4 40 CA DA C2 D8 CC 40 CA D0 E8 40E0 CA CA 96 5B B0 16 50 D7 D4 02 B2 02 86 E2 CD 21 58 48 79 F2 C3
Quote:Original post by Timkin
Thanks guys... it was the omission of 'const' (I must be blind) that was causing the compilation problem...now I just need to work out what's causing the fatal error on startup on the call
EntityFactory::m_registry.insert(std::make_pair(_type,this));

reporting an access violation. 8( Ah well, at least that's one problem out of the way... thanks...

Timkin


EntityFactory::m_registry is probably getting constructed after ObjectFactory::register_this, and since the later's constructor calls insert() on the former.....boom! (construction order of globals and statics is defined within a translation unit, but not between them) If you make m_registry a local static of a function, the standard guarenttees that it's constructed the first time the function is called.

class EntityFactory{    typedef std::map<OBJ_TYPE,EntityFactory*> FactoryMap;    static FactoryMap& getRegistry();    virtual BaseEntity*	Create() = 0;public:    EntityFactory(OBJ_TYPE type);    static BaseEntity* Construct(OBJ_TYPE type);};EntityFactory::FactoryMap& EntityFactory::getRegistry(){    static FactoryMap registry;    return registry;}EntityFactory::EntityFactory(OBJ_TYPE type){    getRegistry().insert(std::make_pair(type,this));}BaseEntity* EntityFactory::Construct(OBJ_TYPE type){    FactoryMap::iterator it = getRegistry().find(type);    if (it != getRegistry().end())    {        EntityFactory* pObjectFactory = it->second;        return pObjectFactory->Create();    }    return 0;}


Also cleaned it up a little. I couldn't see any reason for FactoryMap being public or m_registry being protected, so I made them both private. Also, Create() doesn't need to be accessible from the derived to be overriden by the derived, so I made it private too.

[Edited by - joanusdmentia on June 23, 2007 9:45:08 AM]
"Voilà! In view, a humble vaudevillian veteran, cast vicariously as both victim and villain by the vicissitudes of Fate. This visage, no mere veneer of vanity, is a vestige of the vox populi, now vacant, vanished. However, this valorous visitation of a bygone vexation stands vivified, and has vowed to vanquish these venal and virulent vermin vanguarding vice and vouchsafing the violently vicious and voracious violation of volition. The only verdict is vengeance; a vendetta held as a votive, not in vain, for the value and veracity of such shall one day vindicate the vigilant and the virtuous. Verily, this vichyssoise of verbiage veers most verbose, so let me simply add that it's my very good honor to meet you and you may call me V.".....V
Quote:Original post by Jan Wassenberg
Ooh, this kind of code is unfortunately brittle. Sounds like an order-of-static-ctors issue: are you sure m_registry is initialized before insert is called? (use memory breakpoint)


That's exactly the conclusion I've arrived at... my trace shows that 'register_this' is indeed the first thing being constructed... edit: I didnt see joanus' response before I finished mine..

It's working now, so thanks for the help guys... muchly appreciated... 8)

I think I'm going to consider a less "brittle" approach though during my next code revision... using static variables seems like it's going to be hard for other people to figure what the hell I've done.

Cheers,

Timkin

[Edited by - Timkin on June 23, 2007 9:46:44 AM]
Quote:I think I'm going to consider a less "brittle" approach though during my next code revision... using static variables seems like it's going to be hard for other people to figure what the hell I've done.

:)
Having slogged through all kinds of bugs and errors due to init order, I am faced with the fact that no method of automatic init has failed to cause problems.
- function tables generated at link-time (think .CRT$XI init section) are very prone to linker optimization / the problem mentioned above.
- singletons are difficult to init/shutdown repeatedly (which needs to be done by self-tests) and subject to shutdown order bugs.
- static-ctor registration can blow up as in the OP's case.
- on-demand if(!isInitialized) Init() calls within the module's externally accessible functions are easy to forget under maintenance.

So what do we conclude? A simple interface with explicit Init/Shutdown seems to be the safest way. This makes init order perfectly clear and there's no mysterious stuff going on behind your back that needs to be debugged.
These Init/Shutdown functions can handle being called more than once with a thread-safe and referenced-counted version of the !isInitialized check (see src/lib/module_init in the archive referenced here.
E8 17 00 42 CE DC D2 DC E4 EA C4 40 CA DA C2 D8 CC 40 CA D0 E8 40E0 CA CA 96 5B B0 16 50 D7 D4 02 B2 02 86 E2 CD 21 58 48 79 F2 C3

This topic is closed to new replies.

Advertisement