Fix hidl_ssvc_poll thread issues

1) Patch poll wake up handling logic so that thread no longer quits
   on spurious wake up.
2) Patch(simplify) thread initialization, so that it will not hang
   on waiting for conditional variable.

Test: 1) send signal 3 to system_server, does not observe thread quit.
Test: 2) adb shell "stop;start" many times, every time thread starts.

Bug: 62933449
Bug: 62933799

Change-Id: Icb7477e9d75338b9742e144fc2687d5ced91e89e

diff --git a/services/sensorservice/hidl/SensorManager.cpp b/services/sensorservice/hidl/SensorManager.cpp
index f1f52d8..fee6da1 100644 (file)
--- a/services/sensorservice/hidl/SensorManager.cpp
+++ b/services/sensorservice/hidl/SensorManager.cpp
@@ -24,7 +24,6 @@
 
 #include <sched.h>
 
-#include <thread>
 
 #include "EventQueue.h"
 #include "DirectReportChannel.h"
@@ -43,20 +42,24 @@ using ::android::hardware::sensors::V1_0::SensorInfo;
 using ::android::hardware::sensors::V1_0::SensorsEventFormatOffset;
 using ::android::hardware::hidl_vec;
 using ::android::hardware::Void;
-using ::android::sp;
 
 static const char* POLL_THREAD_NAME = "hidl_ssvc_poll";
 
 SensorManager::SensorManager(JavaVM* vm)
-        : mJavaVm(vm) {
+        : mLooper(new Looper(false /*allowNonCallbacks*/)), mStopThread(true), mJavaVm(vm) {
 }
 
 SensorManager::~SensorManager() {
     // Stops pollAll inside the thread.
-    std::unique_lock<std::mutex> lock(mLooperMutex);
+    std::lock_guard<std::mutex> lock(mThreadMutex);
+
+    mStopThread = true;
     if (mLooper != nullptr) {
         mLooper->wake();
     }
+    if (mPollThread.joinable()) {
+        mPollThread.join();
+    }
 }
 
 // Methods from ::android::frameworks::sensorservice::V1_0::ISensorManager follow.
@@ -131,12 +134,13 @@ Return<void> SensorManager::createGrallocDirectChannel(
 }
 
 /* One global looper for all event queues created from this SensorManager. */
-sp<::android::Looper> SensorManager::getLooper() {
-    std::unique_lock<std::mutex> lock(mLooperMutex);
-    if (mLooper == nullptr) {
-        std::condition_variable looperSet;
+sp<Looper> SensorManager::getLooper() {
+    std::lock_guard<std::mutex> lock(mThreadMutex);
 
-        std::thread{[&mutex = mLooperMutex, &looper = mLooper, &looperSet, javaVm = mJavaVm] {
+    if (!mPollThread.joinable()) {
+        // if thread not initialized, start thread
+        mStopThread = false;
+        std::thread pollThread{[&stopThread = mStopThread, looper = mLooper, javaVm = mJavaVm] {
 
             struct sched_param p = {0};
             p.sched_priority = 10;
@@ -145,16 +149,11 @@ sp<::android::Looper> SensorManager::getLooper() {
                         << strerror(errno);
             }
 
-            std::unique_lock<std::mutex> lock(mutex);
-            if (looper != nullptr) {
-                LOG(INFO) << "Another thread has already set the looper, exiting this one.";
-                return;
-            }
-            looper = Looper::prepare(ALOOPER_PREPARE_ALLOW_NON_CALLBACKS /* opts */);
-            lock.unlock();
+            // set looper
+            Looper::setForThread(looper);
 
-            // Attach the thread to JavaVM so that pollAll do not crash if the event
-            // is from Java.
+            // Attach the thread to JavaVM so that pollAll do not crash if the thread
+            // eventually calls into Java.
             JavaVMAttachArgs args{
                 .version = JNI_VERSION_1_2,
                 .name = POLL_THREAD_NAME,
@@ -165,19 +164,30 @@ sp<::android::Looper> SensorManager::getLooper() {
                 LOG(FATAL) << "Cannot attach SensorManager looper thread to Java VM.";
             }
 
-            looperSet.notify_one();
-            int pollResult = looper->pollAll(-1 /* timeout */);
-            if (pollResult != ALOOPER_POLL_WAKE) {
-                LOG(ERROR) << "Looper::pollAll returns unexpected " << pollResult;
+            LOG(INFO) << POLL_THREAD_NAME << " started.";
+            for (;;) {
+                int pollResult = looper->pollAll(-1 /* timeout */);
+                if (pollResult == Looper::POLL_WAKE) {
+                    if (stopThread == true) {
+                        LOG(INFO) << POLL_THREAD_NAME << ": requested to stop";
+                        break;
+                    } else {
+                        LOG(INFO) << POLL_THREAD_NAME << ": spurious wake up, back to work";
+                    }
+                } else {
+                    LOG(ERROR) << POLL_THREAD_NAME << ": Looper::pollAll returns unexpected "
+                               << pollResult;
+                    break;
+                }
             }
 
             if (javaVm->DetachCurrentThread() != JNI_OK) {
                 LOG(ERROR) << "Cannot detach SensorManager looper thread from Java VM.";
             }
 
-            LOG(INFO) << "Looper thread is terminated.";
-        }}.detach();
-        looperSet.wait(lock, [this]{ return this->mLooper != nullptr; });
+            LOG(INFO) << POLL_THREAD_NAME << " is terminated.";
+        }};
+        mPollThread = std::move(pollThread);
     }
     return mLooper;
 }
@@ -199,6 +209,12 @@ Return<void> SensorManager::createEventQueue(
     }
 
     sp<::android::Looper> looper = getLooper();
+    if (looper == nullptr) {
+        LOG(ERROR) << "::android::SensorManager::createEventQueue cannot initialize looper";
+        _hidl_cb(nullptr, Result::UNKNOWN_ERROR);
+        return Void();
+    }
+
     String8 package(String8::format("hidl_client_pid_%d",
                                     android::hardware::IPCThreadState::self()->getCallingPid()));
     sp<::android::SensorEventQueue> internalQueue = getInternalManager().createEventQueue(package);

diff --git a/services/sensorservice/hidl/include/sensorservicehidl/SensorManager.h b/services/sensorservice/hidl/include/sensorservicehidl/SensorManager.h
index e66c8e5..ddcee28 100644 (file)
--- a/services/sensorservice/hidl/include/sensorservicehidl/SensorManager.h
+++ b/services/sensorservice/hidl/include/sensorservicehidl/SensorManager.h
@@ -20,6 +20,7 @@
 #include <jni.h>
 
 #include <mutex>
+#include <thread>
 
 #include <android/frameworks/sensorservice/1.0/ISensorManager.h>
 #include <android/frameworks/sensorservice/1.0/types.h>
@@ -38,6 +39,7 @@ using ::android::hardware::sensors::V1_0::SensorType;
 using ::android::hardware::hidl_handle;
 using ::android::hardware::hidl_memory;
 using ::android::hardware::Return;
+using ::android::sp;
 
 struct SensorManager final : public ISensorManager {
 
@@ -54,13 +56,15 @@ struct SensorManager final : public ISensorManager {
 private:
     // Block until ::android::SensorManager is initialized.
     ::android::SensorManager& getInternalManager();
-    sp<::android::Looper> getLooper();
+    sp<Looper> getLooper();
 
     std::mutex mInternalManagerMutex;
     ::android::SensorManager* mInternalManager = nullptr; // does not own
+    sp<Looper> mLooper;
 
-    std::mutex mLooperMutex;
-    sp<::android::Looper> mLooper;
+    volatile bool mStopThread;
+    std::mutex mThreadMutex; //protects mPollThread
+    std::thread mPollThread;
 
     JavaVM* mJavaVm;
 };