shared_ptr, intrusive_ptr ...
I'm confused again :( I've read the note about shared_ptr & this in the documentation, but I'm still a bit lost as to how I'm supposed to overcome the problem. The code below would work fine if I were to remove all this smart pointer business and handle new & delete by hand. But because this kind of code is just so typical of the huge Java library I'm porting, there's not much choice but to get an as "surprise free" object model as possible. As far as I understand what is going on, the reason why the runtime is reporting a "double free" is because two independently constructed shared_ptrs' do not share the same reference count, which intrusive_ptr would allow me to do, except that intrusive_ptr doesn't vary "polymorphically" with its body as shared_ptr does. Obviously, I could "roll my own" solution by modifying shared_ptr to use a common reference count the way intrusive_ptr does, but I was wondering: is that my only course of action? Or has anyone found a workaround/clever use of the boost::xxx_ptr to overcome this situation? Many thanks -- JFB #include <iostream> #include <boost/shared_ptr.hpp> struct TaskDoneStruct { virtual void success() = 0 ; virtual void failure() = 0 ; } ; typedef boost::shared_ptr<TaskDoneStruct> TaskDone ; struct LongTaskStruct { TaskDone callback ; virtual void setCallback(const TaskDone& td) { this->callback = td ; } virtual void run() { start() ; complete() ; } virtual void start() = 0 ; virtual void complete() { if (callback) { callback->success() ; } } virtual ~LongTaskStruct() {} } ; struct MyTaskStruct : public LongTaskStruct, TaskDoneStruct { MyTaskStruct() { setCallback(TaskDone(this)) ; // this is the root of the // double free problem } virtual void start() { std::cout << "Starting lengthy process ..." << std::endl ; } virtual void success() { std::cout << "Done lengthy process ..." << std::endl ; } virtual void failure() { std::cout << "Aborted lengthy process ..." << std::endl ; } } ; typedef boost::shared_ptr<MyTaskStruct> MyTask ; void tastTest() { MyTask myTask(new MyTaskStruct) ; myTask->run() ; } int main( int argc , char * argv) { std::cout << "Tests starting ..." << std::endl ; tastTest() ; std::cout << "Tests done ..." << std::endl ; return 0 ; } /* BoostTests(1270) malloc: *** Deallocation of a pointer not malloced: 0x300180; This could be a double free(), or free() called with the middle of an allocated block; */
Jean-François Brouillet schrieb:
I'm confused again :(
I've read the note about shared_ptr & this in the documentation, but I'm still a bit lost as to how I'm supposed to overcome the problem.
The code below would work fine if I were to remove all this smart pointer business and handle new & delete by hand. But because this kind of code is just so typical of the huge Java library I'm porting, there's not much choice but to get an as "surprise free" object model as possible.
in case you've not left out anything important: I think overriding a virtual function for callback is more appropriate here than storing a "cross-casted" version of the this-pointer. however, if the solution may be intrusive you can derive your classes from boost::shared_from_this, and use "shared_from_this()", which returns a shared_ptr, instead of "this".
#include <iostream>
#include <boost/shared_ptr.hpp>
struct TaskDoneStruct { virtual void success() = 0 ; virtual void failure() = 0 ; } ;
typedef boost::shared_ptr<TaskDoneStruct> TaskDone ;
struct LongTaskStruct {
TaskDone callback ;
virtual void setCallback(const TaskDone& td) { this->callback = td ; }
virtual void run() { start() ; complete() ; }
virtual void start() = 0 ; virtual void complete() { if (callback) { callback->success() ; } }
virtual ~LongTaskStruct() {} } ;
struct MyTaskStruct : public LongTaskStruct, TaskDoneStruct {
MyTaskStruct() { setCallback(TaskDone(this)) ; // this is the root of the // double free problem }
virtual void start() { std::cout << "Starting lengthy process ..." << std::endl ; }
virtual void success() { std::cout << "Done lengthy process ..." << std::endl ; }
virtual void failure() { std::cout << "Aborted lengthy process ..." << std::endl ; } } ;
typedef boost::shared_ptr<MyTaskStruct> MyTask ;
void tastTest() { MyTask myTask(new MyTaskStruct) ;
myTask->run() ; }
int main( int argc , char * argv) {
std::cout << "Tests starting ..." << std::endl ;
tastTest() ;
std::cout << "Tests done ..." << std::endl ;
return 0 ; }
/*
BoostTests(1270) malloc: *** Deallocation of a pointer not malloced: 0x300180; This could be a double free(), or free() called with the middle of an allocated block;
*/ _______________________________________________ Boost-users mailing list Boost-users@lists.boost.org http://lists.boost.org/mailman/listinfo.cgi/boost-users
-- Stefan Strasser
Jean-François Brouillet wrote:
As far as I understand what is going on, the reason why the runtime is reporting a "double free" is because two independently constructed shared_ptrs' do not share the same reference count, which intrusive_ptr would allow me to do, except that intrusive_ptr doesn't vary "polymorphically" with its body as shared_ptr does.
I'm not sure what you mean by that. intrusive_ptr<> aside, you need to derive from enable_shared_from_this<> if you want to create a shared_ptr<> to this as explained in: http://boost.org/libs/smart_ptr/enable_shared_from_this.html but you can't do this in a constructor, because the outer shared_ptr<> hasn't been created yet. Use a static create() function as explained here: http://boost.org/libs/smart_ptr/sp_techniques.html#in_constructor The other option is intrusive_ptr<>.
Thanks Stefan & Peter, for your replies. I tried immediately: struct MyTaskStruct : public LongTaskStruct , TaskDoneStruct , boost::enable_shared_from_this<MyTaskStruct> { MyTaskStruct() { setCallback(shared_from_this()) ; } ... } And obviously, I got: terminate called after throwing an instance of 'boost::bad_weak_ptr' what(): boost::bad_weak_ptr precisely because:
but you can't do this in a constructor, because the outer shared_ptr<> hasn't been created yet.
But the next solution
Use a static create() function as explained here :
is not good enough. I've got 150+ Java classes to translate, each with an average of about 2 constructors. That would mean an extra 300 createXYZ, and apart from being ugly, and an insult to the user, is a maintenance nightmare. Ideally, I would want an intrusive_ptr (because no matter how they are created, they can be made to share a single ref-count per body/ instance), but one that offers the shared_ptr guarantee: shared_ptr<T> can be implicitly converted to shared_ptr<U> whenever T* can be implicitly converted to U*. In particular, shared_ptr<T> is implicitly convertible to shared_ptr<T const>, to shared_ptr<U> where U is an accessible base of T, and to shared_ptr<void>. What I am after, is a construct as transparent as possible, for which I can say, in my documentation, "For every struct XXX -- the body --, there is a corresponding envelope -- xxxx_ptr<XXX> -- that walks and talks like a XXX *" So far, shared_ptr had everything covered except that "sharing" thing, which is an artefact, not a requirement. intrusive_ptr makes this "sharing" thingie a non issue (as it should be), but fails on the "polymorphic covariance" quoted above... In this project, it is acceptable to rely on a common base class, (anyway I've got to translate Java's "Object" class :-) so I can stuff it with whatever machinery is required to make garbage collection work. Any idea? Many thanks. -- JFB
Jean-François Brouillet wrote:
Use a static create() function as explained here :
is not good enough. I've got 150+ Java classes to translate, each with an average of about 2 constructors. That would mean an extra 300 createXYZ, and apart from being ugly, and an insult to the user, is a maintenance nightmare.
Insult to the user? Ugly? There is little difference between SomethingPtr p( new Something ); and SomethingPtr p = Something::create(); from user point of view, and it hides the explicit new.
Ideally, I would want an intrusive_ptr (because no matter how they are created, they can be made to share a single ref-count per body/ instance), but one that offers the shared_ptr guarantee:
shared_ptr<T> can be implicitly converted to shared_ptr<U> whenever T* can be implicitly converted to U*. In particular, shared_ptr<T> is implicitly convertible to shared_ptr<T const>, to shared_ptr<U> where U is an accessible base of T, and to shared_ptr<void>.
intrusive_ptr<T> is convertible to intrusive_ptr<U> when T* is convertible to U*. It doesn't support void, of course, but neither does Java. You'll probably need a common base class in which to put the reference count, though... either that (which implies virtual public inheritance to avoid multiple copies of the reference count), or an interface with add_ref and release that is implemented in every root concrete class (since Java only supports single implementation inheritance, multiple counts won't be a problem.) I prefer the static create function, but to each his own. ;-)
On 2005-05-15 00:49:54 +0100, "Peter Dimov" <pdimov@mmltd.net> said:
intrusive_ptr<T> is convertible to intrusive_ptr<U> when T* is convertibl e to U*.
It does, indeed! Many thanks. Not sure why I thought that the guarantee given by shared_ptr somehow negatively implied that such a guarantee wouldn't hold for intrusive_ptr. I was wrong. in http://www.boost.org/libs/smart_ptr/smarttests.htm there's a mention of a "cyclic" smart pointer implementation that dealt with cycles "using a dequeue". Is this available somewhere (I couldn't locate it in boost 1.32) ? Anyway, here's my revised snippet that satisfies all my criteria: - polymorphic covariance (for lack of a better name :) - ad lib creation of the envelope from the base type without any concern about "lack of sharing" - and, the icing on the cake: I haven't any qualm any more about passing intrusive_ptr<T> around by value since they're only the size of a pointer and thus should entirely fit in in a register. Thanks again. -- JFB #include <iostream> #include <boost/intrusive_ptr.hpp> struct ObjectStruct { int refCount ; ObjectStruct() : refCount(0) { std::cout << std::hex << (int) this << " - I'm born" << std::endl ; } void refer() { ++refCount ; } void unrefer() { if (--refCount == 0) delete this ; } virtual ~ObjectStruct() { std::cout << std::hex << (int) this << " - I'm gone" << std::endl ; } } ; void intrusive_ptr_add_ref(ObjectStruct * p) { p->refer() ; } void intrusive_ptr_release(ObjectStruct * p) { p->unrefer() ; } struct TaskDoneStruct : public virtual ObjectStruct { virtual void success() = 0 ; virtual void failure() = 0 ; } ; typedef boost::intrusive_ptr<TaskDoneStruct> TaskDone ; struct MyTaskDone : public TaskDoneStruct { virtual void success() { std::cout << "MyTaskDone::Done lengthy process ..." << std::endl ; } virtual void failure() { std::cout << "MyTaskDone::Aborted lengthy process ..." << std::endl ; } } ; struct LongTaskStruct : public virtual ObjectStruct { TaskDone callback ; virtual void setCallback(const TaskDone& td) { this->callback = td ; } virtual void run() { start() ; complete() ; } virtual void start() = 0 ; virtual void complete() { if (callback) { callback->success() ; } } virtual ~LongTaskStruct() {} } ; struct MyTaskStruct : public LongTaskStruct , TaskDoneStruct { MyTaskStruct() { setCallback(TaskDone(this)) ; } virtual void start() { std::cout << "Starting lengthy process ..." << std::endl ; } virtual void success() { std::cout << "Done lengthy process ..." << std::endl ; } virtual void failure() { std::cout << "Aborted lengthy process ..." << std::endl ; } } ; struct MyFailingTaskStruct : public MyTaskStruct { virtual void start() { std::cout << "Aborting lengthy process ..." << std::endl ; if (callback) { callback->failure() ; } } } ; typedef boost::intrusive_ptr<MyTaskStruct> MyTask ; typedef boost::intrusive_ptr<MyFailingTaskStruct> MyFailingTask ; void callTask(MyTask task) { task->run() ; } void taskTest2() { MyTask myTask(new MyTaskStruct) ; callTask(myTask) ; MyFailingTask myFailingTask(new MyFailingTaskStruct) ; callTask(myFailingTask) ; } void taskTest() { MyTask myTask(new MyTaskStruct) ; myTask->run() ; myTask->setCallback(myTask) ; myTask->run() ; myTask->setCallback(TaskDone(new MyTaskDone)) ; myTask->run() ; } int main( int argc , char * argv[]) { std::cout << "Tests starting ..." << std::endl ; taskTest() ; taskTest2() ; std::cout << "Tests done ..." << std::endl ; return 0 ; } /* Tests starting ... 30018c - I'm born Starting lengthy process ... Done lengthy process ... Starting lengthy process ... Done lengthy process ... 3001a4 - I'm born Starting lengthy process ... MyTaskDone::Done lengthy process ... 3001a4 - I'm gone 30018c - I'm gone 30018c - I'm born Starting lengthy process ... Done lengthy process ... 3001bc - I'm born Aborting lengthy process ... Aborted lengthy process ... Done lengthy process ... Tests done ... */ -- Do your users a favor: give them Style: http://www.uiwithstyle.org
Jean-François Brouillet wrote:
in http://www.boost.org/libs/smart_ptr/smarttests.htm
there's a mention of a "cyclic" smart pointer implementation that dealt with cycles "using a dequeue". Is this available somewhere (I couldn't locate it in boost 1.32) ?
It is probably referring to Greg Colvin's cyclic_ptr. Just do a Google search on cyclic_ptr and you'll be able to find several posts about it. The thread starting at: http://lists.boost.org/MailArchives/boost/msg47966.php is a good summary (but read the followups as well as this post is not entirely correct.) I have another cycle collector that may be of interest - see the attached file for a prototype. It is expensive, though; you should only use reset_and_collect in places where cycles are known to occur. Best if you can avoid cycles entirely (which might be difficult in a Java port). ;-)
Gottlob Frege wrote:
void intrusive_ptr_add_ref(ObjectStruct * p) { p->refer () ; } void intrusive_ptr_release(ObjectStruct * p) { p->unrefer () ; }
Are you going to make it thread-safe (or is that something you just left out of the example?)?
Well, I left it out of the example since my main focus was on getting it to work _at all_. So, yes, I've seen the multi-threading tricks that shared_ptr & friends do use, and I want to take advantage of that. ... But I'm not there yet, unfortunately. Something that escaped my attention when I first ran (and then posted) the snippet is that I've still got a problem to solve :-(
struct MyTaskStruct : public LongTaskStruct , TaskDoneStruct {
MyTaskStruct() { setCallback(TaskDone(this)) ; }
The problem here, is that setCallback is called from the constructor, at a time when
MyTask myTask(new MyTaskStruct) ;
hasn't finished executing yet. So the "myTask" intrusive pointer doesn'y exist yet. In fact the issue is even more subtle but here it goes: The "TaskDone(this)" is perfectly fine, except that setCallback stores it into a member of MyTaskStruct. In turn, this means that when "myTask" is finally created on the stack, the refCount is alreadly 1. Which it increments to 2. When "myTask" goes out of scope, it rightly decrements the refCount, but because the object itself now embeds a pointer to itself, the refCount never reaches 0. The reason I didn't catch this at first is because, the test case also specified aternate callback in which case the embedded pointer inside myTask is *not* referring to itself, so when nyTask gooes out of scope, the refCount drops to 0 and then the embedded pointer unrefer is called, which in turn destroys the referred to callback. Obviously, this is unacceptable. I'm working on this. The following is a perfectly legal Java idiom which I find all over the place in the source I am porting. Basically it says: For simple use, just use me as is. I implement a default policy regarding so and so. However you are free to specify your own policy when required: interface I { void i() ; } class C implements I { I myI ; void i() { System.out.println("I.i() called, as implemented by C") ; } C() { myI = this ; } void setI(I i) { myI = i ; } void f() { myI.i() ; } } The equivalent C++ code would be something along the lines: struct IStruct { virtual void i() = 0 ; } ; typedef intrusive_ptr<IStruct> I ; struct CStruct : public IStruct { I myI ; CStruct() { myI = I(this) ; } } ; typedef instrusive_ptr<CStruct> C ; But this code would fail because of the above problem, where the embedded myI would refer to the same object as the outer smart pointer. Still digging ... :-( -- JFB
On 2005-05-15 12:59:08 +0100, "Peter Dimov" <pdimov@mmltd.net> said:
It is probably referring to Greg Colvin's cyclic_ptr. Just do a Google search on cyclic_ptr and you'll be able to find several posts about it. The thread starting at:
Many Thanks.
is a good summary (but read the followups as well as this post is not entirely correct.
I have another cycle collector that may be of interest - see the attached file for a prototype.
Attached file? My news-reader probably filtered it out :-( Any URL? Many Thanks for your time. -- JFB
Jean-François Brouillet wrote:
On 2005-05-15 12:59:08 +0100, "Peter Dimov" <pdimov@mmltd.net> said:
I have another cycle collector that may be of interest - see the attached file for a prototype.
Attached file?
My news-reader probably filtered it out :-( Any URL?
Many Thanks for your time.
http://lists.boost.org/boost-users/2005/05/11805.php http://lists.boost.org/boost-users/att-11805/reset_and_collect.cpp
Jean-François Brouillet wrote:
The following is a perfectly legal Java idiom which I find all over the place in the source I am porting.
Basically it says: For simple use, just use me as is. I implement a default policy regarding so and so. However you are free to specify your own policy when required:
interface I { void i() ; }
class C implements I {
I myI ;
void i() { System.out.println("I.i() called, as implemented by C") ; }
C() { myI = this ; }
void setI(I i) { myI = i ; }
void f() { myI.i() ; } }
You can do this with shared_ptr, but not with intrusive_ptr. ;-) class C: private I { shared_ptr<I> myI; void i(); // ... C( C const & ); C& operator=( C const & ); public: C(): myI( this, null_deleter() ) { } void setI( shared_ptr<I> const & i ) { myI = i; } }; A null_deleter works here because you know that the internal I and the shared_ptr member have the same lifetime, that of C. http://boost.org/libs/smart_ptr/sp_techniques.html#in_constructor "Depending on context, if the inner shared_ptr this_ doesn't need to keep the object alive, use a null_deleter as explained here and here." http://boost.org/libs/smart_ptr/sp_techniques.html#static
On 2005-05-15 20:46:56 +0100, "Peter Dimov" <pdimov@mmltd.net> said:
You can do this with shared_ptr, but not with intrusive_ptr. ;-)
That's extremely funny ... I moved away from shared_ptr to intrusive_ptr because I needed the envelope to use the same reference count no matter how they were created. Now, I would need to go back to shared_ptr, precisely because the reference counts are separate ... Let's recap: I need an envelope for ``this'' when embedding it into itself. - some kind of weak, non ref-counted pointer would do. I need an envelope for ``this'' when passing it as argument to some library that expects an envelope, not a body. - all such envelopes need to share the same ref-count otherwise I will leak. In one of my intrusive_ptr attempt, I tried ... setCallback(TaskDone(this, false)) ; ... In the hope that the "false" argument (matching; ``addRef'' in the template) would note trigger a call to intrusive_ptr_add_ref. But for some reason that still escapes me, this somehow played harsh with the construction leading to a "pure virtual method call" on the defining "start", whereas passing "true" (that is, using the default value) led to proper behavior in this regard. Unless I can find an appropriate "cyclic" implementation, I'm kind of OKish to accept both a strong and a weak envelope, as would have been the case with intrusive_ptr had I succeeded in getting the ``false'' argument to behave as advertised :-[
class C: private I { shared_ptr<I> myI;
void i(); // ...
C( C const & ); C& operator=( C const & );
public:
C(): myI( this, null_deleter() ) { }
void setI( shared_ptr<I> const & i ) { myI = i; } };
A null_deleter works here because you know that the internal I and the shared_ptr member have the same lifetime, that of C.
Well, a "null_deleter" for a smart_ptr should be semantically equivalent to an "addRef" set to false in an intrusive_ptr, right? I guess I get what the problem was now: intrusive_ptr(T * p, bool add_ref = true): p_(p) { if(p_ != 0 && add_ref) intrusive_ptr_add_ref(p_); } Here the refCount is not incremented if addRef is false. However: ~intrusive_ptr() { if(p_ != 0) intrusive_ptr_release(p_); } has no way to know whether addRef was called or not at construction time. This explains probably why this was wreaking-havoc my tests ... Thanks for keeping this thread alive :-) -- Do your users a favor: give them Style: http://www.uiwithstyle.org
participants (3)
-
Jean-François Brouillet -
Peter Dimov -
Stefan Strasser