views:

1991

answers:

12

What are some ways you can shoot yourself in the foot when using boost::shared_ptr? In other words, what pitfalls do I have to avoid when I use boost::shared_ptr?

+11  A: 

Here are two things to avoid:

  • Calling the get() function to get the raw pointer and use it after the pointed-to object goes out of scope.

  • Passing a reference of or a raw pointer to a shared_ptr should be dangerous too, since it won't increment the internal count which helps keep the object alive.

However, realize that passing a raw pointer (via get()) is common for calling legacy routines - but you have to know that what you're calling doesn't take ownership (or keep the ptr/ref beyond the object's lifetime). So, it's not ideal but it is something you will lkely have to do.
Michael Burr
Hopefully not for too long anymore, since shared_ptr is in tr1, as far as I know. (Although I couldn't find it in my g++ 4.3.3 implementation, except for an "internal header file" that is not supopsed to be included in user code.)
+7  A: 

We debug several weeks strange behavior.

The reason was:
we passed 'this' to some thread workers instead of 'shared_from_this'.

Mykola Golubyev
+13  A: 

Creating multiple unrelated shared_ptr's to the same object:

#include <stdio.h>
#include "boost/shared_ptr.hpp"

class foo
{
public:
    foo() { printf( "foo()\n"); }

    ~foo() { printf( "~foo()\n"); }
};

typedef boost::shared_ptr<foo> pFoo_t;

void doSomething( pFoo_t p)
{
    printf( "doing something...\n");
}

void doSomethingElse( pFoo_t p)
{
    printf( "doing something else...\n");
}

int main() {
    foo* pFoo = new foo;

    doSomething( pFoo_t( pFoo));
    doSomethingElse( pFoo_t( pFoo));

    return 0;
}
Michael Burr
Of course, the solution here is to never use `new` outside a `shared_ptr` constructor.
rlbond
See boost::make_shared() to avoid new completely. :)
Marcus Lindblom
@Marcus: +1 for answering a question I was just about to ask somewhere else...
Chris Kaminski
+17  A: 

Cyclic references: a shared_ptr<> to something that has a shared_ptr<> to the original object. You can use weak_ptr<> to break this cycle, of course.

Kaz Dragon
good one - in non trivial cases it can be difficult to see the whole dependency chain. Of course that could be a design weakness, but it's a pitfall nonetheless
Phil Nash
@Phil, yeah, that's my pet peeve of design weaknesses: not considering ownership of data when creating objects on the heap. It becomes worse in a multi-threaded project.
I'll be honest: I've not fallen into this particular trap myself. But this may be a case of just knowing the pitfall and therefore automatically avoiding it.
Kaz Dragon
+1  A: 

Giving out a shared_ptr< T > to this inside a class definition is also dangerous. Use enabled_shared_from_this instead.

See the following post here

George
+8  A: 

Be careful making two pointers to the same object.

boost::shared_ptr<Base> b( new Derived() );
{
  boost::shared_ptr<Derived> d( b.get() );
} // d goes out of scope here, deletes pointer

b->doSomething(); // crashes

instead use this

boost::shared_ptr<Base> b( new Derived() );
{
  boost::shared_ptr<Derived> d = 
    boost::dynamic_pointer_cast<Derived,Base>( b );
} // d goes out of scope here, refcount--

b->doSomething(); // no crash

Also, any classes holding shared_ptrs should define copy constructors and assignment operators.

Don't try to use shared_from_this() in the constructor--it won't work. Instead create a static method to create the class and have it return a shared_ptr.

I've passed references to shared_ptrs without trouble. Just make sure it's copied before it's saved (i.e., no references as class members).

> "Also, any classes holding shared_ptrs should define copy constructors and assignment operators." ---- Why? Isn't the point of using smart pointers to reduce the need for copy ctor and assignment operator?
Andreas
A: 

If you have a registry of the shared objects (a list of all active instances, for example), the objects will never be freed. Solution: as in the case of circular dependency structures (see Kaz Dragon's answer), use weak_ptr as appropriate.

Mr Fooz
+7  A: 

Constructing an anonymous temporary shared pointer, for instance inside the arguments to a function call:

f(shared_ptr<Foo>(new Foo()), g());

This is because it is permissible for the new Foo() to be executed, then g() called, and g() to throw an exception, without the shared_ptr ever being set up, so the shared_ptr does not have a chance to clean up the Foo object.

Brian Campbell
+1  A: 

You need to be careful when you use shared_ptr in multithread code. It's then relatively easy to become into a case when couple of shared_ptrs, pointing to the same memory, is used by different threads.

oo_olo_oo
A: 

The popular widespread use of shared_ptr will almost inevitably cause unwanted and unseen memory occupation.

Cyclic references are a well known cause and some of them can be indirect and difficult to spot especially in complex code that is worked on by more than one programmer; a programmer may decide than one object needs a reference to another as a quick fix and doesn't have time to examine all the code to see if he is closing a cycle. This hazard is hugely underestimated.

Less well understood is the problem of unreleased references. If an object is shared out to many shared_ptrs then it will not be destroyed until every one of them is zeroed or goes out of scope. It is very easy to overlook one of these references and end up with objects lurking unseen in memory that you thought you had finished with.

Although strictly speaking these are not memory leaks (it will all be released before the program exits) they are just as harmful and harder to detect.

These problems are the consequences of expedient false declarations: 1. Declaring what you really want to be single ownership as shared_ptr. scoped_ptr would be correct but then any other reference to that object will have to be a raw pointer, which could be left dangling. 2. Declaring what you really want to be a passive observing reference as shared_ptr. weak_ptr would be correct but then you have the hassle of converting it to share_ptr every time you want to use it.

I suspect that your project is a fine example of the kind of trouble that this practice can get you into.

If you have a memory intensive application you really need single ownership so that your design can explicitly control object lifetimes.

With single ownership opObject=NULL; will definitely delete the object and it will do it now.

With shared ownership spObject=NULL; ........who knows?......

John Morrison
How will opObject=NULL delete an object? Assuming opObject is a pointer? It only sets pointer to null but your object still lives in memory, which is ok if opObject did not own the object, otherwise memory leak. If ptr owns object you need at least delete ptr; ptr = 0 (in C++ we prefer 0 to NULL ...)
stefanB
I'm going to vote you back up, -2 is harsh and I agree with your main point about shared_ptr having a danger of reference loops that keep objects around permanently.
Zan Lynx
+3  A: 

Not precisely a footgun, but certainly a source of frustration until you wrap your head around how to do it the C++0x way: most of the predicates you know and love from <functional> don't play nicely with shared_ptr. Happily, std::tr1::mem_fn works with objects, pointers and shared_ptrs, replacing std::mem_fun, but if you want to use std::negate, std::not1, std::plus or any of those old friends with shared_ptr, be prepared to get cozy with std::tr1::bind and probably argument placeholders as well. In practice this is actually a lot more generic, since now you basically end up using bind for every function object adaptor, but it does take some getting used to if you're already familiar with the STL's convenience functions.

This DDJ article touches on the subject, with lots of example code. I also blogged about it a few years ago when I first had to figure out how to do it.

Meredith L. Patterson
+1  A: 

Using shared_ptr for really small objects (like char short) could be an overhead if you have a lot of small objects on heap but they are not really "shared". boost::shared_ptr allocates 16 bytes for every new reference count it creates on g++ 4.4.3 and VS2008 with Boost 1.42. std::tr1::shared_ptr allocates 20 bytes. Now if you have a million distinct shared_ptr<char> that means 20 million bytes of your memory are gone in holding just count=1. Not to mention the indirection costs and memory fragmentation. Try with the following on your favorite platform.

void * operator new (size_t size) {
  std::cout << "size = " << size << std::endl;
  void *ptr = malloc(size);
  if(!ptr) throw std::bad_alloc();
  return ptr;
}
void operator delete (void *p) {
  free(p);
}
Sumant
Oh yeah. Pet peeve of mine, people who don't notice memory usage or care about it.
Zan Lynx