A Question On Smart Pointers and Their Inevitable Indeterminism

StackOverflow https://stackoverflow.com/questions/400993

Question

I've been extensively using smart pointers (boost::shared_ptr to be exact) in my projects for the last two years. I understand and appreciate their benefits and I generally like them a lot. But the more I use them, the more I miss the deterministic behavior of C++ with regarding to memory management and RAII that I seem to like in a programming language. Smart pointers simplify the process of memory management and provide automatic garbage collection among other things, but the problem is that using automatic garbage collection in general and smart pointer specifically introduces some degree of indeterminisim in the order of (de)initializations. This indeterminism takes the control away from the programmers and, as I've come to realize lately, makes the job of designing and developing APIs, the usage of which is not completely known in advance at the time of development, annoyingly time-consuming because all usage patterns and corner cases must be well thought of.

To elaborate more, I'm currently developing an API. Parts of this API requires certain objects to be initialized before or destroyed after other objects. Put another way, the order of (de)initialization is important at times. To give you a simple example, let's say we have a class called System. A System provides some basic functionality (logging in our example) and holds a number of Subsystems via smart pointers.

class System {
public:
    boost::shared_ptr< Subsystem > GetSubsystem( unsigned int index ) {
        assert( index < mSubsystems.size() );
        return mSubsystems[ index ];
    }

    void LogMessage( const std::string& message ) {
        std::cout << message << std::endl;
    }

private:
    typedef std::vector< boost::shared_ptr< Subsystem > > SubsystemList;
    SubsystemList mSubsystems;    
};

class Subsystem {
public:
    Subsystem( System* pParentSystem )
         : mpParentSystem( pParentSystem ) {
    }

    ~Subsystem() {
         pParentSubsystem->LogMessage( "Destroying..." );
         // Destroy this subsystem: deallocate memory, release resource, etc.             
    }

    /*
     Other stuff here
    */

private:
    System * pParentSystem; // raw pointer to avoid cycles - can also use weak_ptrs
};

As you can already tell, a Subsystem is only meaningful in the context of a System. But a Subsystem in such a design can easily outlive its parent System.

int main() {
    {
        boost::shared_ptr< Subsystem > pSomeSubsystem;
        {
            boost::shared_ptr< System > pSystem( new System );
            pSomeSubsystem = pSystem->GetSubsystem( /* some index */ );

        } // Our System would go out of scope and be destroyed here, but the Subsystem that pSomeSubsystem points to will not be destroyed.

     } // pSomeSubsystem would go out of scope here but wait a second, how are we going to log messages in Subsystem's destructor?! Its parent System is destroyed after all. BOOM!

    return 0;
}

If we had used raw pointers to hold subsystems, we would have destroyed subsystems when our system had gone down, of course then, pSomeSubsystem would be a dangling pointer.

Although, it's not the job of an API designer to protect the client programmers from themselves, it's a good idea to make the API easy to use correctly and hard to use incorrectly. So I'm asking you guys. What do you think? How should I alleviate this problem? How would you design such a system?

Thanks in advance, Josh

Was it helpful?

Solution

Problem Summary

There are two competing concerns in this question.

  1. Life-cycle management of Subsystems, allowing their removal at the right time.
  2. Clients of Subsystems need to know that the Subsystem they are using is valid.

Handling #1

System owns the Subsystems and should manage their life-cycle with it's own scope. Using shared_ptrs for this is particularly useful as it simplifies destruction, but you should not be handing them out because then you loose the determinism you are seeking with regard to their deallocation.

Handling #2

This is the more intersting concern to address. Describing the problem in more detail, you need clients to receive an object which behaves like a Subsystem while that Subsystem (and it's parent System) exists, but behaves appropriately after a Subsystem is destroyed.

This is easily solved by a combination of the Proxy Pattern, the State Pattern and the Null Object Pattern. While this may seem to be a bit complex of a solution, 'There is a simplicity only to be had on the other side of complexity.' As Library/API developers, we must go the extra mile to make our systems robust. Further, we want our systems to behave intuitively as a user expects, and to decay gracefully when they attempt to misuse them. There are many solutions to this problem, however, this one should get you to that all important point where, as you and Scott Meyers say, it is "easy to use correctly and hard to use incorrectly.'

Now, I am assuming that in reality, System deals in some base class of Subsystems, from which you derive various different Subsystems. I've introduced it below as SubsystemBase. You need to introduce a Proxy object, SubsystemProxy below, which implements the interface of SubsystemBase by forwarding requests to the object it is proxying. (In this sense, it is very much like a special purpose application of the Decorator Pattern.) Each Subsystem creates one of these objects, which it holds via a shared_ptr, and returns when requested via GetProxy(), which is called by the parent System object when GetSubsystem() is called.

When a System goes out of scope, each of it's Subsystem objects gets destructed. In their destructor, they call mProxy->Nullify(), which causes their Proxy objects to change their State. They do this by changing to point to a Null Object, which implements the SubsystemBase interface, but does so by doing nothing.

Using the State Pattern here has allowed the client application to be completely oblivious to whether or not a particular Subsystem exists. Moreover, it does not need to check pointers or keep around instances that should have been destroyed.

The Proxy Pattern allows the client to be dependent on a light weight object that completely wraps up the details of the API's inner workings, and maintains a constant, uniform interface.

The Null Object Pattern allows the Proxy to function after the original Subsystem has been removed.

Sample Code

I had placed a rough pseudo-code quality example here, but I wasn't satisfied with it. I've rewritten it to be a precise, compiling (I used g++) example of what I have described above. To get it to work, I had to introduce a few other classes, but their uses should be clear from their names. I employed the Singleton Pattern for the NullSubsystem class, as it makes sense that you wouldn't need more than one. ProxyableSubsystemBase completely abstracts the Proxying behavior away from the Subsystem, allowing it to be ignorant of this behavior. Here is the UML Diagram of the classes:

UML Diagram of Subsystem and System Hierarchy

Example Code:

#include <iostream>
#include <string>
#include <vector>

#include <boost/shared_ptr.hpp>


// Forward Declarations to allow friending
class System;
class ProxyableSubsystemBase;

// Base defining the interface for Subsystems
class SubsystemBase
{
  public:
    // pure virtual functions
    virtual void DoSomething(void) = 0;
    virtual int GetSize(void) = 0;

    virtual ~SubsystemBase() {} // virtual destructor for base class
};


// Null Object Pattern: an object which implements the interface to do nothing.
class NullSubsystem : public SubsystemBase
{
  public:
    // implements pure virtual functions from SubsystemBase to do nothing.
    void DoSomething(void) { }
    int GetSize(void) { return -1; }

    // Singleton Pattern: We only ever need one NullSubsystem, so we'll enforce that
    static NullSubsystem *instance()
    {
      static NullSubsystem singletonInstance;
      return &singletonInstance;
    }

  private:
    NullSubsystem() {}  // private constructor to inforce Singleton Pattern
};


// Proxy Pattern: An object that takes the place of another to provide better
//   control over the uses of that object
class SubsystemProxy : public SubsystemBase
{
  friend class ProxyableSubsystemBase;

  public:
    SubsystemProxy(SubsystemBase *ProxiedSubsystem)
      : mProxied(ProxiedSubsystem)
      {
      }

    // implements pure virtual functions from SubsystemBase to forward to mProxied
    void DoSomething(void) { mProxied->DoSomething(); }
    int  GetSize(void) { return mProxied->GetSize(); }

  protected:
    // State Pattern: the initial state of the SubsystemProxy is to point to a
    //  valid SubsytemBase, which is passed into the constructor.  Calling Nullify()
    //  causes a change in the internal state to point to a NullSubsystem, which allows
    //  the proxy to still perform correctly, despite the Subsystem going out of scope.
    void Nullify()
    {
        mProxied=NullSubsystem::instance();
    }

  private:
      SubsystemBase *mProxied;
};


// A Base for real Subsystems to add the Proxying behavior
class ProxyableSubsystemBase : public SubsystemBase
{
  friend class System;  // Allow system to call our GetProxy() method.

  public:
    ProxyableSubsystemBase()
      : mProxy(new SubsystemProxy(this)) // create our proxy object
    {
    }
    ~ProxyableSubsystemBase()
    {
      mProxy->Nullify(); // inform our proxy object we are going away
    }

  protected:
    boost::shared_ptr<SubsystemProxy> GetProxy() { return mProxy; }

  private:
    boost::shared_ptr<SubsystemProxy> mProxy;
};


// the managing system
class System
{
  public:
    typedef boost::shared_ptr< SubsystemProxy > SubsystemHandle;
    typedef boost::shared_ptr< ProxyableSubsystemBase > SubsystemPtr;

    SubsystemHandle GetSubsystem( unsigned int index )
    {
        assert( index < mSubsystems.size() );
        return mSubsystems[ index ]->GetProxy();
    }

    void LogMessage( const std::string& message )
    {
        std::cout << "  <System>: " << message << std::endl;
    }

    int AddSubsystem( ProxyableSubsystemBase *pSubsystem )
    {
      LogMessage("Adding Subsystem:");
      mSubsystems.push_back(SubsystemPtr(pSubsystem));
      return mSubsystems.size()-1;
    }

    System()
    {
      LogMessage("System is constructing.");
    }

    ~System()
    {
      LogMessage("System is going out of scope.");
    }

  private:
    // have to hold base pointers
    typedef std::vector< boost::shared_ptr<ProxyableSubsystemBase> > SubsystemList;
    SubsystemList mSubsystems;
};

// the actual Subsystem
class Subsystem : public ProxyableSubsystemBase
{
  public:
    Subsystem( System* pParentSystem, const std::string ID )
      : mParentSystem( pParentSystem )
      , mID(ID)
    {
         mParentSystem->LogMessage( "Creating... "+mID );
    }

    ~Subsystem()
    {
         mParentSystem->LogMessage( "Destroying... "+mID );
    }

    // implements pure virtual functions from SubsystemBase
    void DoSomething(void) { mParentSystem->LogMessage( mID + " is DoingSomething (tm)."); }
    int GetSize(void) { return sizeof(Subsystem); }

  private:
    System * mParentSystem; // raw pointer to avoid cycles - can also use weak_ptrs
    std::string mID;
};



//////////////////////////////////////////////////////////////////
// Actual Use Example
int main(int argc, char* argv[])
{

  std::cout << "main(): Creating Handles H1 and H2 for Subsystems. " << std::endl;
  System::SubsystemHandle H1;
  System::SubsystemHandle H2;

  std::cout << "-------------------------------------------" << std::endl;
  {
    std::cout << "  main(): Begin scope for System." << std::endl;
    System mySystem;
    int FrankIndex = mySystem.AddSubsystem(new Subsystem(&mySystem, "Frank"));
    int ErnestIndex = mySystem.AddSubsystem(new Subsystem(&mySystem, "Ernest"));

    std::cout << "  main(): Assigning Subsystems to H1 and H2." << std::endl;
    H1=mySystem.GetSubsystem(FrankIndex);
    H2=mySystem.GetSubsystem(ErnestIndex);


    std::cout << "  main(): Doing something on H1 and H2." << std::endl;
    H1->DoSomething();
    H2->DoSomething();
    std::cout << "  main(): Leaving scope for System." << std::endl;
  }
  std::cout << "-------------------------------------------" << std::endl;
  std::cout << "main(): Doing something on H1 and H2. (outside System Scope.) " << std::endl;
  H1->DoSomething();
  H2->DoSomething();
  std::cout << "main(): No errors from using handles to out of scope Subsystems because of Proxy to Null Object." << std::endl;

  return 0;
}

Output from the code:

main(): Creating Handles H1 and H2 for Subsystems.
-------------------------------------------
  main(): Begin scope for System.
  <System>: System is constructing.
  <System>: Creating... Frank
  <System>: Adding Subsystem:
  <System>: Creating... Ernest
  <System>: Adding Subsystem:
  main(): Assigning Subsystems to H1 and H2.
  main(): Doing something on H1 and H2.
  <System>: Frank is DoingSomething (tm).
  <System>: Ernest is DoingSomething (tm).
  main(): Leaving scope for System.
  <System>: System is going out of scope.
  <System>: Destroying... Frank
  <System>: Destroying... Ernest
-------------------------------------------
main(): Doing something on H1 and H2. (outside System Scope.)
main(): No errors from using handles to out of scope Subsystems because of Proxy to Null Object.

Other Thoughts:

  • An interesting article I read in one of the Game Programming Gems books talks about using Null Objects for debugging and development. They were specifically talking about using Null Graphics Models and Textures, such as a checkerboard texture to make missing models really stand out. The same could be applied here by changing out the NullSubsystem for a ReportingSubsystem which would log the call and possibly the callstack whenever it is accessed. This would allow you or your library's clients to track down where they are depending on something that has gone out of scope, but without the need to cause a crash.

  • I mentioned in a comment @Arkadiy that the circular dependency he brought up between System and Subsystem is a bit unpleasant. It can easily be remedied by having System derive from an interface on which Subsystem depends, an application of Robert C Martin's Dependency Inversion Principle. Better still would be to isolate the functionality that Subsystems need from their parent, write an interface for that, then hold onto an implementor of that interface in System and pass it to the Subsystems, which would hold it via a shared_ptr. For example, you might have LoggerInterface, which your Subsystem uses to write to the log, then you could derive CoutLogger or FileLogger from it, and keep an instance of such in System.
    Eliminating the Circular Dependency

OTHER TIPS

This is do-able with proper use of the weak_ptr class. In fact, you are already quite close to having a good solution. You are right that you cannot be expected to "out-think" your client programmers, nor should you expect that they will always follow the "rules" of your API (as I'm sure you are already aware). So, the best you can really do is damage control.

I recommend having your call to GetSubsystem return a weak_ptr rather than a shared_ptr simply so that the client developer can test the validity of the pointer without always claiming a reference to it.

Similarly, have pParentSystem be a boost::weak_ptr<System> so that it can internally detect whether its parent System still exists via a call to lock on pParentSystem along with a check for NULL (a raw pointer won't tell you this).

Assuming you change your Subsystem class to always check whether or not its corresponding System object exists, you can ensure that if the client programmer attempts to use the Subsystem object outside of the intended scope that an error will result (that you control), rather than an inexplicable exception (that you must trust the client programmer to catch/properly handle).

So, in your example with main(), things won't go BOOM! The most graceful way to handle this in the Subsystem's dtor would be to have it look something like this:

class Subsystem
{
...
  ~Subsystem() {
       boost::shared_ptr<System> my_system(pParentSystem.lock());

       if (NULL != my_system.get()) {  // only works if pParentSystem refers to a valid System object
         // now you are guaranteed this will work, since a reference is held to the System object
         my_system->LogMessage( "Destroying..." );
       }
       // Destroy this subsystem: deallocate memory, release resource, etc.             

       // when my_system goes out of scope, this may cause the associated System object to be destroyed as well (if it holds the last reference)
  }
...
};

I hope this helps!

Here System clearly owns the subsystems and I see no point in having shared ownership. I would simply return a raw pointer. If a Subsystem outlives its System, that's an error on its own.

You were right at the very beginning in your first paragraph. Your designs based on RAII (like mine and most well written C++ code) require that your objects are held by exclusive ownership pointers. In Boost that would be scoped_ptr.

So why didn't you use scoped_ptr. It will certainly be because you wanted the benefits of weak_ptr to protect against dangling references but you can only point a weak_ptr at a shared_ptr. So you have adopted the common practice of expediently declaring shared_ptr when what you really wanted was single ownership. This is a false declaration and as you say, it compromises destructors being called in the correct sequence. Of course if you never ever share the ownership you will get away with it - but you will have to constantly check all of your code to make sure it was never shared.

To make matters worse the boost::weak_ptr is inconvenient to use (it has no -> operator) so programmers avoid this inconvenience by falsely declaring passive observing references as shared_ptr. This of course shares ownership and if you forget to null that shared_ptr then your object will not get destroyed or its destructor called when you intend it to.

In short, you have been shafted by the boost library - it fails to embrace good C++ programming practices and forces programmers to make false declarations in order to try and derive some benefit from it. It is only useful for scripting glue code that genuinely wants shared ownership and is not interested in tight control over memory or destructors being called in the correct sequence.

I have been down the same path as you. Protection against dangling pointers is badly needed in C++ but the boost library does not provide an acceptable solution. I had to solve this problem - my software department wanted assurances that C++ can be made safe. So I rolled my own - it was quite a lot of work and can be found at:

http://www.codeproject.com/KB/cpp/XONOR.aspx

It is totally adequate for single threaded work and I am about to update it to embrace pointers being shared across threads. Its key feature is that it supports smart (self-zeroing) passive observers of exclusively owned objects.

Unfortunately programmers have become seduced by garbage collection and 'one size fits all' smart pointer solutions and to a large extent are not even thinking about ownership and passive observers - as a result they do not even know that what they are doing is wrong and don't complain. Heresy against Boost is almost unheard of!

The solutions that have been suggested to you are absurdly complicated and of no help at all. They are examples of the absurdity that results from a cultural reluctance to recognize that object pointers have distinct roles that must be correctly declared and a blind faith that Boost must be the solution.

I don't see a problem with having System::GetSubsystem return a raw pointer (RATHER than a smart pointer) to a Subsystem. Since the client is not responsible for constructing the objects, then there is no implicit contract for the client to be responsible for the cleanup. And since it is an internal reference, it should be reasonable to assume that the lifetime of the Subsystem object is dependent on the lifetime of the System object. You should then reinforce this implied contract with documentation stating as much.

The point is, that you are not reassigning or sharing ownership - so why use a smart pointer?

The real problem here is your design. There is no nice solution, because the model doesn't reflect good design principles. Here's a handy rule of thumb I use:

  • If an object holds a collection of other objects, and can return any arbitrary object from that collection, then remove that object from your design.

I realise that your example is contrived, but its an anti-pattern I see a lot at work. Ask yourself, what value is System adding that std::vector< shared_ptr<SubSystem> > doesnt? Users of your API need to know the interface of SubSystem (since you return them), so writing a holder for them is only adding complexity. At least people know the interface to std::vector, forcing them to remember GetSubsystem() above at() or operator[] is just mean.

Your question is about managing object lifetimes, but once you start handing out objects, you either lose control of the lifetime by allowing others to keep them alive (shared_ptr) or risk crashes if they are used after they have gone away (raw pointers). In multi-threaded apps its even worse - who locks the objects you are handing out to different threads? Boosts shared and weak pointers are a complexity inducing trap when used in this fashion, especially since they are just thread safe enough to trip up inexperienced developers.

If you are going to create a holder, it needs to hide complexity from your users and relieve them of burdens you can manage yourself. As an example, an interface consisting of a) Send command to subsystem (e.g. a URI - /system/subsystem/command?param=value) and b) iterate subsystems and subsystem commands (via an stl-like iterator) and possibly c) register subsystem would allow you to hide almost all of the details of your implementation from your users, and enforce the lifetime/ordering/locking requirements internally.

An iteratable/enumerable API is vastly preferable to exposing objects in any case - the commands/registrations could be easily serialised for generating test cases or configuration files, and they could be displayed interactively (say, in a tree control, with dialogs composed by querying the available actions/parameters). You would also be protecting your API users from internal changes you may need to make to the subsystem classes.

I would caution you against following the advice in Aarons answer. Designing a solution to an issue this simple that requires 5 different design patterns to implement can only mean that the wrong problem is being solved. I'm also weary of anyone who quotes Mr Myers in relation to design, since by his own admission:

"I have not written production software in over 20 years, and I have never written production software in C++. Nope, not ever. Furthermore, I’ve never even tried to write production software in C++, so not only am I not a real C++ developer, I’m not even a wannabe. Counterbalancing this slightly is the fact that I did write research software in C++ during my graduate school years (1985-1993), but even that was small (a few thousand lines) single-developer to-be-thrown-away-quickly stuff. And since striking out as a consultant over a dozen years ago, my C++ programming has been limited to toy “let’s see how this works” (or, sometimes, “let’s see how many compilers this breaks”) programs, typically programs that fit in a single file".

Not to say that his books aren't worth reading, but he has no authority to speak on design or complexity.

In your example, it would be better if the System held a vector<Subsystem> rather than a vector<shared_ptr<Subsystem> >. Its both simpler, and eliminates the concern you have. GetSubsystem would return a reference instead.

Stack objects will be released in the opposite order to which they where instantiated, so unless the developer using the API is trying to manage the smart pointer, it's normally not going to be a problem. There are just some things you are not going to be able to prevent, the best you can do is provide warnings at run time, preferably debug only.

Your example seems very much like COM to me, you have reference counting on the subsystems being returned by using shared_ptr, but you are missing it on the system object itself.

If each of the subsystem objects did an addref on the system object on creation, and a release on destruction you could at least display an exception if the reference count was incorrect when the system object is destroyed early.

Use of weak_ptr would also allow you to provide a message instead/aswell as blowing up when things are freed in the wrong order.

The essence of your problem is a circular reference: System refers to Subsystem, and Subsystem, in turn, refers to System. This kind of data structure cannot be easily handled by reference counting - it requires proper garbage collection. You're trying to break the loop by using a raw pointer for one of the edges - this will only produce more complications.

At least two good solutions have been suggested, so I will not attempt to outdo the previous posters. I can only note that in @Aaron's solution you can have a proxy for the System rather than for Subsystems - dependingo n what is more complex and what makes sense.

Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top