Merge "camera: Fix client eviction/disconnect race."

3 files changed, 75 insertions, 4 deletions

diff --git a/services/camera/libcameraservice/CameraService.cpp b/services/camera/libcameraservice/CameraService.cpp
index dabfafa..414d563 100644
--- a/services/camera/libcameraservice/CameraService.cpp
+++ b/services/camera/libcameraservice/CameraService.cpp
@@ -866,7 +866,7 @@ status_t CameraService::handleEvictionsLocked(const String8& cameraId, int clien
         std::shared_ptr<resource_policy::ClientDescriptor<String8, sp<BasicClient>>>* partial) {
 
     status_t ret = NO_ERROR;
-    std::vector<sp<BasicClient>> evictedClients;
+    std::vector<DescriptorPtr> evictedClients;
     DescriptorPtr clientDescriptor;
     {
         if (effectiveApiLevel == API_1) {
@@ -974,7 +974,7 @@ status_t CameraService::handleEvictionsLocked(const String8& cameraId, int clien
 
             ALOGE("CameraService::connect evicting conflicting client for camera ID %s",
                     i->getKey().string());
-            evictedClients.push_back(clientSp);
+            evictedClients.push_back(i);
 
             // Log the clients evicted
             logEvent(String8::format("EVICT device %s client held by package %s (PID"
@@ -1001,12 +1001,31 @@ status_t CameraService::handleEvictionsLocked(const String8& cameraId, int clien
     // Destroy evicted clients
     for (auto& i : evictedClients) {
         // Disconnect is blocking, and should only have returned when HAL has cleaned up
-        i->disconnect(); // Clients will remove themselves from the active client list here
+        i->getValue()->disconnect(); // Clients will remove themselves from the active client list
     }
-    evictedClients.clear();
 
     IPCThreadState::self()->restoreCallingIdentity(token);
 
+    for (const auto& i : evictedClients) {
+        ALOGV("%s: Waiting for disconnect to complete for client for device %s (PID %" PRId32 ")",
+                __FUNCTION__, i->getKey().string(), i->getOwnerId());
+        ret = mActiveClientManager.waitUntilRemoved(i, DEFAULT_DISCONNECT_TIMEOUT_NS);
+        if (ret == TIMED_OUT) {
+            ALOGE("%s: Timed out waiting for client for device %s to disconnect, "
+                    "current clients:\n%s", __FUNCTION__, i->getKey().string(),
+                    mActiveClientManager.toString().string());
+            return -EBUSY;
+        }
+        if (ret != NO_ERROR) {
+            ALOGE("%s: Received error waiting for client for device %s to disconnect: %s (%d), "
+                    "current clients:\n%s", __FUNCTION__, i->getKey().string(), strerror(-ret),
+                    ret, mActiveClientManager.toString().string());
+            return ret;
+        }
+    }
+
+    evictedClients.clear();
+
     // Once clients have been disconnected, relock
     mServiceLock.lock();
 
diff --git a/services/camera/libcameraservice/CameraService.h b/services/camera/libcameraservice/CameraService.h
index 9eda205..91c7d59 100644
--- a/services/camera/libcameraservice/CameraService.h
+++ b/services/camera/libcameraservice/CameraService.h
@@ -90,6 +90,9 @@ public:
     // 3 second busy timeout when other clients are connecting
     static const nsecs_t DEFAULT_CONNECT_TIMEOUT_NS = 3000000000;
 
+    // 1 second busy timeout when other clients are disconnecting
+    static const nsecs_t DEFAULT_DISCONNECT_TIMEOUT_NS = 1000000000;
+
     // Default number of messages to store in eviction log
     static const size_t DEFAULT_EVENT_LOG_LENGTH = 100;
 
diff --git a/services/camera/libcameraservice/utils/ClientManager.h b/services/camera/libcameraservice/utils/ClientManager.h
index ad5486d..aa40a2d 100644
--- a/services/camera/libcameraservice/utils/ClientManager.h
+++ b/services/camera/libcameraservice/utils/ClientManager.h
@@ -17,7 +17,9 @@
 #ifndef ANDROID_SERVICE_UTILS_EVICTION_POLICY_MANAGER_H
 #define ANDROID_SERVICE_UTILS_EVICTION_POLICY_MANAGER_H
 
+#include <utils/Condition.h>
 #include <utils/Mutex.h>
+#include <utils/Timers.h>
 
 #include <algorithm>
 #include <utility>
@@ -263,6 +265,16 @@ public:
      */
     std::shared_ptr<ClientDescriptor<KEY, VALUE>> get(const KEY& key) const;
 
+    /**
+     * Block until the given client is no longer in the active clients list, or the timeout
+     * occurred.
+     *
+     * Returns NO_ERROR if this succeeded, -ETIMEDOUT on a timeout, or a negative error code on
+     * failure.
+     */
+    status_t waitUntilRemoved(const std::shared_ptr<ClientDescriptor<KEY, VALUE>> client,
+            nsecs_t timeout) const;
+
 protected:
     ~ClientManager();
 
@@ -284,6 +296,7 @@ private:
     int64_t getCurrentCostLocked() const;
 
     mutable Mutex mLock;
+    mutable Condition mRemovedCondition;
     int32_t mMaxCost;
     // LRU ordered, most recent at end
     std::vector<std::shared_ptr<ClientDescriptor<KEY, VALUE>>> mClients;
@@ -430,6 +443,7 @@ std::vector<std::shared_ptr<ClientDescriptor<KEY, VALUE>>> ClientManager<KEY, VA
         }), mClients.end());
 
     mClients.push_back(client);
+    mRemovedCondition.broadcast();
 
     return evicted;
 }
@@ -487,6 +501,7 @@ template<class KEY, class VALUE>
 void ClientManager<KEY, VALUE>::removeAll() {
     Mutex::Autolock lock(mLock);
     mClients.clear();
+    mRemovedCondition.broadcast();
 }
 
 template<class KEY, class VALUE>
@@ -505,6 +520,39 @@ std::shared_ptr<ClientDescriptor<KEY, VALUE>> ClientManager<KEY, VALUE>::remove(
             return false;
         }), mClients.end());
 
+    mRemovedCondition.broadcast();
+    return ret;
+}
+
+template<class KEY, class VALUE>
+status_t ClientManager<KEY, VALUE>::waitUntilRemoved(
+        const std::shared_ptr<ClientDescriptor<KEY, VALUE>> client,
+        nsecs_t timeout) const {
+    status_t ret = NO_ERROR;
+    Mutex::Autolock lock(mLock);
+
+    bool isRemoved = false;
+
+    // Figure out what time in the future we should hit the timeout
+    nsecs_t failTime = systemTime(SYSTEM_TIME_MONOTONIC) + timeout;
+
+    while (!isRemoved) {
+        isRemoved = true;
+        for (const auto& i : mClients) {
+            if (i == client) {
+                isRemoved = false;
+            }
+        }
+
+        if (!isRemoved) {
+            ret = mRemovedCondition.waitRelative(mLock, timeout);
+            if (ret != NO_ERROR) {
+                break;
+            }
+            timeout = failTime - systemTime(SYSTEM_TIME_MONOTONIC);
+        }
+    }
+
     return ret;
 }
 
@@ -520,6 +568,7 @@ void ClientManager<KEY, VALUE>::remove(
             }
             return false;
         }), mClients.end());
+    mRemovedCondition.broadcast();
 }
 
 template<class KEY, class VALUE>