From 9dd0e52a82163427e57c2bd63889e023cd153a8b Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 23 Jun 2021 17:18:18 -0400 Subject: [PATCH] Fix Darwin implementation of RunEventLoop to return when StopEventLoopTask is called. (#7854) Co-authored-by: Jerry Johns --- src/include/platform/PlatformManager.h | 51 ++++++++++ src/platform/Darwin/PlatformManagerImpl.cpp | 39 ++++++-- src/platform/Darwin/PlatformManagerImpl.h | 5 +- src/platform/tests/TestPlatformMgr.cpp | 103 +++++++++++++++++++- 4 files changed, 182 insertions(+), 16 deletions(-) diff --git a/src/include/platform/PlatformManager.h b/src/include/platform/PlatformManager.h index c150b539e5c9a1..273d9df1ed97de 100644 --- a/src/include/platform/PlatformManager.h +++ b/src/include/platform/PlatformManager.h @@ -85,12 +85,63 @@ class PlatformManager typedef void (*EventHandlerFunct)(const ChipDeviceEvent * event, intptr_t arg); + /** + * InitChipStack() initializes the PlatformManager. After calling that, a + * consumer is allowed to call either StartEventLoopTask or RunEventLoop to + * process pending work. Calling both is not allowed: it must be one or the + * other. + */ CHIP_ERROR InitChipStack(); CHIP_ERROR AddEventHandler(EventHandlerFunct handler, intptr_t arg = 0); void RemoveEventHandler(EventHandlerFunct handler, intptr_t arg = 0); + /** + * ScheduleWork can be called after InitChipStack has been called. Calls + * that happen before either StartEventLoopTask or RunEventLoop will queue + * the work up but that work will NOT run until one of those functions is + * called. + */ void ScheduleWork(AsyncWorkFunct workFunct, intptr_t arg = 0); + /** + * Process work items until StopEventLoopTask is called. RunEventLoop will + * not return until work item processing is stopped. Once it returns it + * guarantees that no more work items will be processed unless there's + * another call to RunEventLoop. + * + * Consumers that call RunEventLoop must not call StartEventLoopTask. + * + * Consumers that call RunEventLoop must ensure that RunEventLoop returns + * before calling Shutdown. + */ void RunEventLoop(); + /** + * Process work items until StopEventLoopTask is called. + * + * StartEventLoopTask processes items asynchronously. It can return before + * any items are processed, or after some items have been processed, or + * while an item is being processed, or even after StopEventLoopTask() has + * been called (e.g. if ScheduleWork() was called with a work item that + * calls StopEventLoopTask before StartEventLoopTask was called). + * + * Consumers that call StartEventLoopTask must not call RunEventLoop. + * + * Consumers that call StartEventLoopTask must ensure that they call + * StopEventLoopTask before calling Shutdown. + */ CHIP_ERROR StartEventLoopTask(); + /** + * Stop processing of work items by the event loop. + * + * If called from outside work item processing, StopEventLoopTask guarantees + * that any currently-executing work item completes execution and no more + * work items will run before it returns. This is generally how + * StopEventLoopTask is used in conjunction with StartEventLoopTask. + * + * If called from inside work item processing, StopEventLoopTask makes no + * guarantees about exactly when work item processing will stop. What it + * does guarantee is that if it is used this way in conjunction with + * RunEventLoop then all work item processing will stop before RunEventLoop + * returns. + */ CHIP_ERROR StopEventLoopTask(); void LockChipStack(); bool TryLockChipStack(); diff --git a/src/platform/Darwin/PlatformManagerImpl.cpp b/src/platform/Darwin/PlatformManagerImpl.cpp index 4eb375e85faa6e..e7434b02f452a8 100644 --- a/src/platform/Darwin/PlatformManagerImpl.cpp +++ b/src/platform/Darwin/PlatformManagerImpl.cpp @@ -44,6 +44,8 @@ CHIP_ERROR PlatformManagerImpl::_InitChipStack() err = Internal::PosixConfig::Init(); SuccessOrExit(err); + mRunLoopSem = dispatch_semaphore_create(0); + // Call _InitChipStack() on the generic implementation base class // to finish the initialization process. err = Internal::GenericPlatformManagerImpl::_InitChipStack(); @@ -68,26 +70,43 @@ CHIP_ERROR PlatformManagerImpl::_StartEventLoopTask() CHIP_ERROR PlatformManagerImpl::_StopEventLoopTask() { - if (mIsWorkQueueRunning == true) { mIsWorkQueueRunning = false; - - // dispatch_sync is used in order to guarantee serialization of the caller with - // respect to any tasks that might already be on the queue, or running. - dispatch_sync(mWorkQueue, ^{ - dispatch_suspend(mWorkQueue); - }); + if (dispatch_get_current_queue() != mWorkQueue) + { + // dispatch_sync is used in order to guarantee serialization of the caller with + // respect to any tasks that might already be on the queue, or running. + dispatch_sync(mWorkQueue, ^{ + dispatch_suspend(mWorkQueue); + }); + } + else + { + // We are called from a task running on our work queue. Dispatch async, + // so we don't deadlock ourselves. Note that we do have to dispatch to + // guarantee that we don't signal the semaphore until we have ensured + // that no more tasks will run on the queue. + dispatch_async(mWorkQueue, ^{ + dispatch_suspend(mWorkQueue); + dispatch_semaphore_signal(mRunLoopSem); + }); + } } return CHIP_NO_ERROR; -}; +} void PlatformManagerImpl::_RunEventLoop() { _StartEventLoopTask(); - CFRunLoopRun(); -}; + + // + // Block on the semaphore till we're signalled to stop by + // _StopEventLoopTask() + // + dispatch_semaphore_wait(mRunLoopSem, DISPATCH_TIME_FOREVER); +} CHIP_ERROR PlatformManagerImpl::_Shutdown() { diff --git a/src/platform/Darwin/PlatformManagerImpl.h b/src/platform/Darwin/PlatformManagerImpl.h index 651092ffd74a03..d0990b6e3e3179 100644 --- a/src/platform/Darwin/PlatformManagerImpl.h +++ b/src/platform/Darwin/PlatformManagerImpl.h @@ -80,7 +80,10 @@ class PlatformManagerImpl final : public PlatformManager, public Internal::Gener static PlatformManagerImpl sInstance; dispatch_queue_t mWorkQueue = nullptr; - bool mIsWorkQueueRunning = false; + // Semaphore used to implement blocking behavior in _RunEventLoop. + dispatch_semaphore_t mRunLoopSem; + + bool mIsWorkQueueRunning = false; inline ImplClass * Impl() { return static_cast(this); } }; diff --git a/src/platform/tests/TestPlatformMgr.cpp b/src/platform/tests/TestPlatformMgr.cpp index 981fe6369785c0..5a6ebae34f79fa 100644 --- a/src/platform/tests/TestPlatformMgr.cpp +++ b/src/platform/tests/TestPlatformMgr.cpp @@ -44,15 +44,105 @@ using namespace chip::DeviceLayer; // Unit tests // ================================= -static void TestPlatformMgr_Init(nlTestSuite * inSuite, void * inContext) +static void TestPlatformMgr_InitShutdown(nlTestSuite * inSuite, void * inContext) { CHIP_ERROR err = PlatformMgr().InitChipStack(); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + err = PlatformMgr().Shutdown(); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); +} + +static void TestPlatformMgr_BasicEventLoopTask(nlTestSuite * inSuite, void * inContext) +{ + CHIP_ERROR err = PlatformMgr().InitChipStack(); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + err = PlatformMgr().StartEventLoopTask(); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + err = PlatformMgr().StopEventLoopTask(); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + err = PlatformMgr().Shutdown(); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); +} + +static bool stopRan; + +static void StopTheLoop(intptr_t) +{ + // Testing the return value here would involve multi-threaded access to the + // nlTestSuite, and it's not clear whether that's OK. + stopRan = true; + PlatformMgr().StopEventLoopTask(); +} + +static void TestPlatformMgr_BasicRunEventLoop(nlTestSuite * inSuite, void * inContext) +{ + stopRan = false; + + CHIP_ERROR err = PlatformMgr().InitChipStack(); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + PlatformMgr().ScheduleWork(StopTheLoop); + + PlatformMgr().RunEventLoop(); + NL_TEST_ASSERT(inSuite, stopRan); + + err = PlatformMgr().Shutdown(); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); } -static void TestPlatformMgr_StartEventLoopTask(nlTestSuite * inSuite, void * inContext) +static bool sleepRan; + +static void SleepSome(intptr_t) { - CHIP_ERROR err = PlatformMgr().StartEventLoopTask(); + sleep(1); + sleepRan = true; +} + +static void TestPlatformMgr_RunEventLoopTwoTasks(nlTestSuite * inSuite, void * inContext) +{ + stopRan = false; + sleepRan = false; + + CHIP_ERROR err = PlatformMgr().InitChipStack(); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + PlatformMgr().ScheduleWork(SleepSome); + PlatformMgr().ScheduleWork(StopTheLoop); + + PlatformMgr().RunEventLoop(); + NL_TEST_ASSERT(inSuite, stopRan); + NL_TEST_ASSERT(inSuite, sleepRan); + + err = PlatformMgr().Shutdown(); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); +} + +void StopAndSleep(intptr_t arg) +{ + // Ensure that we don't proceed after stopping until the sleep is done too. + StopTheLoop(arg); + SleepSome(arg); +} + +static void TestPlatformMgr_RunEventLoopStopBeforeSleep(nlTestSuite * inSuite, void * inContext) +{ + stopRan = false; + sleepRan = false; + + CHIP_ERROR err = PlatformMgr().InitChipStack(); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + PlatformMgr().ScheduleWork(StopAndSleep); + + PlatformMgr().RunEventLoop(); + NL_TEST_ASSERT(inSuite, stopRan); + NL_TEST_ASSERT(inSuite, sleepRan); + + err = PlatformMgr().Shutdown(); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); } @@ -92,8 +182,11 @@ static void TestPlatformMgr_AddEventHandler(nlTestSuite * inSuite, void * inCont */ static const nlTest sTests[] = { - NL_TEST_DEF("Test PlatformMgr::Init", TestPlatformMgr_Init), - NL_TEST_DEF("Test PlatformMgr::StartEventLoopTask", TestPlatformMgr_StartEventLoopTask), + NL_TEST_DEF("Test PlatformMgr::Init/Shutdown", TestPlatformMgr_InitShutdown), + NL_TEST_DEF("Test basic PlatformMgr::StartEventLoopTask", TestPlatformMgr_BasicEventLoopTask), + NL_TEST_DEF("Test basic PlatformMgr::RunEventLoop", TestPlatformMgr_BasicRunEventLoop), + NL_TEST_DEF("Test PlatformMgr::RunEventLoop with two tasks", TestPlatformMgr_RunEventLoopTwoTasks), + NL_TEST_DEF("Test PlatformMgr::RunEventLoop with stop before sleep", TestPlatformMgr_RunEventLoopStopBeforeSleep), NL_TEST_DEF("Test PlatformMgr::TryLockChipStack", TestPlatformMgr_TryLockChipStack), NL_TEST_DEF("Test PlatformMgr::AddEventHandler", TestPlatformMgr_AddEventHandler),