From bf6d63b0b975e2afb59c0151b931404b3c019e7b Mon Sep 17 00:00:00 2001 From: ihsandemir Date: Mon, 18 Jun 2018 12:52:54 +0300 Subject: [PATCH 1/2] Fix for issues #428 and #429. Do not access any object after runnable is finished. An e.g. scenario: ShutdownTask is started, client is shutdown and client destructor is being executed which causes the ClientConnectionManagerImpl to be destructed and the shared_ptr in connectionManager.shutdownThreads is destructed and since there is no other reference to the thread, the thread object is also destructed while the thread did not finish the runnableThread method. This fix simply do not delete the heap allocated shutdown thread. We assume that there will not be too many of such threads, and hence the leak can be ignored. --- .../client/connection/ClientConnectionManagerImpl.h | 2 -- .../connection/ClientConnectionManagerImpl.cpp | 12 +++++++++--- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/hazelcast/include/hazelcast/client/connection/ClientConnectionManagerImpl.h b/hazelcast/include/hazelcast/client/connection/ClientConnectionManagerImpl.h index 00f83e9ad8..ab2aced600 100644 --- a/hazelcast/include/hazelcast/client/connection/ClientConnectionManagerImpl.h +++ b/hazelcast/include/hazelcast/client/connection/ClientConnectionManagerImpl.h @@ -318,8 +318,6 @@ namespace hazelcast { util::Atomic > principal; std::auto_ptr connectionStrategy; boost::shared_ptr clusterConnectionExecutor; - // This queue is used for avoiding memory leak - util::SynchronizedQueue shutdownThreads; int32_t connectionAttemptPeriod; int32_t connectionAttemptLimit; bool shuffleMemberList; diff --git a/hazelcast/src/hazelcast/client/connection/ClientConnectionManagerImpl.cpp b/hazelcast/src/hazelcast/client/connection/ClientConnectionManagerImpl.cpp index 0020a50b72..215b3db5ea 100644 --- a/hazelcast/src/hazelcast/client/connection/ClientConnectionManagerImpl.cpp +++ b/hazelcast/src/hazelcast/client/connection/ClientConnectionManagerImpl.cpp @@ -774,10 +774,16 @@ namespace hazelcast { } catch (exception::IException &e) { connectionManager.logger.warning() << "Could not connect to cluster, shutting down the client. " << e.getMessage(); - boost::shared_ptr shutdownThread(new util::Thread( - boost::shared_ptr(new ShutdownTask(connectionManager.client)))); + /** + * Allocate this thread on heap. Since we do not want this thread to be deleted while thread is + * executing we can not keep it a shared pointer in the HazelcastClient (There is a possibility that + * it may be deleted during client shutdown and the object being destructed.). + * + * TODO: Find a way not to leak this object and also manage the lifecycle of the destructed objects. + */ + util::Thread *shutdownThread = new util::Thread( + boost::shared_ptr(new ShutdownTask(connectionManager.client))); shutdownThread->start(); - connectionManager.shutdownThreads.offer(shutdownThread); throw; } From b907fb64de0962b41e4ba76bb73d82bf006faa07 Mon Sep 17 00:00:00 2001 From: ihsandemir Date: Mon, 25 Jun 2018 23:56:20 +0300 Subject: [PATCH 2/2] Obtain the client name at the constructor of ShutdownTask to eliminate the illegal memory access when the client is shutdown e.g. during heartbeat failure and the client object is destructed already (the destructor waits on a latch which is released once the ShutdownTask::run is finished. Hence the ShutdownTask thread should not access the possibly destructed client on thread destructor where the name of the thread is written. fixes #428 fixes #429 Cleaned up unused header file and corrected the exception print at the ClientPartitionServiceImpl. --- .../connection/ClientConnectionManagerImpl.h | 1 + .../hazelcast/client/connection/HeartBeater.h | 62 ------------------- .../ClientConnectionManagerImpl.cpp | 7 ++- .../spi/impl/ClientPartitionServiceImpl.cpp | 2 +- 4 files changed, 6 insertions(+), 66 deletions(-) delete mode 100644 hazelcast/include/hazelcast/client/connection/HeartBeater.h diff --git a/hazelcast/include/hazelcast/client/connection/ClientConnectionManagerImpl.h b/hazelcast/include/hazelcast/client/connection/ClientConnectionManagerImpl.h index ab2aced600..b04901cee6 100644 --- a/hazelcast/include/hazelcast/client/connection/ClientConnectionManagerImpl.h +++ b/hazelcast/include/hazelcast/client/connection/ClientConnectionManagerImpl.h @@ -272,6 +272,7 @@ namespace hazelcast { private: spi::ClientContext &client; + std::string name; }; class TimeoutAuthenticationTask : public util::Runnable { diff --git a/hazelcast/include/hazelcast/client/connection/HeartBeater.h b/hazelcast/include/hazelcast/client/connection/HeartBeater.h deleted file mode 100644 index 8c9f86cc7c..0000000000 --- a/hazelcast/include/hazelcast/client/connection/HeartBeater.h +++ /dev/null @@ -1,62 +0,0 @@ -/* - * Copyright (c) 2008-2018, Hazelcast, Inc. All Rights Reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -// -// Created by sancar koyunlu on 20/08/14. -// - -#ifndef HAZELCAST_HeartBeater -#define HAZELCAST_HeartBeater - -#include "hazelcast/util/Runnable.h" - -#if defined(WIN32) || defined(_WIN32) || defined(WIN64) || defined(_WIN64) -#pragma warning(push) -#pragma warning(disable: 4251) //for dll export -#endif - -namespace hazelcast { - namespace util { - class Thread; - } - namespace client { - namespace spi{ - class ClientContext; - } - - namespace connection { - class HAZELCAST_API HeartBeater : public util::Runnable { - public: - HeartBeater(spi::ClientContext &clientContext, util::Thread ¤tThread); - - virtual void run(); - - virtual const std::string getName() const; - - private: - spi::ClientContext& clientContext; - int heartBeatIntervalSeconds; - int heartBeatTimeoutSeconds; - util::Thread ¤tThread; - }; - } - } -} - -#if defined(WIN32) || defined(_WIN32) || defined(WIN64) || defined(_WIN64) -#pragma warning(pop) -#endif - -#endif //HAZELCAST_HeartBeater diff --git a/hazelcast/src/hazelcast/client/connection/ClientConnectionManagerImpl.cpp b/hazelcast/src/hazelcast/client/connection/ClientConnectionManagerImpl.cpp index 215b3db5ea..a7862a68be 100644 --- a/hazelcast/src/hazelcast/client/connection/ClientConnectionManagerImpl.cpp +++ b/hazelcast/src/hazelcast/client/connection/ClientConnectionManagerImpl.cpp @@ -794,18 +794,19 @@ namespace hazelcast { } ClientConnectionManagerImpl::ShutdownTask::ShutdownTask(spi::ClientContext &client) - : client(client) {} + : client(client), name(client.getName() + ".clientShutdown-") {} void ClientConnectionManagerImpl::ShutdownTask::run() { try { client.getLifecycleService().shutdown(); } catch (exception::IException &exception) { - util::ILogger::getLogger().severe() << "Exception during client shutdown: " << exception; + util::ILogger::getLogger().severe() << "Exception during client shutdown task " << + name << ":" << exception; } } const std::string ClientConnectionManagerImpl::ShutdownTask::getName() const { - return client.getName() + ".clientShutdown-"; + return name; } void ClientConnectionManagerImpl::TimeoutAuthenticationTask::run() { diff --git a/hazelcast/src/hazelcast/client/spi/impl/ClientPartitionServiceImpl.cpp b/hazelcast/src/hazelcast/client/spi/impl/ClientPartitionServiceImpl.cpp index b2fa8d7e0d..558931b1f9 100644 --- a/hazelcast/src/hazelcast/client/spi/impl/ClientPartitionServiceImpl.cpp +++ b/hazelcast/src/hazelcast/client/spi/impl/ClientPartitionServiceImpl.cpp @@ -48,7 +48,7 @@ namespace hazelcast { const boost::shared_ptr &t) { if (partitionService.client.getLifecycleService().isRunning()) { partitionService.logger.warning() << "Error while fetching cluster partition table! Cause:" - << t; + << *t; } }