----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14555/#review26818 -----------------------------------------------------------
/trunk/qpid/cpp/src/qpid/legacystore/MessageStoreImpl.cpp <https://reviews.apache.org/r/14555/#comment52171> The question would be (maybe Kim is best place to answer it), is there ever a case where an exception is thrown and the journal itself releases? The addRef() is presumably needed because the store will hold on to the object after having returned from enqueue_data_record()/enqueue_txn_data_record()? So another option would be to only make the addRef call after these two return successfully (and add a comment as to why). That said, I have no objections to the patch as proposed. - Gordon Sim On Oct. 9, 2013, 12:15 p.m., Pavel Moravec wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14555/ > ----------------------------------------------------------- > > (Updated Oct. 9, 2013, 12:15 p.m.) > > > Review request for qpid, Gordon Sim and Kim van der Riet. > > > Bugs: https://issues.apache.org/jira/browse/QPID-5214 > > https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/QPID-5214 > > > Repository: qpid > > > Description > ------- > > mrg::msgstore::MessageStoreImpl::enqueue method calls: > > .. > if (queue) { > boost::intrusive_ptr<DataTokenImpl> dtokp(new DataTokenImpl); > dtokp->addRef(); > .. > (where in further code calls executed, an exception is raised within the > scope of dtokp life) > > Apparently, boost library has a limitation with throwing exceptions where it > forgets to remove some reference to the object, causing delete is not > executed when intrusive_ptr variable is gone. See my reproducer below, > independend on qpid/store code. > > Therefore, to workaround it, let release the dtokp manually just before > throwing the exception. But I don't fully like adding new parameter to > handleIoResult method (and not sure with re-casting dtokp from data_tok* to > DataTokenImpl* is done in proper way). > > Reproducer of boost::intrusive_ptr limitation when throwing exception: let > run below program via valgrind, first as is (direct memory loss), and then > with "dtokp->release();" line uncommented (no direct memory loss). > > #include <iostream> > #include <boost/intrusive_ptr.hpp> > #include <boost/utility.hpp> > #include <boost/detail/atomic_count.hpp> > > class RefCounted : public boost::noncopyable { > mutable boost::detail::atomic_count count; > > public: > RefCounted() : count(0) {} > void addRef() const { ++count; } > void release() const { if (--count==0) released(); } > long refCount() { return count; } > > protected: > virtual ~RefCounted() {}; > // Allow subclasses to over-ride behavior when refcount reaches 0. > virtual void released() const { delete this; } > }; > > // intrusive_ptr support. > inline void intrusive_ptr_add_ref(const RefCounted* p) { p->addRef(); } > inline void intrusive_ptr_release(const RefCounted* p) { p->release(); } > > > class DataTokenImpl : public RefCounted > { > public: > DataTokenImpl() {} > ~DataTokenImpl() {} > }; > > void enqueue_data_record(const DataTokenImpl* dtokp) { > // dtokp->release(); > throw 20; > } > > int local() > { > try { > boost::intrusive_ptr<DataTokenImpl> dtokp(new DataTokenImpl); > dtokp->addRef(); > enqueue_data_record(dtokp.get()); > } > catch (int e) { > std::cout << "local: caught exception " << e << std::endl; > throw; > } > return 0; > } > > int main(void) > { > try { > local(); > } > catch (int e) { > std::cout << "main: caught exception " << e << std::endl; > } > return 0; > } > > > Diffs > ----- > > /trunk/qpid/cpp/src/qpid/legacystore/MessageStoreImpl.cpp 1528730 > > Diff: https://reviews.apache.org/r/14555/diff/ > > > Testing > ------- > > Valgrind used for reproducer returned no memory related problems, automated > tests passed. > > > Thanks, > > Pavel Moravec > >
