Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[pulsar-client-cpp] Fix memory leak caused by not being executed ClientConnection destructor #5286

Merged
merged 1 commit into from Sep 29, 2019

Conversation

massakam
Copy link
Contributor

Motivation

Currently, closing a C++ Pulsar client object does not execute the destructor for the generated ClientConnection object. As a result, memory usage increases in applications that repeatedly create and close clients.

This pull-request is a continuation of #5246.

Modifications

The reason why the ClientConnection destructor is not executed is that the reference count of the shared pointer does not become 0 even when the ClientConnection object is closed. As a result of the investigation, it was found that if the shared pointer executor_ held by the ClientConnection object is reset, the reference count becomes 0 and the destructor is executed.

--- a/pulsar-client-cpp/lib/ClientConnection.cc
+++ b/pulsar-client-cpp/lib/ClientConnection.cc
@@ -1385,6 +1385,10 @@ void ClientConnection::close() {
     if (tlsSocket_) {
         tlsSocket_->lowest_layer().close();
     }
+
+    if (executor_) {
+        executor_.reset();
+    }
 }

 bool ClientConnection::isClosed() const { return state_ == Disconnected; }

However, with this fix alone, executing the following code causes an "Illegal instruction" error and the program crashes.
https://gist.github.com/massakam/ebd169caff3db802eee5001b0d06c980

The exact cause is unknown, but this issue doesn't seem to occur if closing ClientConnection before closing ExecutorServiceProvider. So, I changed the ConnectionPool destructor added in #5246 to the close() method and call it before closing ExecutorServiceProvider.

@massakam massakam added type/bug The PR fixed a bug or issue reported a bug component/c++ labels Sep 27, 2019
@massakam massakam added this to the 2.4.2 milestone Sep 27, 2019
@massakam massakam self-assigned this Sep 27, 2019
@massakam
Copy link
Contributor Author

rerun cpp tests

@massakam
Copy link
Contributor Author

rerun java8 tests

@merlimat merlimat merged commit 30d280d into apache:master Sep 29, 2019
@massakam massakam deleted the fix-cpp-cnx-leak branch September 30, 2019 01:52
wolfstudy pushed a commit that referenced this pull request Nov 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants