diff options
author | Lais Andrade <lsandrade@google.com> | 2024-04-26 15:32:59 +0000 |
---|---|---|
committer | Lais Andrade <lsandrade@google.com> | 2024-04-29 11:01:21 +0000 |
commit | 1fc5c892343fe3b920df176364edb97c3ad435d6 (patch) | |
tree | 73c4c59b4f131b8c514fda698df6f7767af0afa9 | |
parent | 951e73500c545b4d3ca995440d05f00dd7d6df07 (diff) | |
download | native-1fc5c892343fe3b920df176364edb97c3ad435d6.tar.gz |
Fix flaky VibratorCallbackSchedulerTest
Use TestCounter to wait on scheduled callbacks with a timeout.
The VibratorCallbackScheduler destructor joins on the scheduler
thread to wait for the main loop to finish, but the conditional variable
is waiting indefinitely without a predicate, which can cause it
sometimes to miss the notify call from the destructor and get stuck.
Adding a predicate condition fixes the VibratorCallbackSchedulerTest
flakiness for the timeout "No test results." failures.
Bug: 335951228
Bug: 335577082
Change-Id: If1af34f70de9fa5175aa38ebfb22f5b6d9112272
Merged-In: Id9501c10fe5209003d9b74b0f39f2bcf87de05c2
Merged-In: Ib519c3e91608c7373b2999fc596dab3413226a1e
Test: atest --rerun-until-failure 1000 VibratorCallbackSchedulerTest
-rw-r--r-- | services/vibratorservice/VibratorCallbackScheduler.cpp | 8 | ||||
-rw-r--r-- | services/vibratorservice/test/VibratorCallbackSchedulerTest.cpp | 69 | ||||
-rw-r--r-- | services/vibratorservice/test/test_utils.h | 28 |
3 files changed, 54 insertions, 51 deletions
diff --git a/services/vibratorservice/VibratorCallbackScheduler.cpp b/services/vibratorservice/VibratorCallbackScheduler.cpp index 7eda9ef0c7..b2b1988d97 100644 --- a/services/vibratorservice/VibratorCallbackScheduler.cpp +++ b/services/vibratorservice/VibratorCallbackScheduler.cpp @@ -87,13 +87,13 @@ void CallbackScheduler::loop() { lock.lock(); } if (mQueue.empty()) { - // Wait until a new callback is scheduled. - mCondition.wait(mMutex); + // Wait until a new callback is scheduled or destructor was called. + mCondition.wait(lock, [this] { return mFinished || !mQueue.empty(); }); } else { - // Wait until next callback expires, or a new one is scheduled. + // Wait until next callback expires or a new one is scheduled. // Use the monotonic steady clock to wait for the measured delay interval via wait_for // instead of using a wall clock via wait_until. - mCondition.wait_for(mMutex, mQueue.top().getWaitForExpirationDuration()); + mCondition.wait_for(lock, mQueue.top().getWaitForExpirationDuration()); } } } diff --git a/services/vibratorservice/test/VibratorCallbackSchedulerTest.cpp b/services/vibratorservice/test/VibratorCallbackSchedulerTest.cpp index 426cd426f7..881e32152c 100644 --- a/services/vibratorservice/test/VibratorCallbackSchedulerTest.cpp +++ b/services/vibratorservice/test/VibratorCallbackSchedulerTest.cpp @@ -14,20 +14,13 @@ * limitations under the License. */ -#define LOG_TAG "VibratorHalWrapperAidlTest" - -#include <android-base/thread_annotations.h> -#include <android/hardware/vibrator/IVibrator.h> -#include <condition_variable> - #include <gmock/gmock.h> #include <gtest/gtest.h> -#include <utils/Log.h> -#include <thread> - #include <vibratorservice/VibratorCallbackScheduler.h> +#include "test_utils.h" + using std::chrono::milliseconds; using std::chrono::steady_clock; using std::chrono::time_point; @@ -39,29 +32,25 @@ using namespace testing; // ------------------------------------------------------------------------------------------------- // Delay allowed for the scheduler to process callbacks during this test. -static const auto TEST_TIMEOUT = 50ms; +static const auto TEST_TIMEOUT = 100ms; class VibratorCallbackSchedulerTest : public Test { public: - void SetUp() override { - mScheduler = std::make_unique<vibrator::CallbackScheduler>(); - std::lock_guard<std::mutex> lock(mMutex); - mExpiredCallbacks.clear(); - } + void SetUp() override { mScheduler = std::make_unique<vibrator::CallbackScheduler>(); } protected: std::mutex mMutex; - std::condition_variable_any mCondition; std::unique_ptr<vibrator::CallbackScheduler> mScheduler = nullptr; + vibrator::TestCounter mCallbackCounter; std::vector<int32_t> mExpiredCallbacks GUARDED_BY(mMutex); std::function<void()> createCallback(int32_t id) { - return [=]() { + return [this, id]() { { std::lock_guard<std::mutex> lock(mMutex); mExpiredCallbacks.push_back(id); } - mCondition.notify_all(); + mCallbackCounter.increment(); }; } @@ -71,56 +60,42 @@ protected: } int32_t waitForCallbacks(int32_t callbackCount, milliseconds timeout) { - time_point<steady_clock> expirationTime = steady_clock::now() + timeout + TEST_TIMEOUT; - int32_t expiredCallbackCount = 0; - while (steady_clock::now() < expirationTime) { - std::lock_guard<std::mutex> lock(mMutex); - expiredCallbackCount = mExpiredCallbacks.size(); - if (callbackCount <= expiredCallbackCount) { - return expiredCallbackCount; - } - auto currentTimeout = std::chrono::duration_cast<std::chrono::milliseconds>( - expirationTime - steady_clock::now()); - if (currentTimeout > currentTimeout.zero()) { - // Use the monotonic steady clock to wait for the requested timeout via wait_for - // instead of using a wall clock via wait_until. - mCondition.wait_for(mMutex, currentTimeout); - } - } - return expiredCallbackCount; + mCallbackCounter.tryWaitUntilCountIsAtLeast(callbackCount, timeout); + return mCallbackCounter.get(); } }; // ------------------------------------------------------------------------------------------------- TEST_F(VibratorCallbackSchedulerTest, TestScheduleRunsOnlyAfterDelay) { + auto callbackDuration = 50ms; time_point<steady_clock> startTime = steady_clock::now(); - mScheduler->schedule(createCallback(1), 50ms); + mScheduler->schedule(createCallback(1), callbackDuration); - ASSERT_EQ(1, waitForCallbacks(1, 50ms)); + ASSERT_THAT(waitForCallbacks(1, callbackDuration + TEST_TIMEOUT), Eq(1)); time_point<steady_clock> callbackTime = steady_clock::now(); - // Callback happened at least 50ms after the beginning of the test. - ASSERT_TRUE(startTime + 50ms <= callbackTime); - ASSERT_THAT(getExpiredCallbacks(), ElementsAre(1)); + // Callback took at least the required duration to trigger. + ASSERT_THAT(callbackTime, Ge(startTime + callbackDuration)); } TEST_F(VibratorCallbackSchedulerTest, TestScheduleMultipleCallbacksRunsInDelayOrder) { // Schedule first callbacks long enough that all 3 will be scheduled together and run in order. - mScheduler->schedule(createCallback(1), 50ms); - mScheduler->schedule(createCallback(2), 40ms); - mScheduler->schedule(createCallback(3), 10ms); + mScheduler->schedule(createCallback(1), 50ms + 2 * TEST_TIMEOUT); + mScheduler->schedule(createCallback(2), 50ms + TEST_TIMEOUT); + mScheduler->schedule(createCallback(3), 50ms); - ASSERT_EQ(3, waitForCallbacks(3, 50ms)); + // Callbacks triggered in the expected order based on the requested durations. + ASSERT_THAT(waitForCallbacks(3, 50ms + 3 * TEST_TIMEOUT), Eq(3)); ASSERT_THAT(getExpiredCallbacks(), ElementsAre(3, 2, 1)); } TEST_F(VibratorCallbackSchedulerTest, TestDestructorDropsPendingCallbacksAndKillsThread) { // Schedule callback long enough that scheduler will be destroyed while it's still scheduled. - mScheduler->schedule(createCallback(1), 50ms); + mScheduler->schedule(createCallback(1), 100ms); mScheduler.reset(nullptr); // Should timeout waiting for callback to run. - ASSERT_EQ(0, waitForCallbacks(1, 50ms)); - ASSERT_TRUE(getExpiredCallbacks().empty()); + ASSERT_THAT(waitForCallbacks(1, 100ms + TEST_TIMEOUT), Eq(0)); + ASSERT_THAT(getExpiredCallbacks(), IsEmpty()); } diff --git a/services/vibratorservice/test/test_utils.h b/services/vibratorservice/test/test_utils.h index 1933a118ef..c08cfc6bfa 100644 --- a/services/vibratorservice/test/test_utils.h +++ b/services/vibratorservice/test/test_utils.h @@ -85,6 +85,34 @@ private: ~TestFactory() = delete; }; +class TestCounter { +public: + TestCounter(int32_t init = 0) : mMutex(), mCondVar(), mCount(init) {} + + int32_t get() { + std::lock_guard<std::mutex> lock(mMutex); + return mCount; + } + + void increment() { + { + std::lock_guard<std::mutex> lock(mMutex); + mCount += 1; + } + mCondVar.notify_all(); + } + + void tryWaitUntilCountIsAtLeast(int32_t count, std::chrono::milliseconds timeout) { + std::unique_lock<std::mutex> lock(mMutex); + mCondVar.wait_for(lock, timeout, [&] { return mCount >= count; }); + } + +private: + std::mutex mMutex; + std::condition_variable mCondVar; + int32_t mCount GUARDED_BY(mMutex); +}; + // ------------------------------------------------------------------------------------------------- } // namespace vibrator |