This is an automated email from the ASF dual-hosted git repository.
bbender pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/geode-native.git
The following commit(s) were added to refs/heads/develop by this push:
new bb40163 GEODE-7299: Fix PdxInstanceImpl memory leak (#537)
bb40163 is described below
commit bb40163f5eb900b0ca18512d619e17ca88d13c65
Author: Blake Bender <[email protected]>
AuthorDate: Thu Oct 17 18:47:11 2019 +0000
GEODE-7299: Fix PdxInstanceImpl memory leak (#537)
* GEODE-7299: Fix memory leak in PdxInstanceImpl::updatePdxStream
- include a unit test which will show the leak when run under leak-checking
app, e.g. Valgrind
- m_buffer member variable changed to a std::vector
- m_bufferLength member variable no longer needed, removed
Co-authored-by: Mike Martell <[email protected]>
---
cppcache/src/PdxInstanceImpl.cpp | 59 +++++++++++++++--------------
cppcache/src/PdxInstanceImpl.hpp | 5 +--
cppcache/test/PdxInstanceImplTest.cpp | 70 +++++++++++++++++++++++++++++++++++
3 files changed, 101 insertions(+), 33 deletions(-)
diff --git a/cppcache/src/PdxInstanceImpl.cpp b/cppcache/src/PdxInstanceImpl.cpp
index 0796320..2bfa8d4 100644
--- a/cppcache/src/PdxInstanceImpl.cpp
+++ b/cppcache/src/PdxInstanceImpl.cpp
@@ -59,24 +59,21 @@ bool sortFunc(std::shared_ptr<PdxFieldType> field1,
}
}
-PdxInstanceImpl::~PdxInstanceImpl() noexcept {
- _GEODE_SAFE_DELETE_ARRAY(m_buffer);
-}
+PdxInstanceImpl::~PdxInstanceImpl() noexcept {}
-PdxInstanceImpl::PdxInstanceImpl(uint8_t* buffer, int length, int typeId,
- CachePerfStats& cacheStats,
+PdxInstanceImpl::PdxInstanceImpl(const uint8_t* buffer, size_t length,
+ int typeId, CachePerfStats& cacheStats,
PdxTypeRegistry& pdxTypeRegistry,
const CacheImpl& cacheImpl,
bool enableTimeStatistics)
- : m_buffer(DataInput::getBufferCopy(buffer, length)),
- m_bufferLength(length),
+ : m_buffer(buffer, buffer + length),
m_typeId(typeId),
m_pdxType(nullptr),
m_cacheStats(cacheStats),
m_pdxTypeRegistry(pdxTypeRegistry),
m_cacheImpl(cacheImpl),
m_enableTimeStatistics(enableTimeStatistics) {
- LOGDEBUG("PdxInstanceImpl::m_bufferLength = %d ", m_bufferLength);
+ LOGDEBUG("PdxInstanceImpl::m_bufferLength = %zu ", m_buffer.size());
}
PdxInstanceImpl::PdxInstanceImpl(FieldVsValues fieldVsValue,
@@ -85,9 +82,7 @@ PdxInstanceImpl::PdxInstanceImpl(FieldVsValues fieldVsValue,
PdxTypeRegistry& pdxTypeRegistry,
const CacheImpl& cacheImpl,
bool enableTimeStatistics)
- : m_buffer(nullptr),
- m_bufferLength(0),
- m_typeId(0),
+ : m_typeId(0),
m_pdxType(pdxType),
m_updatedFields(fieldVsValue),
m_cacheStats(cacheStats),
@@ -253,11 +248,11 @@ void PdxInstanceImpl::writeField(PdxWriter& writer,
}
}
std::shared_ptr<WritablePdxInstance> PdxInstanceImpl::createWriter() {
- LOGDEBUG("PdxInstanceImpl::createWriter m_bufferLength = %d m_typeId = %d ",
- m_bufferLength, m_typeId);
+ LOGDEBUG("PdxInstanceImpl::createWriter m_bufferLength = %zu m_typeId = %d ",
+ m_buffer.size(), m_typeId);
return std::make_shared<PdxInstanceImpl>(
- m_buffer, m_bufferLength, m_typeId, m_cacheStats, m_pdxTypeRegistry,
- m_cacheImpl,
+ m_buffer.data(), m_buffer.size(), m_typeId, m_cacheStats,
+ m_pdxTypeRegistry, m_cacheImpl,
m_enableTimeStatistics); // need to create duplicate byte stream);
}
@@ -647,7 +642,8 @@ int32_t PdxInstanceImpl::hashcode() const {
auto pdxIdentityFieldList = getIdentityPdxFields(pt);
- auto dataInput = m_cacheImpl.createDataInput(m_buffer, m_bufferLength);
+ auto dataInput =
+ m_cacheImpl.createDataInput(m_buffer.data(), m_buffer.size());
for (uint32_t i = 0; i < pdxIdentityFieldList.size(); i++) {
auto pField = pdxIdentityFieldList.at(i);
@@ -709,9 +705,10 @@ int32_t PdxInstanceImpl::hashcode() const {
}
void PdxInstanceImpl::updatePdxStream(uint8_t* newPdxStream, int len) {
- m_buffer = DataInput::getBufferCopy(newPdxStream, len);
- m_bufferLength = len;
+ m_buffer.resize(len);
+ memcpy(m_buffer.data(), newPdxStream, len);
}
+
std::shared_ptr<PdxType> PdxInstanceImpl::getPdxType() const {
if (m_typeId == 0) {
if (m_pdxType == nullptr) {
@@ -1060,11 +1057,13 @@ std::string PdxInstanceImpl::toString() const {
}
std::shared_ptr<PdxSerializable> PdxInstanceImpl::getObject() {
- auto dataInput = m_cacheImpl.createDataInput(m_buffer, m_bufferLength);
+ auto dataInput =
+ m_cacheImpl.createDataInput(m_buffer.data(), m_buffer.size());
int64_t sampleStartNanos =
m_enableTimeStatistics ? Utils::startStatOpTime() : 0;
//[ToDo] do we have to call incPdxDeSerialization here?
- auto ret = PdxHelper::deserializePdx(dataInput, m_typeId, m_bufferLength);
+ auto ret = PdxHelper::deserializePdx(dataInput, m_typeId,
+ static_cast<int32_t>(m_buffer.size()));
if (m_enableTimeStatistics) {
Utils::updateStatOpTime(m_cacheStats.getStat(),
@@ -1133,9 +1132,10 @@ bool PdxInstanceImpl::operator==(const CacheableKey&
other) const {
equatePdxFields(myPdxIdentityFieldList, otherPdxIdentityFieldList);
equatePdxFields(otherPdxIdentityFieldList, myPdxIdentityFieldList);
- auto myDataInput = m_cacheImpl.createDataInput(m_buffer, m_bufferLength);
- auto otherDataInput =
- m_cacheImpl.createDataInput(otherPdx->m_buffer,
otherPdx->m_bufferLength);
+ auto myDataInput =
+ m_cacheImpl.createDataInput(m_buffer.data(), m_buffer.size());
+ auto otherDataInput = m_cacheImpl.createDataInput(otherPdx->m_buffer.data(),
+ otherPdx->m_buffer.size());
PdxFieldTypes fieldTypeId;
for (size_t i = 0; i < myPdxIdentityFieldList.size(); i++) {
@@ -1334,10 +1334,9 @@ void PdxInstanceImpl::toDataMutable(PdxWriter& writer) {
pt->getPdxFieldTypes();
int position = 0; // ignore typeid and length
int nextFieldPosition = 0;
- if (m_buffer != nullptr) {
- uint8_t* copy = apache::geode::client::DataInput::getBufferCopy(
- m_buffer, m_bufferLength);
- auto dataInput = m_cacheImpl.createDataInput(copy, m_bufferLength);
+ if (m_buffer.size() != 0) {
+ auto dataInput =
+ m_cacheImpl.createDataInput(m_buffer.data(), m_buffer.size());
for (size_t i = 0; i < pdxFieldList->size(); i++) {
auto currPf = pdxFieldList->at(i);
LOGDEBUG("toData filedname = %s , isVarLengthType = %d ",
@@ -1366,7 +1365,6 @@ void PdxInstanceImpl::toDataMutable(PdxWriter& writer) {
position = nextFieldPosition; // mark next field;
}
}
- _GEODE_SAFE_DELETE_ARRAY(copy);
} else {
for (size_t i = 0; i < pdxFieldList->size(); i++) {
auto currPf = pdxFieldList->at(i);
@@ -1946,7 +1944,7 @@ void PdxInstanceImpl::setOffsetForObject(DataInput&
dataInput,
size_t PdxInstanceImpl::objectSize() const {
auto size = sizeof(PdxInstanceImpl);
- size += m_bufferLength;
+ size += m_buffer.size();
size += m_pdxType->objectSize();
for (FieldVsValues::const_iterator iter = m_updatedFields.begin();
iter != m_updatedFields.end(); ++iter) {
@@ -1969,7 +1967,8 @@ DataInput PdxInstanceImpl::getDataInputForField(
throw IllegalStateException("PdxInstance doesn't have field " + fieldname);
}
- auto dataInput = m_cacheImpl.createDataInput(m_buffer, m_bufferLength);
+ auto dataInput =
+ m_cacheImpl.createDataInput(m_buffer.data(), m_buffer.size());
auto pos = getOffset(dataInput, pt, pft->getSequenceId());
dataInput.reset();
diff --git a/cppcache/src/PdxInstanceImpl.hpp b/cppcache/src/PdxInstanceImpl.hpp
index b9b3cc0..4f527d2 100644
--- a/cppcache/src/PdxInstanceImpl.hpp
+++ b/cppcache/src/PdxInstanceImpl.hpp
@@ -199,7 +199,7 @@ class APACHE_GEODE_EXPORT PdxInstanceImpl : public
WritablePdxInstance {
* @brief constructors
*/
- PdxInstanceImpl(uint8_t* buffer, int length, int typeId,
+ PdxInstanceImpl(const uint8_t* buffer, size_t length, int typeId,
CachePerfStats& cacheStats, PdxTypeRegistry& pdxTypeRegistry,
const CacheImpl& cacheImpl, bool enableTimeStatistics);
@@ -216,8 +216,7 @@ class APACHE_GEODE_EXPORT PdxInstanceImpl : public
WritablePdxInstance {
void updatePdxStream(uint8_t* newPdxStream, int len);
private:
- uint8_t* m_buffer;
- int m_bufferLength;
+ std::vector<uint8_t> m_buffer;
int m_typeId;
std::shared_ptr<PdxType> m_pdxType;
FieldVsValues m_updatedFields;
diff --git a/cppcache/test/PdxInstanceImplTest.cpp
b/cppcache/test/PdxInstanceImplTest.cpp
new file mode 100644
index 0000000..604b46f
--- /dev/null
+++ b/cppcache/test/PdxInstanceImplTest.cpp
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You 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.
+ */
+
+#include <CachePerfStats.hpp>
+#include <PdxInstanceImpl.hpp>
+#include <statistics/StatisticsFactory.hpp>
+
+#include <gtest/gtest.h>
+
+#include <geode/AuthenticatedView.hpp>
+#include <geode/Cache.hpp>
+#include <geode/PoolManager.hpp>
+#include <geode/Properties.hpp>
+#include <geode/RegionFactory.hpp>
+#include <geode/RegionShortcut.hpp>
+
+using apache::geode::client::Cache;
+using apache::geode::client::CacheFactory;
+using apache::geode::client::CacheImpl;
+using apache::geode::client::CachePerfStats;
+using apache::geode::client::PdxInstanceImpl;
+using apache::geode::client::Properties;
+using apache::geode::statistics::StatisticsFactory;
+
+#define __1K__ 1024
+#define __100K__ (100 * __1K__)
+#define __1M__ (__1K__ * __1K__)
+
+//
+// Test to check for memory leak in PdxInstanceImpl::updatePdxStream. This
+// method was leaking a buffer equivalent in size to the passed-in buffer on
+// each call. Test will still pass without the fix in updatePdxStream, but it
+// will take *much* longer to complete (60sec+ vs ~4sec on a typical machine).
+// Still, to actually verify the fix, you will have to run this test under a
+// memory-checking framework of some kind, such as Valgrind or XCode's
+// Instruments.
+//
+TEST(PdxInstanceImplTest, updatePdxStream) {
+ auto properties = std::make_shared<Properties>();
+ CacheFactory cacheFactory;
+ auto cache = cacheFactory.create();
+ CacheImpl cacheImpl(&cache, properties, true, false, nullptr);
+ auto buffer = std::vector<uint8_t>(__1M__, 0xcc);
+ auto len = static_cast<int32_t>(buffer.size());
+ PdxInstanceImpl pdxInstanceImpl(
+ buffer.data(), len, 0xdeadbeef, cacheImpl.getCachePerfStats(),
+ *(cacheImpl.getPdxTypeRegistry()), cacheImpl, false);
+
+ for (auto i = 0; i < __100K__; i++) {
+ try {
+ pdxInstanceImpl.updatePdxStream(buffer.data(), len);
+ } catch (const std::exception&) {
+ GTEST_FAIL();
+ }
+ }
+}
