Trouble with Factory and dynamic allocation in C++

Question

I have a factory that builds the objects with longest lifetime in my application. These have types, lets say, ClientA and ClientB, which depend on Provider (an abstract class with many possible implementations), so both clients have a reference to Provider as member.

According to the command-line arguments, the factory chooses one implementation of Provider, constructs it (with "new"), and passes it to the constructors of both clients.

The factory returns an object that represents my entire app. My main function is basically this:

int main(int argc, char** argv)
{
    AppFactory factory(argc, argv);
    App app = factory.buildApp();
    return app.run();
}

And the buildApp method is basically this:

App AppFactory::buildApp()
{
    Provider* provider = NULL;

    if (some condition)
    {
        provider = new ProviderX(); 
    }
    else
    {
        provider = new ProviderY();
    }

    ClientA clientA(*provider);
    ClientB clientB(*provider);

    App app(clientA, clientB);
    return app;
}

So, when execution ends, destructors of all objects are called, except for the provider object (because it was constructed with "new").

How can I improve this design to make sure that the destructor of the provider is called?

EDIT: To clarify, my intention is that both clients, the provider and the App object to share the same lifetime. After all answers, I now think both clients and the provider should be allocated on the heap its references passed to the App object, which will be responsible for deleting them when it dies. What do you say?

Answer 1

make provider an instance variable of AppFactory. then make provider a smart pointer or delete it in AppFactory's dtor.

Answer 2

Just call

delete provider;
provider = NULL;

in the destructor of ClientA and ClientB. This will also call the destructor of the provider.

Answer 3

there's not really enough here to help much, but without changing too much you can add reference counting to the Provider object and when the clients get destructed they drop the reference. When the reference goes to 0 in the Provider object, then call delete this.

Your lifetimes and scope are a little sketchy. Why do you create some objects on the stack and some on the heap - specifically your clients?

Answer 4

Unless App's constructor copies the Clients then they need to be new()ed as well - the current ones are allocated on the stack and will be deleted when app is returned.

I think you might want to be careful about which objects are being created - e.g. put debug statements in the Client's constructors.

What you probably want is to make Provider be reference counted, and have each Client just decrement the reference count, but that's a lot of work.

Alternatively make AppFactory own the Provider.

Answer 5

An option would be to have the factory return an AppComponents object containing all the components that the factory constructs. I.e. something like this:

int main(int argc, char** argv)
{
    AppFactory factory(argc, argv);
    AppComponents components = factory.buildApp();
    return components.getApp().run();
}

The AppComponents class would then be responsible for deleting your Provider and other objects.

Answer 6

Make provider a member variable of AppFactory, and delete it in destructor:

class AppFactory
{
    public:
    AppFactory(int argc, char** argv) : provider(NULL)
    {
       //...
    }
    ~AppFactory()
    {
        if (provider != NULL)
            delete provider;
    }
    App buildApp()
    {

        if (some condition)
        {
            provider = new ProviderX(); 
        }
        else
        {
            provider = new ProviderY();
        }

        ClientA clientA(*provider);
        ClientB clientB(*provider);

        App app(clientA, clientB);
        return app;

    } 
    private:
    Provider* provider;

};

int main(int argc, char** argv)
{
    AppFactory factory(argc, argv);
    App app = factory.buildApp();
    return app.run();
}

Answer 7

It's very simple with a shared ownership smart pointer:

App AppFactory::buildApp()
{
    boost::shared_ptr<Provider> provider;

    if (some condition)
    {
        provider.reset(new ProviderX()); 
    }
    else
    {
        provider.reset(new ProviderY());
    }

    ClientA clientA(provider);
    ClientB clientB(provider);

    App app(clientA, clientB);
    return app;
}

Assuming the app object owns the clients, and the clients all share the one provider. Make the clients take a shared_ptr<Provider> then, instead of a Provider& . As long as there is still a copy of a shared_ptr owning the provider object, the object won't be freed.

The best would be to not copy clientA and clientB, and not copy app by returning it by value, but move the clients into the app, and move the app itself into the returned object. That will be possible with the upcoming C++ version. But currently, either you make them pointers (using shared_ptr), or you keep copying them. Another option would be to use auto_ptr, which has a pseudo-transfer-of-ownership semantic. But that template has some inherent problems. So you should avoid using it.

Answer 8

You say "the provider and the App object to share the same lifetime" but beware that in in C++, the following code fragment ...

App app(clientA, clientB);
return app;

... is returning a copy of the App object: and therefore you may (depending on the compiler, see http://msdn.microsoft.com/en-us/library/ms364057(VS.80).aspx for example) actually have two App object instances (one inside the AppFactory::buildApp() method and another inside the main function).

To answer your question, I think I agree with your edit: pass the pointer-to-Provider into your App constructor, store it as member data of the App instance, and delete it when you destroy the App instance. As well as this, though, you might also change your code to ensure you don't copy the App instance: for example, allocate the App instance on the heap, change the AppFactory::buildApp() method to return a pointer to the App, and delete the App instance at the end of the main function.