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 5120038 GEODE-9179: Solve possible race conditions in UTs (#787)
5120038 is described below
commit 5120038f675a1e30b382769e68dfda4217d88910
Author: Mario Salazar de Torres <[email protected]>
AuthorDate: Sat Apr 24 17:50:13 2021 +0200
GEODE-9179: Solve possible race conditions in UTs (#787)
- Solved a potential race condition in ExpiryTaskTest UTs TS.
condition_variable was replaced by a binary_semaphore.
- Implemented try_acquire_for binary_semaphore operation.
---
cppcache/src/util/concurrent/binary_semaphore.cpp | 10 +++++++
cppcache/src/util/concurrent/binary_semaphore.hpp | 1 +
cppcache/test/ExpiryTaskTest.cpp | 36 ++++++++++-------------
cppcache/test/gmock_extensions.h | 2 +-
4 files changed, 27 insertions(+), 22 deletions(-)
diff --git a/cppcache/src/util/concurrent/binary_semaphore.cpp
b/cppcache/src/util/concurrent/binary_semaphore.cpp
index f9dd465..74cb3e6 100644
--- a/cppcache/src/util/concurrent/binary_semaphore.cpp
+++ b/cppcache/src/util/concurrent/binary_semaphore.cpp
@@ -35,6 +35,16 @@ void binary_semaphore::acquire() {
released_ = false;
}
+bool binary_semaphore::try_acquire_for(
+ const std::chrono::milliseconds& period) {
+ std::unique_lock<std::mutex> lock(mutex_);
+ if (!cv_.wait_for(lock, period, [this]() { return released_; })) {
+ return false;
+ }
+
+ released_ = false;
+ return true;
+}
} // namespace client
} // namespace geode
} // namespace apache
diff --git a/cppcache/src/util/concurrent/binary_semaphore.hpp
b/cppcache/src/util/concurrent/binary_semaphore.hpp
index 5d1e802..4fc4b44 100644
--- a/cppcache/src/util/concurrent/binary_semaphore.hpp
+++ b/cppcache/src/util/concurrent/binary_semaphore.hpp
@@ -32,6 +32,7 @@ class binary_semaphore {
void release();
void acquire();
+ bool try_acquire_for(const std::chrono::milliseconds& period);
protected:
bool released_;
diff --git a/cppcache/test/ExpiryTaskTest.cpp b/cppcache/test/ExpiryTaskTest.cpp
index 9f0cc37..4788da8 100644
--- a/cppcache/test/ExpiryTaskTest.cpp
+++ b/cppcache/test/ExpiryTaskTest.cpp
@@ -22,12 +22,15 @@
#include "ExpiryTaskManager.hpp"
#include "gmock_extensions.h"
#include "mock/MockExpiryTask.hpp"
+#include "util/concurrent/binary_semaphore.hpp"
using ::testing::DoAll;
using ::testing::InvokeWithoutArgs;
using ::testing::Return;
using ::testing::Sequence;
+using apache::geode::client::binary_semaphore;
+
using apache::geode::client::ExpiryTask;
using apache::geode::client::ExpiryTaskManager;
using apache::geode::client::IllegalStateException;
@@ -36,31 +39,26 @@ using apache::geode::client::MockExpiryTask;
constexpr std::chrono::milliseconds DEFAULT_TIMEOUT{100};
TEST(ExpiryTaskTest, scheduleSingleshot) {
- std::mutex cv_mutex;
+ binary_semaphore sem{0};
ExpiryTaskManager manager;
- std::condition_variable cv;
- std::unique_lock<std::mutex> lock(cv_mutex);
-
EXPECT_NO_THROW(manager.start());
auto task = std::make_shared<MockExpiryTask>(manager);
EXPECT_CALL(*task, on_expire())
.Times(1)
- .WillOnce(DoAll(CvNotifyOne(&cv), Return(true)));
+ .WillOnce(DoAll(ReleaseSem(&sem), Return(true)));
auto id = manager.schedule(std::move(task), std::chrono::seconds(0));
EXPECT_NE(id, ExpiryTask::invalid());
- EXPECT_EQ(cv.wait_for(lock, DEFAULT_TIMEOUT), std::cv_status::no_timeout);
+ EXPECT_TRUE(sem.try_acquire_for(DEFAULT_TIMEOUT));
EXPECT_NO_THROW(manager.stop());
}
TEST(ExpiryTaskTest, schedulePeriodic) {
- std::mutex cv_mutex;
+ binary_semaphore sem{0};
ExpiryTaskManager manager;
- std::condition_variable cv;
- std::unique_lock<std::mutex> lock(cv_mutex);
EXPECT_NO_THROW(manager.start());
@@ -74,23 +72,21 @@ TEST(ExpiryTaskTest, schedulePeriodic) {
EXPECT_CALL(*task, on_expire())
.Times(1)
.InSequence(s)
- .WillOnce(DoAll(CvNotifyOne(&cv), Return(true)));
+ .WillOnce(DoAll(ReleaseSem(&sem), Return(true)));
EXPECT_CALL(*task, on_expire()).InSequence(s).WillRepeatedly(Return(true));
}
auto id = manager.schedule(std::move(task), std::chrono::seconds(0),
std::chrono::nanoseconds(1));
EXPECT_NE(id, ExpiryTask::invalid());
- EXPECT_EQ(cv.wait_for(lock, DEFAULT_TIMEOUT), std::cv_status::no_timeout);
+ EXPECT_TRUE(sem.try_acquire_for(DEFAULT_TIMEOUT));
EXPECT_NO_THROW(manager.stop());
}
TEST(ExpiryTaskTest, resetSingleshot) {
- std::mutex cv_mutex;
+ binary_semaphore sem{0};
ExpiryTaskManager manager;
- std::condition_variable cv;
- std::unique_lock<std::mutex> lock(cv_mutex);
EXPECT_NO_THROW(manager.start());
@@ -109,22 +105,20 @@ TEST(ExpiryTaskTest, resetSingleshot) {
EXPECT_CALL(task_ref, on_expire())
.Times(1)
.InSequence(s)
- .WillOnce(DoAll(CvNotifyOne(&cv), Return(true)));
+ .WillOnce(DoAll(ReleaseSem(&sem), Return(true)));
}
auto id = manager.schedule(std::move(task), std::chrono::seconds(0));
EXPECT_NE(id, ExpiryTask::invalid());
- EXPECT_EQ(cv.wait_for(lock, DEFAULT_TIMEOUT), std::cv_status::no_timeout);
+ EXPECT_TRUE(sem.try_acquire_for(DEFAULT_TIMEOUT));
EXPECT_NO_THROW(manager.stop());
}
TEST(ExpiryTaskTest, resetPeriodic) {
- std::mutex cv_mutex;
+ binary_semaphore sem{0};
ExpiryTaskManager manager;
- std::condition_variable cv;
- std::unique_lock<std::mutex> lock(cv_mutex);
EXPECT_NO_THROW(manager.start());
@@ -142,12 +136,12 @@ TEST(ExpiryTaskTest, resetPeriodic) {
}));
EXPECT_CALL(task_ref, on_expire())
.InSequence(s)
- .WillRepeatedly(DoAll(CvNotifyOne(&cv), Return(true)));
+ .WillRepeatedly(DoAll(ReleaseSem(&sem), Return(true)));
}
auto id = manager.schedule(std::move(task), std::chrono::seconds(0));
EXPECT_NE(id, ExpiryTask::invalid());
- EXPECT_EQ(cv.wait_for(lock, DEFAULT_TIMEOUT), std::cv_status::no_timeout);
+ EXPECT_TRUE(sem.try_acquire_for(DEFAULT_TIMEOUT));
EXPECT_NO_THROW(manager.stop());
}
diff --git a/cppcache/test/gmock_extensions.h b/cppcache/test/gmock_extensions.h
index 34b688f..8bca2d9 100644
--- a/cppcache/test/gmock_extensions.h
+++ b/cppcache/test/gmock_extensions.h
@@ -22,6 +22,6 @@
#include <gmock/gmock.h>
-ACTION_P(CvNotifyOne, cv) { cv->notify_one(); }
+ACTION_P(ReleaseSem, sem) { sem->release(); }
#endif // GEODE_GMOCK_EXTENSIONS_H_
