You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Pavel Moravec <pm...@redhat.com> on 2013/10/09 11:42:26 UTC
Review Request 14555: QPID-5214: [C++ broker] Memory leak in legacystore
when raising RHM_IORES_ENQCAPTHRESH
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14555/
-----------------------------------------------------------
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/JournalImpl.h 1528730
/trunk/qpid/cpp/src/qpid/legacystore/JournalImpl.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
Re: Review Request 14555: QPID-5214: [C++ broker] Memory leak in legacystore
when raising RHM_IORES_ENQCAPTHRESH
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14555/#review26815
-----------------------------------------------------------
int local()
{
try {
boost::intrusive_ptr<DataTokenImpl> dtokp(new DataTokenImpl);
dtokp->addRef();
I believe the above line is the issue, not boost itself. This is adding an *extra* reference over and above the one added automatically through the dtokp pointer instance. The count will never reach zero unless there is a corresponding *manual* release to offset this manual increment of the reference count.
enqueue_data_record(dtokp.get());
}
catch (int e) {
std::cout << "local: caught exception " << e << std::endl;
throw;
}
return 0;
}
- Gordon Sim
On Oct. 9, 2013, 9:42 a.m., Pavel Moravec wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14555/
> -----------------------------------------------------------
>
> (Updated Oct. 9, 2013, 9:42 a.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/JournalImpl.h 1528730
> /trunk/qpid/cpp/src/qpid/legacystore/JournalImpl.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
>
>
Re: Review Request 14555: QPID-5214: [C++ broker] Memory leak in legacystore
when raising RHM_IORES_ENQCAPTHRESH
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 14555: QPID-5214: [C++ broker] Memory leak in legacystore
when raising RHM_IORES_ENQCAPTHRESH
Posted by Pavel Moravec <pm...@redhat.com>.
-----------------------------------------------------------
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.
Changes
-------
So then this patch makes the fix trivial.
I was under assumption I tried it already and it did not work..
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 (updated)
-----
/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