cpp: fix reference leak when reader create #7793

jiazhai · 2020-08-10T19:16:38Z

Motivation

User reports a valgrind error for client::createReader method:

==23308== 284,826 (160 direct, 284,666 indirect) bytes in 1 blocks are definitely lost in loss record 113 of 113
==23308==    at 0x4C2A593: operator new(unsigned long) (vg_replace_malloc.c:344)
==23308==    by 0x5303B4A: allocate (new_allocator.h:104)
==23308==    by 0x5303B4A: allocate (alloc_traits.h:351)
==23308==    by 0x5303B4A: __shared_count<pulsar::InternalState<pulsar::Result, pulsar::Reader>, std::allocator<pulsar::InternalState<pulsar::Result, pulsar::Reader> > > (shared_ptr_base.h:499)
==23308==    by 0x5303B4A: __shared_ptr<std::allocator<pulsar::InternalState<pulsar::Result, pulsar::Reader> > > (shared_ptr_base.h:957)
==23308==    by 0x5303B4A: shared_ptr<std::allocator<pulsar::InternalState<pulsar::Result, pulsar::Reader> > > (shared_ptr.h:316)
==23308==    by 0x5303B4A: allocate_shared<pulsar::InternalState<pulsar::Result, pulsar::Reader>, std::allocator<pulsar::InternalState<pulsar::Result, pulsar::Reader> > > (shared_ptr.h:598)
==23308==    by 0x5303B4A: make_shared<pulsar::InternalState<pulsar::Result, pulsar::Reader> > (shared_ptr.h:614)
==23308==    by 0x5303B4A: Promise (Future.h:91)
==23308==    by 0x5303B4A: pulsar::Client::createReader(std::string const&, pulsar::MessageId const&, pulsar::ReaderConfiguration const&, pulsar::Reader&) (Client.cc:142)
==23308==    by 0x401DDB: main (pulsarReader.cpp:92)
==23308==

It seems the ReaderImpl has been tracked twice when call WaitForCallbackValue. this PR is to fix the issue.

Modifications

fix WaitForCallbackValue which is changed in PR Replaced boost::bind with std::bind #3484.
add test for the reference issue.

Verifying this change

ut passed.
valgrind found no issue:

==14758== LEAK SUMMARY:
==14758==    definitely lost: 0 bytes in 0 blocks
==14758==    indirectly lost: 0 bytes in 0 blocks
==14758==      possibly lost: 0 bytes in 0 blocks
==14758==    still reachable: 12,621 bytes in 145 blocks
==14758==         suppressed: 0 bytes in 0 blocks
==14758== 
==14758== For lists of detected and suppressed errors, rerun with: -s
==14758== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

### Motivation User reports a valgrind error for `client::createReader` method: ``` ==23308== 284,826 (160 direct, 284,666 indirect) bytes in 1 blocks are definitely lost in loss record 113 of 113 ==23308== at 0x4C2A593: operator new(unsigned long) (vg_replace_malloc.c:344) ==23308== by 0x5303B4A: allocate (new_allocator.h:104) ==23308== by 0x5303B4A: allocate (alloc_traits.h:351) ==23308== by 0x5303B4A: __shared_count<pulsar::InternalState<pulsar::Result, pulsar::Reader>, std::allocator<pulsar::InternalState<pulsar::Result, pulsar::Reader> > > (shared_ptr_base.h:499) ==23308== by 0x5303B4A: __shared_ptr<std::allocator<pulsar::InternalState<pulsar::Result, pulsar::Reader> > > (shared_ptr_base.h:957) ==23308== by 0x5303B4A: shared_ptr<std::allocator<pulsar::InternalState<pulsar::Result, pulsar::Reader> > > (shared_ptr.h:316) ==23308== by 0x5303B4A: allocate_shared<pulsar::InternalState<pulsar::Result, pulsar::Reader>, std::allocator<pulsar::InternalState<pulsar::Result, pulsar::Reader> > > (shared_ptr.h:598) ==23308== by 0x5303B4A: make_shared<pulsar::InternalState<pulsar::Result, pulsar::Reader> > (shared_ptr.h:614) ==23308== by 0x5303B4A: Promise (Future.h:91) ==23308== by 0x5303B4A: pulsar::Client::createReader(std::string const&, pulsar::MessageId const&, pulsar::ReaderConfiguration const&, pulsar::Reader&) (Client.cc:142) ==23308== by 0x401DDB: main (pulsarReader.cpp:92) ==23308== ``` It seems the `ReaderImpl` has been tracked twice when call WaitForCallbackValue. this PR is to fix the issue. ### Modifications - fix WaitForCallbackValue which is changed in PR #3484. - add test for the reference issue. ### Verifying this change ut passed. valgrind found no issue: ``` ==14758== LEAK SUMMARY: ==14758== definitely lost: 0 bytes in 0 blocks ==14758== indirectly lost: 0 bytes in 0 blocks ==14758== possibly lost: 0 bytes in 0 blocks ==14758== still reachable: 12,621 bytes in 145 blocks ==14758== suppressed: 0 bytes in 0 blocks ==14758== ==14758== For lists of detected and suppressed errors, rerun with: -s ==14758== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0) ``` (cherry picked from commit 0e67fc5)

### Motivation User reports a valgrind error for `client::createReader` method: ``` ==23308== 284,826 (160 direct, 284,666 indirect) bytes in 1 blocks are definitely lost in loss record 113 of 113 ==23308== at 0x4C2A593: operator new(unsigned long) (vg_replace_malloc.c:344) ==23308== by 0x5303B4A: allocate (new_allocator.h:104) ==23308== by 0x5303B4A: allocate (alloc_traits.h:351) ==23308== by 0x5303B4A: __shared_count<pulsar::InternalState<pulsar::Result, pulsar::Reader>, std::allocator<pulsar::InternalState<pulsar::Result, pulsar::Reader> > > (shared_ptr_base.h:499) ==23308== by 0x5303B4A: __shared_ptr<std::allocator<pulsar::InternalState<pulsar::Result, pulsar::Reader> > > (shared_ptr_base.h:957) ==23308== by 0x5303B4A: shared_ptr<std::allocator<pulsar::InternalState<pulsar::Result, pulsar::Reader> > > (shared_ptr.h:316) ==23308== by 0x5303B4A: allocate_shared<pulsar::InternalState<pulsar::Result, pulsar::Reader>, std::allocator<pulsar::InternalState<pulsar::Result, pulsar::Reader> > > (shared_ptr.h:598) ==23308== by 0x5303B4A: make_shared<pulsar::InternalState<pulsar::Result, pulsar::Reader> > (shared_ptr.h:614) ==23308== by 0x5303B4A: Promise (Future.h:91) ==23308== by 0x5303B4A: pulsar::Client::createReader(std::string const&, pulsar::MessageId const&, pulsar::ReaderConfiguration const&, pulsar::Reader&) (Client.cc:142) ==23308== by 0x401DDB: main (pulsarReader.cpp:92) ==23308== ``` It seems the `ReaderImpl` has been tracked twice when call WaitForCallbackValue. this PR is to fix the issue. ### Modifications - fix WaitForCallbackValue which is changed in PR apache#3484. - add test for the reference issue. ### Verifying this change ut passed. valgrind found no issue: ``` ==14758== LEAK SUMMARY: ==14758== definitely lost: 0 bytes in 0 blocks ==14758== indirectly lost: 0 bytes in 0 blocks ==14758== possibly lost: 0 bytes in 0 blocks ==14758== still reachable: 12,621 bytes in 145 blocks ==14758== suppressed: 0 bytes in 0 blocks ==14758== ==14758== For lists of detected and suppressed errors, rerun with: -s ==14758== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0) ```

Fixes apache#14848 Fixes apache#14719 ### Motivation apache#7793 introduced a `testReferenceLeak` to avoid cyclic referenece of the reader. However, it adds a unused field `readerImplWeakPtr_` only for tests. The access to this field is not thread safe that the write operation happens in `handleConsumerCreated` while the read operation can happen anywhere via the getter. So there is a little chance that `readerPtr` in `testReferenceLeak` doesn't point to the right object. In addition, we should only guarantee the reference count becomes 0 after the producer, consumer or reader goes out of its scope. apache#14797 adds a `ClientTest.testReferenceCount` but it's also flaky. It's caused by the shared pointer of `ProducerImpl` is published to another thread via `shared_from_this()` but the test has a strong expectation that the reference count is exactly 1. ### Modifications - Remove `readerImplWeakPtr_` from `ReaderImpl` and get the weak pointer from `Reader` directly by adding a method to `PulsarFriend`. - Add the check of reader's reference count to `testReferenceCount` and remove the redundant `testReferenceLeak`. - Instead of asserting the reference count of producer/consumer/reader is 1, just assume the it's greater than 0.

Fixes #14848 Fixes #14719 ### Motivation #7793 introduced a `testReferenceLeak` to avoid cyclic referenece of the reader. However, it adds a unused field `readerImplWeakPtr_` only for tests. The access to this field is not thread safe that the write operation happens in `handleConsumerCreated` while the read operation can happen anywhere via the getter. So there is a little chance that `readerPtr` in `testReferenceLeak` doesn't point to the right object. In addition, we should only guarantee the reference count becomes 0 after the producer, consumer or reader goes out of its scope. #14797 adds a `ClientTest.testReferenceCount` but it's also flaky. It's caused by the shared pointer of `ProducerImpl` is published to another thread via `shared_from_this()` but the test has a strong expectation that the reference count is exactly 1. ### Modifications - Remove `readerImplWeakPtr_` from `ReaderImpl` and get the weak pointer from `Reader` directly by adding a method to `PulsarFriend`. - Add the check of reader's reference count to `testReferenceCount` and remove the redundant `testReferenceLeak`. - Instead of asserting the reference count of producer/consumer/reader is 1, just assume the it's greater than 0.

Fixes #14848 Fixes #14719 ### Motivation #7793 introduced a `testReferenceLeak` to avoid cyclic referenece of the reader. However, it adds a unused field `readerImplWeakPtr_` only for tests. The access to this field is not thread safe that the write operation happens in `handleConsumerCreated` while the read operation can happen anywhere via the getter. So there is a little chance that `readerPtr` in `testReferenceLeak` doesn't point to the right object. In addition, we should only guarantee the reference count becomes 0 after the producer, consumer or reader goes out of its scope. #14797 adds a `ClientTest.testReferenceCount` but it's also flaky. It's caused by the shared pointer of `ProducerImpl` is published to another thread via `shared_from_this()` but the test has a strong expectation that the reference count is exactly 1. ### Modifications - Remove `readerImplWeakPtr_` from `ReaderImpl` and get the weak pointer from `Reader` directly by adding a method to `PulsarFriend`. - Add the check of reader's reference count to `testReferenceCount` and remove the redundant `testReferenceLeak`. - Instead of asserting the reference count of producer/consumer/reader is 1, just assume the it's greater than 0. (cherry picked from commit f84ff57)

Fixes apache#14848 Fixes apache#14719 ### Motivation apache#7793 introduced a `testReferenceLeak` to avoid cyclic referenece of the reader. However, it adds a unused field `readerImplWeakPtr_` only for tests. The access to this field is not thread safe that the write operation happens in `handleConsumerCreated` while the read operation can happen anywhere via the getter. So there is a little chance that `readerPtr` in `testReferenceLeak` doesn't point to the right object. In addition, we should only guarantee the reference count becomes 0 after the producer, consumer or reader goes out of its scope. apache#14797 adds a `ClientTest.testReferenceCount` but it's also flaky. It's caused by the shared pointer of `ProducerImpl` is published to another thread via `shared_from_this()` but the test has a strong expectation that the reference count is exactly 1. ### Modifications - Remove `readerImplWeakPtr_` from `ReaderImpl` and get the weak pointer from `Reader` directly by adding a method to `PulsarFriend`. - Add the check of reader's reference count to `testReferenceCount` and remove the redundant `testReferenceLeak`. - Instead of asserting the reference count of producer/consumer/reader is 1, just assume the it's greater than 0.

fix reference leak of reader create

00986ad

jiazhai added area/client component/c++ labels Aug 10, 2020

jiazhai requested review from merlimat, sijie and codelipenghui August 10, 2020 19:16

jiazhai self-assigned this Aug 10, 2020

sijie approved these changes Aug 11, 2020

View reviewed changes

sijie added this to the 2.7.0 milestone Aug 11, 2020

sijie added the release/2.6.1 label Aug 11, 2020

fix deep copy in wait

197ca38

jiazhai changed the title ~~[WIP]cpp: fix reference leak when reader create~~ cpp: fix reference leak when reader create Aug 11, 2020

sijie merged commit 0e67fc5 into apache:master Aug 11, 2020

BewareMyPower mentioned this pull request Aug 12, 2020

Memory Leak in C++Pulsar Reader Interface #7797

Closed

BewareMyPower mentioned this pull request Mar 24, 2022

[C++] Fix flaky tests about reference count #14854

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cpp: fix reference leak when reader create #7793

cpp: fix reference leak when reader create #7793

jiazhai commented Aug 10, 2020 •

edited

Loading

cpp: fix reference leak when reader create #7793

cpp: fix reference leak when reader create #7793

Conversation

jiazhai commented Aug 10, 2020 • edited Loading

Motivation

Modifications

Verifying this change

jiazhai commented Aug 10, 2020 •

edited

Loading