diff options
author | Yifan Hong <elsk@google.com> | 2024-02-01 16:36:37 -0800 |
---|---|---|
committer | Yifan Hong <elsk@google.com> | 2024-02-01 17:14:49 -0800 |
commit | 15136273928b577c551bb44643ab6994688d0c0d (patch) | |
tree | fb1fb7bbebf211bdfd547cc16f404591435e0832 | |
parent | 6b13e50920d2b6693861eed9c86e81939e05a12e (diff) | |
download | native-15136273928b577c551bb44643ab6994688d0c0d.tar.gz |
Revert^2 "lshal: use std::async"
This reverts commit 2cfbc514764472becab46b41ebe0323a68369cad.
Reason for revert: reapply change
Bug: 323268003
Bug: 311143089
Test: See next CL
Change-Id: I392eca8f3368c1d74b4de37d5f49663d3ddbf7e0
-rw-r--r-- | cmds/lshal/Timeout.h | 91 | ||||
-rw-r--r-- | cmds/lshal/main.cpp | 3 | ||||
-rw-r--r-- | cmds/lshal/test.cpp | 122 |
3 files changed, 143 insertions, 73 deletions
diff --git a/cmds/lshal/Timeout.h b/cmds/lshal/Timeout.h index 37f41beea9..d97ba89122 100644 --- a/cmds/lshal/Timeout.h +++ b/cmds/lshal/Timeout.h @@ -16,83 +16,44 @@ #pragma once -#include <condition_variable> #include <chrono> -#include <functional> -#include <mutex> -#include <thread> +#include <future> #include <hidl/Status.h> +#include <utils/Errors.h> namespace android { namespace lshal { -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; -} - +// 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. 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; - 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) { + + // 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) { return Status::fromStatusT(TIMED_OUT); } - return ret; + return Status::fromExceptionCode(Status::Exception::EX_ILLEGAL_STATE, "Illegal future_status"); } - -} // namespace lshal -} // namespace android +} // namespace lshal +} // namespace android diff --git a/cmds/lshal/main.cpp b/cmds/lshal/main.cpp index 366c9383a2..bd5fa32521 100644 --- a/cmds/lshal/main.cpp +++ b/cmds/lshal/main.cpp @@ -18,5 +18,6 @@ int main(int argc, char **argv) { using namespace ::android::lshal; - return Lshal{}.main(Arg{argc, argv}); + // Use _exit() to force terminate background threads in Timeout.h + _exit(Lshal{}.main(Arg{argc, argv})); } diff --git a/cmds/lshal/test.cpp b/cmds/lshal/test.cpp index cba7c4bf2a..c24f827e73 100644 --- a/cmds/lshal/test.cpp +++ b/cmds/lshal/test.cpp @@ -14,6 +14,10 @@ * 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> @@ -36,6 +40,8 @@ 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; @@ -934,12 +940,9 @@ TEST_F(ListTest, DumpDebug) { return hardware::Void(); })); EXPECT_CALL(*serviceManager, get(_, _)) - .WillRepeatedly( - Invoke([&](const hidl_string&, const hidl_string& instance) -> sp<IBase> { - int id = getIdFromInstanceName(instance); - if (id > inheritanceLevel) return nullptr; - return sp<IBase>(service); - })); + .WillRepeatedly(Invoke([&](const hidl_string&, const hidl_string&) -> sp<IBase> { + return sp<IBase>(service); + })); const std::string expected = "[fake description 0]\n" "Interface\n" @@ -957,6 +960,110 @@ 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 { @@ -1079,5 +1186,6 @@ TEST_F(HelpTest, UnknownOptionHelp2) { int main(int argc, char **argv) { ::testing::InitGoogleMock(&argc, argv); - return RUN_ALL_TESTS(); + // Use _exit() to force terminate background threads in Timeout.h + _exit(RUN_ALL_TESTS()); } |