Merge "Revert "lshal: use std::async"" into main am: cb59ad9 am: 4f1c485

Original change: https://android-review.googlesource.com/c/platform/frameworks/native/+/2942572

Change-Id: I55392b139923b79609b3398355a40bc03ee11af1
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

Author: Treehugger Robot

cmds/lshal/Timeout.h

@@ -16,44 +16,83 @@
 
 #pragma once
 
+#include <condition_variable>
 #include <chrono>
-#include <future>
+#include <functional>
+#include <mutex>
+#include <thread>
 
 #include <hidl/Status.h>
-#include <utils/Errors.h>
 
 namespace android {
 namespace lshal {
 
-// Call function on interfaceObject and wait for result until the given timeout has reached.
-// Callback functions pass to timeoutIPC() may be executed after the this function
-// has returned, especially if deadline has been reached. Hence, care must be taken when passing
-// data between the background thread and the main thread. See b/311143089.
+class BackgroundTaskState {
+public:
+    explicit BackgroundTaskState(std::function<void(void)> &&func)
+            : mFunc(std::forward<decltype(func)>(func)) {}
+    void notify() {
+        std::unique_lock<std::mutex> lock(mMutex);
+        mFinished = true;
+        lock.unlock();
+        mCondVar.notify_all();
+    }
+    template<class C, class D>
+    bool wait(std::chrono::time_point<C, D> end) {
+        std::unique_lock<std::mutex> lock(mMutex);
+        mCondVar.wait_until(lock, end, [this](){ return this->mFinished; });
+        return mFinished;
+    }
+    void operator()() {
+        mFunc();
+    }
+private:
+    std::mutex mMutex;
+    std::condition_variable mCondVar;
+    bool mFinished = false;
+    std::function<void(void)> mFunc;
+};
+
+void *callAndNotify(void *data) {
+    BackgroundTaskState &state = *static_cast<BackgroundTaskState *>(data);
+    state();
+    state.notify();
+    return nullptr;
+}
+
+template<class R, class P>
+bool timeout(std::chrono::duration<R, P> delay, std::function<void(void)> &&func) {
+    auto now = std::chrono::system_clock::now();
+    BackgroundTaskState state{std::forward<decltype(func)>(func)};
+    pthread_t thread;
+    if (pthread_create(&thread, nullptr, callAndNotify, &state)) {
+        std::cerr << "FATAL: could not create background thread." << std::endl;
+        return false;
+    }
+    bool success = state.wait(now + delay);
+    if (!success) {
+        pthread_kill(thread, SIGINT);
+    }
+    pthread_join(thread, nullptr);
+    return success;
+}
+
 template<class R, class P, class Function, class I, class... Args>
 typename std::invoke_result<Function, I *, Args...>::type
 timeoutIPC(std::chrono::duration<R, P> wait, const sp<I> &interfaceObject, Function &&func,
            Args &&... args) {
     using ::android::hardware::Status;
-
-    // Execute on a background thread but do not defer execution.
-    auto future =
-            std::async(std::launch::async, func, interfaceObject, std::forward<Args>(args)...);
-    auto status = future.wait_for(wait);
-    if (status == std::future_status::ready) {
-        return future.get();
-    }
-
-    // This future belongs to a background thread that we no longer care about.
-    // Putting this in the global list avoids std::future::~future() that may wait for the
-    // result to come back.
-    // This leaks memory, but lshal is a debugging tool, so this is fine.
-    static std::vector<decltype(future)> gDeadPool{};
-    gDeadPool.emplace_back(std::move(future));
-
-    if (status == std::future_status::timeout) {
+    typename std::result_of<Function(I *, Args...)>::type ret{Status::ok()};
+    auto boundFunc = std::bind(std::forward<Function>(func),
+            interfaceObject.get(), std::forward<Args>(args)...);
+    bool success = timeout(wait, [&ret, &boundFunc] {
+        ret = std::move(boundFunc());
+    });
+    if (!success) {
         return Status::fromStatusT(TIMED_OUT);
     }
-    return Status::fromExceptionCode(Status::Exception::EX_ILLEGAL_STATE, "Illegal future_status");
+    return ret;
 }
-} // namespace lshal
-} // namespace android
+
+}  // namespace lshal
+}  // namespace android

cmds/lshal/main.cpp

@@ -18,6 +18,5 @@
 
 int main(int argc, char **argv) {
     using namespace ::android::lshal;
-    // Use _exit() to force terminate background threads in Timeout.h
-    _exit(Lshal{}.main(Arg{argc, argv}));
+    return Lshal{}.main(Arg{argc, argv});
 }

cmds/lshal/test.cpp

@@ -14,10 +14,6 @@
  * limitations under the License.
  */
 
-#include <chrono>
-#include <future>
-#include <mutex>
-#include "android/hidl/base/1.0/IBase.h"
 #define LOG_TAG "Lshal"
 #include <android-base/logging.h>
 
@@ -40,8 +36,6 @@
 
 using namespace testing;
 
-using std::chrono_literals::operator""ms;
-
 using ::android::hidl::base::V1_0::DebugInfo;
 using ::android::hidl::base::V1_0::IBase;
 using ::android::hidl::manager::V1_0::IServiceManager;
@@ -940,9 +934,12 @@ TEST_F(ListTest, DumpDebug) {
         return hardware::Void();
     }));
     EXPECT_CALL(*serviceManager, get(_, _))
-            .WillRepeatedly(Invoke([&](const hidl_string&, const hidl_string&) -> sp<IBase> {
-                return sp<IBase>(service);
-            }));
+            .WillRepeatedly(
+                    Invoke([&](const hidl_string&, const hidl_string& instance) -> sp<IBase> {
+                        int id = getIdFromInstanceName(instance);
+                        if (id > inheritanceLevel) return nullptr;
+                        return sp<IBase>(service);
+                    }));
 
     const std::string expected = "[fake description 0]\n"
                                  "Interface\n"
@@ -960,110 +957,6 @@ TEST_F(ListTest, DumpDebug) {
     EXPECT_EQ("", err.str());
 }
 
-// In SlowService, everything goes slooooooow. Each IPC call will wait for
-// the specified time before calling the callback function or returning.
-class SlowService : public IBase {
-public:
-    explicit SlowService(std::chrono::milliseconds wait) : mWait(wait) {}
-    android::hardware::Return<void> interfaceDescriptor(interfaceDescriptor_cb cb) override {
-        std::this_thread::sleep_for(mWait);
-        cb(getInterfaceName(1));
-        storeHistory("interfaceDescriptor");
-        return hardware::Void();
-    }
-    android::hardware::Return<void> interfaceChain(interfaceChain_cb cb) override {
-        std::this_thread::sleep_for(mWait);
-        std::vector<hidl_string> ret;
-        ret.push_back(getInterfaceName(1));
-        ret.push_back(IBase::descriptor);
-        cb(ret);
-        storeHistory("interfaceChain");
-        return hardware::Void();
-    }
-    android::hardware::Return<void> getHashChain(getHashChain_cb cb) override {
-        std::this_thread::sleep_for(mWait);
-        std::vector<hidl_hash> ret;
-        ret.push_back(getHashFromId(0));
-        ret.push_back(getHashFromId(0xff));
-        cb(ret);
-        storeHistory("getHashChain");
-        return hardware::Void();
-    }
-    android::hardware::Return<void> debug(const hidl_handle&,
-                                          const hidl_vec<hidl_string>&) override {
-        std::this_thread::sleep_for(mWait);
-        storeHistory("debug");
-        return Void();
-    }
-
-    template <class R, class P, class Pred>
-    bool waitForHistory(std::chrono::duration<R, P> wait, Pred predicate) {
-        std::unique_lock<std::mutex> lock(mLock);
-        return mCv.wait_for(lock, wait, [&]() { return predicate(mCallHistory); });
-    }
-
-private:
-    void storeHistory(std::string hist) {
-        {
-            std::lock_guard<std::mutex> lock(mLock);
-            mCallHistory.emplace_back(std::move(hist));
-        }
-        mCv.notify_all();
-    }
-
-    const std::chrono::milliseconds mWait;
-    std::mutex mLock;
-    std::condition_variable mCv;
-    // List of functions that have finished being called on this interface.
-    std::vector<std::string> mCallHistory;
-};
-
-class TimeoutTest : public ListTest {
-public:
-    void setMockServiceManager(sp<IBase> service) {
-        EXPECT_CALL(*serviceManager, list(_))
-                .WillRepeatedly(Invoke([&](IServiceManager::list_cb cb) {
-                    std::vector<hidl_string> ret;
-                    ret.push_back(getInterfaceName(1) + "/default");
-                    cb(ret);
-                    return hardware::Void();
-                }));
-        EXPECT_CALL(*serviceManager, get(_, _))
-                .WillRepeatedly(Invoke([&](const hidl_string&, const hidl_string&) -> sp<IBase> {
-                    return service;
-                }));
-    }
-};
-
-TEST_F(TimeoutTest, BackgroundThreadIsKept) {
-    auto lshalIpcTimeout = 100ms;
-    auto serviceIpcTimeout = 200ms;
-    lshal->setWaitTimeForTest(lshalIpcTimeout, lshalIpcTimeout);
-    sp<SlowService> service = new SlowService(serviceIpcTimeout);
-    setMockServiceManager(service);
-
-    optind = 1; // mimic Lshal::parseArg()
-    EXPECT_NE(0u, mockList->main(createArg({"lshal", "--types=b", "-i", "--neat"})));
-    EXPECT_THAT(err.str(), HasSubstr("Skipping \"a.h.foo1@1.0::IFoo/default\""));
-    EXPECT_TRUE(service->waitForHistory(serviceIpcTimeout * 5, [](const auto& hist) {
-        return hist.size() == 1 && hist[0] == "interfaceChain";
-    })) << "The background thread should continue after the main thread moves on, but it is killed";
-}
-
-TEST_F(TimeoutTest, BackgroundThreadDoesNotBlockMainThread) {
-    auto lshalIpcTimeout = 100ms;
-    auto serviceIpcTimeout = 2000ms;
-    auto start = std::chrono::system_clock::now();
-    lshal->setWaitTimeForTest(lshalIpcTimeout, lshalIpcTimeout);
-    sp<SlowService> service = new SlowService(serviceIpcTimeout);
-    setMockServiceManager(service);
-
-    optind = 1; // mimic Lshal::parseArg()
-    EXPECT_NE(0u, mockList->main(createArg({"lshal", "--types=b", "-i", "--neat"})));
-    EXPECT_LE(std::chrono::system_clock::now(), start + 5 * lshalIpcTimeout)
-            << "The main thread should not be blocked by the background task";
-}
-
 class ListVintfTest : public ListTest {
 public:
     virtual void SetUp() override {
@@ -1186,6 +1079,5 @@ TEST_F(HelpTest, UnknownOptionHelp2) {
 
 int main(int argc, char **argv) {
     ::testing::InitGoogleMock(&argc, argv);
-    // Use _exit() to force terminate background threads in Timeout.h
-    _exit(RUN_ALL_TESTS());
+    return RUN_ALL_TESTS();
 }