Fix Darwin implementation of RunEventLoop to return when StopEventLoo…

…pTask is called. (#7854)

Co-authored-by: Jerry Johns <[email protected]>

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();

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<PlatformManagerImpl>::_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()
 {

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<PlatformManagerImpl *>(this); }
 };

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),