Skip to content

Commit

Permalink
Fix hang timer tests on macos (facebook#7208)
Browse files Browse the repository at this point in the history 
Summary:
And re-enable disabled tests.
The issue is caused by `CondVar.TimedWait()` doesn't use `MockTimeEnv`.

Issue: facebook#6698

Pull Request resolved: facebook#7208

Test Plan: `./timer_test --gtest_repeat=1000`

Reviewed By: riversand963

Differential Revision: D22857855

Pulled By: jay-zhuang

fbshipit-source-id: 6d15f65f6ae58b75b76cb132815c16ad81ffd12f
  • Loading branch information
@jay-zhuang @facebook-github-bot
jay-zhuang authored and facebook-github-bot committed Aug 3, 2020
1 parent 56ed601 commit d941b89
Show file tree
Hide file tree
Showing 2 changed files with 58 additions and 42 deletions.
18 changes: 9 additions & 9 deletions util/timer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
#include <utility>
#include <vector>

#include "port/port.h"
#include "monitoring/instrumented_mutex.h"
#include "rocksdb/env.h"
#include "util/mutexlock.h"

Expand Down Expand Up @@ -53,13 +53,13 @@ class Timer {
env_->NowMicros() + start_after_us,
repeat_every_us));

MutexLock l(&mutex_);
InstrumentedMutexLock l(&mutex_);
heap_.push(fn_info.get());
map_.emplace(std::make_pair(fn_name, std::move(fn_info)));
}

void Cancel(const std::string& fn_name) {
MutexLock l(&mutex_);
InstrumentedMutexLock l(&mutex_);

auto it = map_.find(fn_name);
if (it != map_.end()) {
Expand All @@ -70,13 +70,13 @@ class Timer {
}

void CancelAll() {
MutexLock l(&mutex_);
InstrumentedMutexLock l(&mutex_);
CancelAllWithLock();
}

// Start the Timer
bool Start() {
MutexLock l(&mutex_);
InstrumentedMutexLock l(&mutex_);
if (running_) {
return false;
}
Expand All @@ -89,7 +89,7 @@ class Timer {
// Shutdown the Timer
bool Shutdown() {
{
MutexLock l(&mutex_);
InstrumentedMutexLock l(&mutex_);
if (!running_) {
return false;
}
Expand All @@ -107,7 +107,7 @@ class Timer {
private:

void Run() {
MutexLock l(&mutex_);
InstrumentedMutexLock l(&mutex_);

while (running_) {
if (heap_.empty()) {
Expand Down Expand Up @@ -204,8 +204,8 @@ class Timer {
Env* const env_;
// This mutex controls both the heap_ and the map_. It needs to be held for
// making any changes in them.
port::Mutex mutex_;
port::CondVar cond_var_;
InstrumentedMutex mutex_;
InstrumentedCondVar cond_var_;
std::unique_ptr<port::Thread> thread_;
bool running_;

Expand Down
82 changes: 49 additions & 33 deletions util/timer_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,21 +15,42 @@ class TimerTest : public testing::Test {

protected:
std::unique_ptr<MockTimeEnv> mock_env_;

#if defined(OS_MACOSX) && !defined(NDEBUG)
// On MacOS, `CondVar.TimedWait()` doesn't use the time from MockTimeEnv,
// instead it still uses the system time.
// This is just a mitigation that always trigger the CV timeout. It is not
// perfect, only works for this test.
void SetUp() override {
ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->DisableProcessing();
ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->ClearAllCallBacks();
ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->SetCallBack(
"InstrumentedCondVar::TimedWaitInternal", [&](void* arg) {
uint64_t* time_us = reinterpret_cast<uint64_t*>(arg);
if (*time_us < mock_env_->RealNowMicros()) {
*time_us = mock_env_->RealNowMicros() + 1000;
}
});
ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->EnableProcessing();
}
#endif // OS_MACOSX && !NDEBUG

const uint64_t kSecond = 1000000; // 1sec = 1000000us
};

TEST_F(TimerTest, SingleScheduleOnceTest) {
const uint64_t kSecond = 1000000; // 1sec = 1000000us
const int kIterations = 1;
uint64_t time_counter = 0;
mock_env_->set_current_time(0);
port::Mutex mutex;
port::CondVar test_cv(&mutex);

InstrumentedMutex mutex;
InstrumentedCondVar test_cv(&mutex);

Timer timer(mock_env_.get());
int count = 0;
timer.Add(
[&] {
MutexLock l(&mutex);
InstrumentedMutexLock l(&mutex);
count++;
if (count >= kIterations) {
test_cv.SignalAll();
Expand All @@ -41,7 +62,7 @@ TEST_F(TimerTest, SingleScheduleOnceTest) {

// Wait for execution to finish
{
MutexLock l(&mutex);
InstrumentedMutexLock l(&mutex);
while(count < kIterations) {
time_counter += kSecond;
mock_env_->set_current_time(time_counter);
Expand All @@ -55,31 +76,30 @@ TEST_F(TimerTest, SingleScheduleOnceTest) {
}

TEST_F(TimerTest, MultipleScheduleOnceTest) {
const uint64_t kSecond = 1000000; // 1sec = 1000000us
const int kIterations = 1;
uint64_t time_counter = 0;
mock_env_->set_current_time(0);
port::Mutex mutex1;
port::CondVar test_cv1(&mutex1);
InstrumentedMutex mutex1;
InstrumentedCondVar test_cv1(&mutex1);

Timer timer(mock_env_.get());
int count1 = 0;
timer.Add(
[&] {
MutexLock l(&mutex1);
InstrumentedMutexLock l(&mutex1);
count1++;
if (count1 >= kIterations) {
test_cv1.SignalAll();
}
},
"fn_sch_test1", 1 * kSecond, 0);

port::Mutex mutex2;
port::CondVar test_cv2(&mutex2);
InstrumentedMutex mutex2;
InstrumentedCondVar test_cv2(&mutex2);
int count2 = 0;
timer.Add(
[&] {
MutexLock l(&mutex2);
InstrumentedMutexLock l(&mutex2);
count2 += 5;
if (count2 >= kIterations) {
test_cv2.SignalAll();
Expand All @@ -91,7 +111,7 @@ TEST_F(TimerTest, MultipleScheduleOnceTest) {

// Wait for execution to finish
{
MutexLock l(&mutex1);
InstrumentedMutexLock l(&mutex1);
while (count1 < kIterations) {
time_counter += kSecond;
mock_env_->set_current_time(time_counter);
Expand All @@ -101,7 +121,7 @@ TEST_F(TimerTest, MultipleScheduleOnceTest) {

// Wait for execution to finish
{
MutexLock l(&mutex2);
InstrumentedMutexLock l(&mutex2);
while(count2 < kIterations) {
time_counter += kSecond;
mock_env_->set_current_time(time_counter);
Expand All @@ -115,21 +135,20 @@ TEST_F(TimerTest, MultipleScheduleOnceTest) {
ASSERT_EQ(5, count2);
}

TEST_F(TimerTest, DISABLED_SingleScheduleRepeatedlyTest) {
const uint64_t kSecond = 1000000; // 1sec = 1000000us
TEST_F(TimerTest, SingleScheduleRepeatedlyTest) {
const int kIterations = 5;
uint64_t time_counter = 0;
mock_env_->set_current_time(0);
port::Mutex mutex;
port::CondVar test_cv(&mutex);

InstrumentedMutex mutex;
InstrumentedCondVar test_cv(&mutex);

Timer timer(mock_env_.get());
int count = 0;
timer.Add(
[&] {
MutexLock l(&mutex);
InstrumentedMutexLock l(&mutex);
count++;
fprintf(stderr, "%d\n", count);
if (count >= kIterations) {
test_cv.SignalAll();
}
Expand All @@ -140,7 +159,7 @@ TEST_F(TimerTest, DISABLED_SingleScheduleRepeatedlyTest) {

// Wait for execution to finish
{
MutexLock l(&mutex);
InstrumentedMutexLock l(&mutex);
while(count < kIterations) {
time_counter += kSecond;
mock_env_->set_current_time(time_counter);
Expand All @@ -153,36 +172,33 @@ TEST_F(TimerTest, DISABLED_SingleScheduleRepeatedlyTest) {
ASSERT_EQ(5, count);
}

TEST_F(TimerTest, DISABLED_MultipleScheduleRepeatedlyTest) {
const uint64_t kSecond = 1000000; // 1sec = 1000000us
TEST_F(TimerTest, MultipleScheduleRepeatedlyTest) {
uint64_t time_counter = 0;
mock_env_->set_current_time(0);
Timer timer(mock_env_.get());

port::Mutex mutex1;
port::CondVar test_cv1(&mutex1);
InstrumentedMutex mutex1;
InstrumentedCondVar test_cv1(&mutex1);
const int kIterations1 = 5;
int count1 = 0;
timer.Add(
[&] {
MutexLock l(&mutex1);
InstrumentedMutexLock l(&mutex1);
count1++;
fprintf(stderr, "hello\n");
if (count1 >= kIterations1) {
test_cv1.SignalAll();
}
},
"fn_sch_test1", 0, 2 * kSecond);

port::Mutex mutex2;
port::CondVar test_cv2(&mutex2);
InstrumentedMutex mutex2;
InstrumentedCondVar test_cv2(&mutex2);
const int kIterations2 = 5;
int count2 = 0;
timer.Add(
[&] {
MutexLock l(&mutex2);
InstrumentedMutexLock l(&mutex2);
count2++;
fprintf(stderr, "world\n");
if (count2 >= kIterations2) {
test_cv2.SignalAll();
}
Expand All @@ -193,7 +209,7 @@ TEST_F(TimerTest, DISABLED_MultipleScheduleRepeatedlyTest) {

// Wait for execution to finish
{
MutexLock l(&mutex1);
InstrumentedMutexLock l(&mutex1);
while(count1 < kIterations1) {
time_counter += kSecond;
mock_env_->set_current_time(time_counter);
Expand All @@ -205,7 +221,7 @@ TEST_F(TimerTest, DISABLED_MultipleScheduleRepeatedlyTest) {

// Wait for execution to finish
{
MutexLock l(&mutex2);
InstrumentedMutexLock l(&mutex2);
while(count2 < kIterations2) {
time_counter += kSecond;
mock_env_->set_current_time(time_counter);
Expand Down

0 comments on commit d941b89

Please sign in to comment.