Skip to content

Commit d3a185b

Browse files
albertogpzBlake Bender
albertogpz
authored and
Blake Bender
committed
GEODE-7476: Fix several leaks in native client (#553)
* Fixed most leaks found when running the new integration tests under valgrind.
1 parent 13caf57 commit d3a185b

37 files changed

+418
-436
lines changed

‎cppcache/src/BucketServerLocation.hpp

+12-18
Original file line numberDiff line numberDiff line change
@@ -71,19 +71,16 @@ class BucketServerLocation : public ServerLocation {
7171
m_isPrimary(isPrimary),
7272
m_version(version) {
7373
int32_t size = static_cast<int32_t>(serverGroups.size());
74-
std::shared_ptr<CacheableString>* ptrArr = nullptr;
7574
if (size > 0) {
76-
ptrArr = new std::shared_ptr<CacheableString>[size];
77-
for (int i = 0; i < size; i++) {
78-
ptrArr[i] = CacheableString::create(serverGroups[i]);
75+
std::vector<std::shared_ptr<CacheableString>> tmpServerGroups;
76+
tmpServerGroups.reserve(size);
77+
for (auto&& serverGroup : serverGroups) {
78+
tmpServerGroups.emplace_back(CacheableString::create(serverGroup));
7979
}
80-
}
81-
if (size > 0) {
8280
if (size > 0x7f) {
8381
// TODO: should fail here since m_numServerGroups is int8_t?
8482
}
85-
m_serverGroups = CacheableStringArray::create(
86-
std::vector<std::shared_ptr<CacheableString>>(ptrArr, ptrArr + size));
83+
m_serverGroups = CacheableStringArray::create(std::move(tmpServerGroups));
8784
m_numServerGroups = static_cast<int8_t>(size);
8885
} else {
8986
m_serverGroups = nullptr;
@@ -104,8 +101,8 @@ class BucketServerLocation : public ServerLocation {
104101
output.write(m_version);
105102
output.write(static_cast<int8_t>(m_numServerGroups));
106103
if (m_numServerGroups > 0) {
107-
for (int i = 0; i < m_numServerGroups; i++) {
108-
output.writeObject(m_serverGroups->value()[i]);
104+
for (auto&& serverGroup : m_serverGroups->value()) {
105+
output.writeObject(serverGroup);
109106
}
110107
}
111108
}
@@ -116,17 +113,14 @@ class BucketServerLocation : public ServerLocation {
116113
m_isPrimary = input.readBoolean();
117114
m_version = input.read();
118115
m_numServerGroups = input.read();
119-
std::shared_ptr<CacheableString>* serverGroups = nullptr;
116+
120117
if (m_numServerGroups > 0) {
121-
serverGroups = new std::shared_ptr<CacheableString>[m_numServerGroups];
118+
std::vector<std::shared_ptr<CacheableString>> serverGroups;
119+
serverGroups.reserve(m_numServerGroups);
122120
for (int i = 0; i < m_numServerGroups; i++) {
123-
serverGroups[i] = CacheableString::create(input.readString());
121+
serverGroups.emplace_back(CacheableString::create(input.readString()));
124122
}
125-
}
126-
if (m_numServerGroups > 0) {
127-
m_serverGroups = CacheableStringArray::create(
128-
std::vector<std::shared_ptr<CacheableString>>(
129-
serverGroups, serverGroups + m_numServerGroups));
123+
m_serverGroups = CacheableStringArray::create(std::move(serverGroups));
130124
}
131125
}
132126

‎cppcache/src/CachePerfStats.hpp

+3-11
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ class APACHE_GEODE_EXPORT CachePerfStats {
4242

4343
if (statsType == nullptr) {
4444
const bool largerIsBetter = true;
45-
StatisticDescriptor** statDescArr = new StatisticDescriptor*[24];
45+
std::vector<std::shared_ptr<StatisticDescriptor>> statDescArr(24);
4646

4747
statDescArr[0] = factory->createIntCounter(
4848
"creates", "The total number of cache creates", "entries",
@@ -139,7 +139,7 @@ class APACHE_GEODE_EXPORT CachePerfStats {
139139

140140
statsType = factory->createType("CachePerfStats",
141141
"Statistics about native client cache",
142-
statDescArr, 24);
142+
std::move(statDescArr));
143143
}
144144
// Create Statistics object
145145
m_cachePerfStats =
@@ -206,15 +206,7 @@ class APACHE_GEODE_EXPORT CachePerfStats {
206206

207207
virtual ~CachePerfStats() { m_cachePerfStats = nullptr; }
208208

209-
void close() {
210-
/*StatisticDescriptor** statDescArr =
211-
m_cachePerfStats->getType()->getStatistics();
212-
for ( int i = 0; i < m_cachePerfStats->getType()->getDescriptorsCount();
213-
i++) {
214-
delete statDescArr[i];
215-
}*/
216-
m_cachePerfStats->close();
217-
}
209+
void close() { m_cachePerfStats->close(); }
218210

219211
inline void incDestroys() { m_cachePerfStats->incInt(m_destroysId, 1); }
220212

‎cppcache/src/CqQueryVsdStats.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ CqQueryVsdStats::CqQueryVsdStats(StatisticsFactory* factory,
4444
auto statsType = factory->findType(STATS_NAME);
4545
if (!statsType) {
4646
const bool largerIsBetter = true;
47-
auto stats = new StatisticDescriptor*[4];
47+
std::vector<std::shared_ptr<StatisticDescriptor>> stats(4);
4848
stats[0] = factory->createIntCounter(
4949
"inserts", "The total number of inserts this cq qurey", "entries",
5050
largerIsBetter);
@@ -58,7 +58,7 @@ CqQueryVsdStats::CqQueryVsdStats(StatisticsFactory* factory,
5858
"events", "The total number of events for this cq query", "entries",
5959
largerIsBetter);
6060

61-
statsType = factory->createType(STATS_NAME, STATS_DESC, stats, 4);
61+
statsType = factory->createType(STATS_NAME, STATS_DESC, std::move(stats));
6262
}
6363

6464
m_cqQueryVsdStats =

‎cppcache/src/CqServiceVsdStats.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ CqServiceVsdStats::CqServiceVsdStats(StatisticsFactory* factory,
4242
auto statsType = factory->findType(STATS_NAME);
4343
if (!statsType) {
4444
const bool largerIsBetter = true;
45-
auto stats = new StatisticDescriptor*[5];
45+
std::vector<std::shared_ptr<StatisticDescriptor>> stats(5);
4646
stats[0] = factory->createIntCounter(
4747
"CqsActive", "The total number of CqsActive this cq qurey", "entries",
4848
largerIsBetter);
@@ -60,7 +60,7 @@ CqServiceVsdStats::CqServiceVsdStats(StatisticsFactory* factory,
6060
"The total number of Cqs on the client for this cq Service", "entries",
6161
largerIsBetter);
6262

63-
statsType = factory->createType(STATS_NAME, STATS_DESC, stats, 5);
63+
statsType = factory->createType(STATS_NAME, STATS_DESC, std::move(stats));
6464
}
6565

6666
m_cqServiceVsdStats =

‎cppcache/src/ExecutionImpl.cpp

+3-3
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ Execution ExecutionImpl::withCollector(std::shared_ptr<ResultCollector> rs) {
7373
m_authenticatedView)));
7474
}
7575

76-
std::vector<int8_t>* ExecutionImpl::getFunctionAttributes(
76+
std::shared_ptr<std::vector<int8_t>> ExecutionImpl::getFunctionAttributes(
7777
const std::string& func) {
7878
auto&& itr = m_func_attrs.find(func);
7979
if (itr != m_func_attrs.end()) {
@@ -360,8 +360,8 @@ std::shared_ptr<ResultCollector> ExecutionImpl::execute(
360360
return m_rc;
361361
}
362362

363-
GfErrType ExecutionImpl::getFuncAttributes(const std::string& func,
364-
std::vector<int8_t>** attr) {
363+
GfErrType ExecutionImpl::getFuncAttributes(
364+
const std::string& func, std::shared_ptr<std::vector<int8_t>>* attr) {
365365
ThinClientPoolDM* tcrdm = dynamic_cast<ThinClientPoolDM*>(m_pool.get());
366366
if (tcrdm == nullptr) {
367367
throw IllegalArgumentException(

‎cppcache/src/ExecutionImpl.hpp

+4-3
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ namespace apache {
3535
namespace geode {
3636
namespace client {
3737

38-
typedef std::map<std::string, std::vector<int8_t>*>
38+
typedef std::map<std::string, std::shared_ptr<std::vector<int8_t>>>
3939
FunctionToFunctionAttributes;
4040

4141
class ExecutionImpl {
@@ -119,9 +119,10 @@ class ExecutionImpl {
119119
const std::string& func, uint8_t getResult,
120120
std::chrono::milliseconds timeout = DEFAULT_QUERY_RESPONSE_TIMEOUT);
121121

122-
std::vector<int8_t>* getFunctionAttributes(const std::string& func);
122+
std::shared_ptr<std::vector<int8_t>> getFunctionAttributes(
123+
const std::string& func);
123124
GfErrType getFuncAttributes(const std::string& func,
124-
std::vector<int8_t>** attr);
125+
std::shared_ptr<std::vector<int8_t>>* attr);
125126
};
126127
} // namespace client
127128
} // namespace geode

‎cppcache/src/ExpiryTaskManager.cpp

+5-6
Original file line numberDiff line numberDiff line change
@@ -38,16 +38,14 @@ namespace client {
3838
const char* ExpiryTaskManager::NC_ETM_Thread = "NC ETM Thread";
3939

4040
ExpiryTaskManager::ExpiryTaskManager() : m_reactorEventLoopRunning(false) {
41-
auto timer = std::unique_ptr<GF_Timer_Heap_ImmediateReset>(
42-
new GF_Timer_Heap_ImmediateReset());
41+
auto timer = new GF_Timer_Heap_ImmediateReset();
4342
#if defined(_WIN32)
44-
m_reactor = new ACE_Reactor(new ACE_WFMO_Reactor(nullptr, timer.get()), 1);
43+
m_reactor = new ACE_Reactor(new ACE_WFMO_Reactor(nullptr, timer), 1);
4544
#elif defined(WITH_ACE_Select_Reactor)
46-
m_reactor = new ACE_Reactor(new ACE_Select_Reactor(nullptr, timer.get()), 1);
45+
m_reactor = new ACE_Reactor(new ACE_Select_Reactor(nullptr, timer), 1);
4746
#else
48-
m_reactor = new ACE_Reactor(new ACE_Dev_Poll_Reactor(nullptr, timer.get()) 1);
47+
m_reactor = new ACE_Reactor(new ACE_Dev_Poll_Reactor(nullptr, timer) 1);
4948
#endif
50-
timer.release();
5149
}
5250

5351
int ExpiryTaskManager::resetTask(ExpiryTaskManager::id_type id, uint32_t sec) {
@@ -92,6 +90,7 @@ void ExpiryTaskManager::begin() {
9290

9391
ExpiryTaskManager::~ExpiryTaskManager() {
9492
stopExpiryTaskManager();
93+
9594
delete m_reactor;
9695
m_reactor = nullptr;
9796
}

‎cppcache/src/PoolStatistics.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ PoolStats::PoolStats(StatisticsFactory* factory, const std::string& poolName) {
4343
auto statsType = factory->findType(STATS_NAME);
4444

4545
if (statsType == nullptr) {
46-
auto stats = new StatisticDescriptor*[27];
46+
std::vector<std::shared_ptr<StatisticDescriptor>> stats(27);
4747

4848
stats[0] = factory->createIntGauge(
4949
"locators", "Current number of locators discovered", "locators");
@@ -133,7 +133,7 @@ PoolStats::PoolStats(StatisticsFactory* factory, const std::string& poolName) {
133133
"queryExecutionTime",
134134
"Total time spent while processing queryExecution", "nanoseconds");
135135

136-
statsType = factory->createType(STATS_NAME, STATS_DESC, stats, 27);
136+
statsType = factory->createType(STATS_NAME, STATS_DESC, std::move(stats));
137137
}
138138
m_locatorsId = statsType->nameToId("locators");
139139
m_serversId = statsType->nameToId("servers");

‎cppcache/src/RegionStats.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ RegionStats::RegionStats(StatisticsFactory* factory,
4040

4141
if (!statsType) {
4242
const bool largerIsBetter = true;
43-
auto stats = new StatisticDescriptor*[25];
43+
std::vector<std::shared_ptr<StatisticDescriptor>> stats(25);
4444
stats[0] = factory->createIntCounter(
4545
"creates", "The total number of cache creates for this region",
4646
"entries", largerIsBetter);
@@ -135,7 +135,7 @@ RegionStats::RegionStats(StatisticsFactory* factory,
135135
"removeAllTime",
136136
"Total time spent doing removeAlls operations for this region",
137137
"Nanoseconds", !largerIsBetter);
138-
statsType = factory->createType(STATS_NAME, STATS_DESC, stats, 25);
138+
statsType = factory->createType(STATS_NAME, STATS_DESC, std::move(stats));
139139
}
140140

141141
m_destroysId = statsType->nameToId("destroys");

‎cppcache/src/TcrConnection.cpp

+12-21
Original file line numberDiff line numberDiff line change
@@ -1112,29 +1112,25 @@ std::vector<int8_t> TcrConnection::readHandshakeData(
11121112
if (msgLength < 0) {
11131113
msgLength = 0;
11141114
}
1115-
char* recvMessage;
1116-
_GEODE_NEW(recvMessage, char[msgLength + 1]);
1117-
recvMessage[msgLength] = '\0';
1115+
std::vector<int8_t> message(msgLength + 1);
1116+
message.data()[msgLength] = '\0';
11181117
if (msgLength == 0) {
1119-
return std::vector<int8_t>(recvMessage, recvMessage + 1);
1118+
return message;
11201119
}
1121-
if ((error = receiveData(recvMessage, msgLength, connectTimeout, false)) !=
1122-
CONN_NOERR) {
1120+
if ((error = receiveData(reinterpret_cast<char*>(message.data()), msgLength,
1121+
connectTimeout, false)) != CONN_NOERR) {
1122+
GF_SAFE_DELETE_CON(m_conn);
11231123
if (error & CONN_TIMEOUT) {
1124-
_GEODE_SAFE_DELETE_ARRAY(recvMessage);
1125-
GF_SAFE_DELETE_CON(m_conn);
11261124
throwException(
11271125
TimeoutException("TcrConnection::TcrConnection: "
11281126
"Timeout in handshake"));
11291127
} else {
1130-
_GEODE_SAFE_DELETE_ARRAY(recvMessage);
1131-
GF_SAFE_DELETE_CON(m_conn);
11321128
throwException(
11331129
GeodeIOException("TcrConnection::TcrConnection: "
11341130
"Handshake failure"));
11351131
}
11361132
} else {
1137-
return std::vector<int8_t>(recvMessage, recvMessage + msgLength + 1);
1133+
return message;
11381134
}
11391135
}
11401136

@@ -1147,26 +1143,21 @@ std::shared_ptr<CacheableBytes> TcrConnection::readHandshakeRawData(
11471143
if (msgLength == 0) {
11481144
return nullptr;
11491145
}
1150-
char* recvMessage;
1151-
_GEODE_NEW(recvMessage, char[msgLength]);
1152-
if ((error = receiveData(recvMessage, msgLength, connectTimeout, false)) !=
1153-
CONN_NOERR) {
1146+
std::vector<int8_t> message(msgLength);
1147+
if ((error = receiveData(reinterpret_cast<char*>(message.data()), msgLength,
1148+
connectTimeout, false)) != CONN_NOERR) {
1149+
GF_SAFE_DELETE_CON(m_conn);
11541150
if (error & CONN_TIMEOUT) {
1155-
_GEODE_SAFE_DELETE_ARRAY(recvMessage);
1156-
GF_SAFE_DELETE_CON(m_conn);
11571151
throwException(
11581152
TimeoutException("TcrConnection::TcrConnection: "
11591153
"Timeout in handshake"));
11601154
} else {
1161-
_GEODE_SAFE_DELETE_ARRAY(recvMessage);
1162-
GF_SAFE_DELETE_CON(m_conn);
11631155
throwException(
11641156
GeodeIOException("TcrConnection::TcrConnection: "
11651157
"Handshake failure"));
11661158
}
11671159
} else {
1168-
return CacheableBytes::create(
1169-
std::vector<int8_t>(recvMessage, recvMessage + msgLength));
1160+
return CacheableBytes::create(std::move(message));
11701161
}
11711162
}
11721163

‎cppcache/src/TcrConnectionManager.cpp

+2-1
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,8 @@ TcrConnectionManager::TcrConnectionManager(CacheImpl *cache)
6767
m_redundancyTask(nullptr),
6868
m_isDurable(false),
6969
m_isNetDown(false) {
70-
m_redundancyManager = new ThinClientRedundancyManager(this);
70+
m_redundancyManager = std::unique_ptr<ThinClientRedundancyManager>(
71+
new ThinClientRedundancyManager(this));
7172
}
7273

7374
ExpiryTaskManager::id_type TcrConnectionManager::getPingTaskId() {

‎cppcache/src/TcrConnectionManager.hpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -179,7 +179,7 @@ class TcrConnectionManager {
179179

180180
bool m_isNetDown;
181181

182-
ThinClientRedundancyManager* m_redundancyManager;
182+
std::unique_ptr<ThinClientRedundancyManager> m_redundancyManager;
183183

184184
void failover(std::atomic<bool>& isRunning);
185185
void redundancy(std::atomic<bool>& isRunning);

‎cppcache/src/TcrMessage.cpp

+4-2
Original file line numberDiff line numberDiff line change
@@ -239,7 +239,7 @@ int8_t TcrMessage::getserverGroupVersion() const {
239239
return m_serverGroupVersion;
240240
}
241241

242-
std::vector<int8_t>* TcrMessage::getFunctionAttributes() {
242+
std::shared_ptr<std::vector<int8_t>> TcrMessage::getFunctionAttributes() {
243243
return m_functionAttributes;
244244
}
245245

@@ -1206,7 +1206,9 @@ void TcrMessage::handleByteArrayResponse(
12061206
input.readInt32();
12071207
input.advanceCursor(1); // ignore byte
12081208

1209-
m_functionAttributes = new std::vector<int8_t>();
1209+
if (!m_functionAttributes) {
1210+
m_functionAttributes = std::make_shared<std::vector<int8_t>>();
1211+
}
12101212
m_functionAttributes->push_back(input.read());
12111213
m_functionAttributes->push_back(input.read());
12121214
m_functionAttributes->push_back(input.read());

‎cppcache/src/TcrMessage.hpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -275,7 +275,7 @@ class TcrMessage {
275275
int8_t getMetaDataVersion() const;
276276
uint32_t getEntryNotFound() const;
277277
int8_t getserverGroupVersion() const;
278-
std::vector<int8_t>* getFunctionAttributes();
278+
std::shared_ptr<std::vector<int8_t>> getFunctionAttributes();
279279

280280
// set the DM for chunked response messages
281281
void setDM(ThinClientBaseDM* dm);
@@ -420,7 +420,7 @@ class TcrMessage {
420420
std::unique_ptr<DataInput> m_delta;
421421
int8_t* m_deltaBytes;
422422
std::vector<std::shared_ptr<FixedPartitionAttributesImpl>>* m_fpaSet;
423-
std::vector<int8_t>* m_functionAttributes;
423+
std::shared_ptr<std::vector<int8_t>> m_functionAttributes;
424424
std::shared_ptr<CacheableBytes> m_connectionIDBytes;
425425
std::shared_ptr<Properties> m_creds;
426426
std::shared_ptr<CacheableKey> m_key;

0 commit comments

Comments
 (0)