From 4f9576bf48c5909782c12490e8a9faa974ae68d6 Mon Sep 17 00:00:00 2001 From: Ruben Brunk Date: Fri, 10 Apr 2015 17:26:56 -0700 Subject: camera: Fix client eviction/disconnect race. - Add blocking wait in camera service connect call to prevent race when client has called disconnect while eviction of that client is taking place, resulting in early call of device initialization before all HAL resources are available. Bug: 20038135 Change-Id: I7afc5bfa23612ba7f83293fa542ff983a5991230 --- services/camera/libcameraservice/CameraService.cpp | 27 ++++++++++-- services/camera/libcameraservice/CameraService.h | 3 ++ .../camera/libcameraservice/utils/ClientManager.h | 49 ++++++++++++++++++++++ 3 files changed, 75 insertions(+), 4 deletions(-) (limited to 'services') 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>>* partial) { status_t ret = NO_ERROR; - std::vector> evictedClients; + std::vector 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 #include +#include #include #include @@ -263,6 +265,16 @@ public: */ std::shared_ptr> 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> 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>> mClients; @@ -430,6 +443,7 @@ std::vector>> ClientManager void ClientManager::removeAll() { Mutex::Autolock lock(mLock); mClients.clear(); + mRemovedCondition.broadcast(); } template @@ -505,6 +520,39 @@ std::shared_ptr> ClientManager::remove( return false; }), mClients.end()); + mRemovedCondition.broadcast(); + return ret; +} + +template +status_t ClientManager::waitUntilRemoved( + const std::shared_ptr> 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::remove( } return false; }), mClients.end()); + mRemovedCondition.broadcast(); } template -- cgit v1.1