You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@geode.apache.org by bb...@apache.org on 2020/10/27 18:22:31 UTC
[geode-native] branch develop updated: GEODE-8531: Fix coredump in
MapSegment::remove (#667)
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 3e9adc8 GEODE-8531: Fix coredump in MapSegment::remove (#667)
3e9adc8 is described below
commit 3e9adc8adeebacbeb414708448bf58d98cad0ced
Author: Mario Salazar de Torres <ma...@est.tech>
AuthorDate: Tue Oct 27 19:22:23 2020 +0100
GEODE-8531: Fix coredump in MapSegment::remove (#667)
- Fixed a coredump in MapSegment::remove whenever the the region holding the
MapSegment object had consistency checks disabled.
- Added an integration test in RegisterKeysTest TS as it was were this issue
was detected. Test approach is to setup a g-mocked cache listener for the listener region and set
a call expectation on afterDestroy. All of that giving a timeout of 5seconds for the call to be completed.
- With this approach the complete 5 seconds will be only spent in the event of an error.
- Also linked gmock to the new integration tests target.
---
cppcache/integration/test/CMakeLists.txt | 1 +
cppcache/integration/test/RegisterKeysTest.cpp | 74 ++++++++++++++++++++++++--
cppcache/src/MapSegment.cpp | 10 +++-
3 files changed, 80 insertions(+), 5 deletions(-)
diff --git a/cppcache/integration/test/CMakeLists.txt b/cppcache/integration/test/CMakeLists.txt
index be68ea1..fd6ebc5 100644
--- a/cppcache/integration/test/CMakeLists.txt
+++ b/cppcache/integration/test/CMakeLists.txt
@@ -70,6 +70,7 @@ target_link_libraries(cpp-integration-test
ACE::ACE
GTest::gtest
GTest::gtest_main
+ GTest::gmock
Boost::boost
Boost::system
Boost::log
diff --git a/cppcache/integration/test/RegisterKeysTest.cpp b/cppcache/integration/test/RegisterKeysTest.cpp
index 58eb113..0c1a6fc 100644
--- a/cppcache/integration/test/RegisterKeysTest.cpp
+++ b/cppcache/integration/test/RegisterKeysTest.cpp
@@ -14,13 +14,16 @@
* limitations under the License.
*/
-#include <iostream>
+#include <gmock/gmock.h>
+
+#include <condition_variable>
+#include <mutex>
#include <gtest/gtest.h>
#include <geode/Cache.hpp>
#include <geode/CacheFactory.hpp>
-#include <geode/PoolManager.hpp>
+#include <geode/EntryEvent.hpp>
#include <geode/RegionFactory.hpp>
#include <geode/RegionShortcut.hpp>
@@ -28,6 +31,12 @@
#include "framework/Framework.h"
#include "framework/Gfsh.h"
+class CacheListenerMock : public apache::geode::client::CacheListener {
+ public:
+ MOCK_METHOD1(afterDestroy,
+ void(const apache::geode::client::EntryEvent& event));
+};
+
namespace {
using apache::geode::client::Cache;
@@ -38,6 +47,9 @@ using apache::geode::client::CacheFactory;
using apache::geode::client::IllegalStateException;
using apache::geode::client::Region;
using apache::geode::client::RegionShortcut;
+using ::testing::_;
+
+ACTION_P(CvNotifyOne, cv) { cv->notify_one(); }
Cache createTestCache() {
CacheFactory cacheFactory;
@@ -46,9 +58,11 @@ Cache createTestCache() {
.create();
}
-std::shared_ptr<Region> setupCachingProxyRegion(Cache& cache) {
+std::shared_ptr<Region> setupCachingProxyRegion(Cache& cache,
+ bool consistency = true) {
auto region = cache.createRegionFactory(RegionShortcut::CACHING_PROXY)
.setPoolName("default")
+ .setConcurrencyChecksEnabled(consistency)
.create("region");
return region;
@@ -112,6 +126,60 @@ TEST(RegisterKeysTest, RegisterAllWithCachingRegion) {
}
}
+TEST(RegisterKeysTest, RegisterAllWithConsistencyDisabled) {
+ Cluster cluster{LocatorCount{1}, ServerCount{1}};
+
+ cluster.start();
+
+ cluster.getGfsh()
+ .create()
+ .region()
+ .withName("region")
+ .withType("PARTITION")
+ .execute();
+
+ auto producer_cache = createTestCache();
+ auto listener_cache = createTestCache();
+ std::shared_ptr<Region> producer_region;
+ std::shared_ptr<Region> listener_region;
+
+ {
+ auto poolFactory = producer_cache.getPoolManager().createFactory();
+ cluster.applyLocators(poolFactory);
+ poolFactory.create("default");
+ producer_region = setupProxyRegion(producer_cache);
+ }
+
+ auto listener = std::make_shared<CacheListenerMock>();
+ {
+ auto poolFactory =
+ listener_cache.getPoolManager().createFactory().setSubscriptionEnabled(
+ true);
+ cluster.applyLocators(poolFactory);
+ poolFactory.create("default");
+ listener_region =
+ listener_cache.createRegionFactory(RegionShortcut::CACHING_PROXY)
+ .setPoolName("default")
+ .setCacheListener(listener)
+ .setConcurrencyChecksEnabled(false)
+ .create("region");
+ listener_region->registerAllKeys();
+ }
+
+ producer_region->put("one", std::make_shared<CacheableInt16>(1));
+ producer_region->destroy("one");
+
+ std::mutex cv_mutex;
+ std::condition_variable cv;
+ EXPECT_CALL(*listener, afterDestroy(_)).Times(1).WillOnce(CvNotifyOne(&cv));
+
+ {
+ std::unique_lock<std::mutex> lock(cv_mutex);
+ EXPECT_EQ(cv.wait_for(lock, std::chrono::seconds(5)),
+ std::cv_status::no_timeout);
+ }
+}
+
TEST(RegisterKeysTest, RegisterAnyWithCachingRegion) {
Cluster cluster{LocatorCount{1}, ServerCount{1}};
diff --git a/cppcache/src/MapSegment.cpp b/cppcache/src/MapSegment.cpp
index ce88114..36ef62f 100644
--- a/cppcache/src/MapSegment.cpp
+++ b/cppcache/src/MapSegment.cpp
@@ -332,7 +332,6 @@ GfErrType MapSegment::remove(const std::shared_ptr<CacheableKey>& key,
std::shared_ptr<MapEntryImpl>& me, int updateCount,
std::shared_ptr<VersionTag> versionTag,
bool afterRemote, bool& isEntryFound) {
- std::shared_ptr<MapEntry> entry;
if (m_concurrencyChecksEnabled) {
TombstoneExpiryHandler* handler;
auto id = m_tombstoneList->getExpiryTask(&handler);
@@ -353,7 +352,9 @@ GfErrType MapSegment::remove(const std::shared_ptr<CacheableKey>& key,
}
std::lock_guard<decltype(m_spinlock)> lk(m_spinlock);
- if (m_map->erase(key) == 0) {
+ auto&& iter = m_map->find(key);
+
+ if (iter == m_map->end()) {
// didn't unbind, probably no entry...
oldValue = nullptr;
volatile int destroyTrackers = *m_numDestroyTrackers;
@@ -363,16 +364,21 @@ GfErrType MapSegment::remove(const std::shared_ptr<CacheableKey>& key,
return GF_CACHE_ENTRY_NOT_FOUND;
}
+ auto entry = iter->second;
+ m_map->erase(iter);
+
if (updateCount >= 0 && updateCount != entry->getUpdateCount()) {
// this is the case when entry has been updated while being tracked
return GF_CACHE_ENTRY_UPDATED;
}
+
auto entryImpl = entry->getImplPtr();
entryImpl->getValueI(oldValue);
if (CacheableToken::isTombstone(oldValue)) oldValue = nullptr;
if (oldValue) {
me = entryImpl;
}
+
return GF_NOERR;
}