From 417ca2541aad2f4a639408da2f9d1e01acf42e08 Mon Sep 17 00:00:00 2001
From: Yu Shan <shanyu@google.com>
Date: Thu, 8 Dec 2022 11:39:26 -0800
Subject: Avoid holding lock while calling recurrent actions.

This CL fixes a dead lock issue caused by RecurrentTimer holding
internal locks while calling actions. The dead lock is caused by
the following situation:
1. Caller holds a lock, call RecurrentTimer.registerCallback which
waits for RecurrentTimer's lock.
2. Another recurrent action happens at the same time. Recurrent
timer holds lock before calling the client action. The client action
is now waiting for the lock that is currently hold by 1.

Test: atest RecurrentTimerTest
Bug: 255574557
Change-Id: I3999f4e9cdf581cb851d5f49341dbcc0c160f234
(cherry picked from commit 93a821077effec224d3880875d98c13ef1787e4c)
---
 .../aidl/impl/utils/common/src/RecurrentTimer.cpp  | 67 +++++++++++-----------
 1 file changed, 35 insertions(+), 32 deletions(-)

(limited to 'automotive/vehicle/aidl/impl/utils/common/src/RecurrentTimer.cpp')

diff --git a/automotive/vehicle/aidl/impl/utils/common/src/RecurrentTimer.cpp b/automotive/vehicle/aidl/impl/utils/common/src/RecurrentTimer.cpp
index 2eca6b7a17..908564c2ff 100644
--- a/automotive/vehicle/aidl/impl/utils/common/src/RecurrentTimer.cpp
+++ b/automotive/vehicle/aidl/impl/utils/common/src/RecurrentTimer.cpp
@@ -101,68 +101,71 @@ void RecurrentTimer::removeInvalidCallbackLocked() {
     }
 }
 
-std::unique_ptr<RecurrentTimer::CallbackInfo> RecurrentTimer::popNextCallbackLocked() {
+std::shared_ptr<RecurrentTimer::Callback> RecurrentTimer::getNextCallbackLocked(int64_t now) {
     std::pop_heap(mCallbackQueue.begin(), mCallbackQueue.end(), CallbackInfo::cmp);
-    std::unique_ptr<CallbackInfo> info = std::move(mCallbackQueue[mCallbackQueue.size() - 1]);
-    mCallbackQueue.pop_back();
+    auto& callbackInfo = mCallbackQueue[mCallbackQueue.size() - 1];
+    auto nextCallback = callbackInfo->callback;
+    // intervalCount is the number of interval we have to advance until we pass now.
+    size_t intervalCount = (now - callbackInfo->nextTime) / callbackInfo->interval + 1;
+    callbackInfo->nextTime += intervalCount * callbackInfo->interval;
+    std::push_heap(mCallbackQueue.begin(), mCallbackQueue.end(), CallbackInfo::cmp);
+
     // Make sure the first element is always valid.
     removeInvalidCallbackLocked();
-    return info;
+
+    return nextCallback;
 }
 
 void RecurrentTimer::loop() {
-    std::unique_lock<std::mutex> uniqueLock(mLock);
-
+    std::vector<std::shared_ptr<Callback>> callbacksToRun;
     while (true) {
-        // Wait until the timer exits or we have at least one recurrent callback.
-        mCond.wait(uniqueLock, [this] {
-            ScopedLockAssertion lockAssertion(mLock);
-            return mStopRequested || mCallbackQueue.size() != 0;
-        });
-
-        int64_t interval;
         {
+            std::unique_lock<std::mutex> uniqueLock(mLock);
             ScopedLockAssertion lockAssertion(mLock);
+            // Wait until the timer exits or we have at least one recurrent callback.
+            mCond.wait(uniqueLock, [this] {
+                ScopedLockAssertion lockAssertion(mLock);
+                return mStopRequested || mCallbackQueue.size() != 0;
+            });
+
+            int64_t interval;
             if (mStopRequested) {
                 return;
             }
             // The first element is the nearest next event.
             int64_t nextTime = mCallbackQueue[0]->nextTime;
             int64_t now = uptimeNanos();
+
             if (nextTime > now) {
                 interval = nextTime - now;
             } else {
                 interval = 0;
             }
-        }
 
-        // Wait for the next event or the timer exits.
-        if (mCond.wait_for(uniqueLock, std::chrono::nanoseconds(interval), [this] {
-                ScopedLockAssertion lockAssertion(mLock);
-                return mStopRequested;
-            })) {
-            return;
-        }
+            // Wait for the next event or the timer exits.
+            if (mCond.wait_for(uniqueLock, std::chrono::nanoseconds(interval), [this] {
+                    ScopedLockAssertion lockAssertion(mLock);
+                    return mStopRequested;
+                })) {
+                return;
+            }
 
-        {
-            ScopedLockAssertion lockAssertion(mLock);
-            int64_t now = uptimeNanos();
+            now = uptimeNanos();
+            callbacksToRun.clear();
             while (mCallbackQueue.size() > 0) {
                 int64_t nextTime = mCallbackQueue[0]->nextTime;
                 if (nextTime > now) {
                     break;
                 }
 
-                std::unique_ptr<CallbackInfo> info = popNextCallbackLocked();
-                info->nextTime += info->interval;
-
-                auto callback = info->callback;
-                mCallbackQueue.push_back(std::move(info));
-                std::push_heap(mCallbackQueue.begin(), mCallbackQueue.end(), CallbackInfo::cmp);
-
-                (*callback)();
+                callbacksToRun.push_back(getNextCallbackLocked(now));
             }
         }
+
+        // Do not execute the callback while holding the lock.
+        for (size_t i = 0; i < callbacksToRun.size(); i++) {
+            (*callbacksToRun[i])();
+        }
     }
 }
 
-- 
cgit v1.2.3