Add the ability for EventLoopHandlers to participate in a Select-based event loop (#34433)

* Darwin: Make it possible to build with chip_system_config_use_dispatch=false

Note: This is for unit testing / development purposes only.

* Add the ability for EventLoopHandlers to participate in the event loop

* Avoid leaking the fallback timer in the test

* Expand documentation

* Fix GN logic so fake platform tests can run on Darwin

* Disable TestEventLoopHandler when chip_device_platform == "fake"

The fake PlatformManagerImpl doesn't drive the SystemLayer event loop.

Author: ksperling-apple

src/lib/support/BUILD.gn

@@ -23,6 +23,7 @@ import("${chip_root}/build/chip/chip_version.gni")
 import("${chip_root}/build/chip/java/config.gni")
 import("${chip_root}/build/chip/tests.gni")
 import("${chip_root}/src/lib/core/core.gni")
+import("${chip_root}/src/platform/device.gni")
 
 declare_args() {
   # Set to true to run the PersistentStorageDelegate API compliance audit
@@ -255,7 +256,7 @@ static_library("support") {
     "verhoeff/Verhoeff10.cpp",
   ]
 
-  if (current_os == "android" || matter_enable_java_compilation) {
+  if (chip_device_platform == "android" || matter_enable_java_compilation) {
     if (matter_enable_java_compilation) {
       include_dirs = java_matter_controller_dependent_paths
     }
@@ -304,7 +305,7 @@ static_library("support") {
   # Platforms that utilize CHIP_SYSTEM_CONFIG_PLATFORM_LOG need to
   # be pulled in here as public_deps since they hook into logging at
   # the macro level rather than just providing a LogV implementation.
-  if (current_os == "mac" || current_os == "ios") {
+  if (chip_device_platform == "darwin") {
     public_deps += [ "${chip_root}/src/platform/Darwin:logging" ]
   }
 

src/lib/support/IntrusiveList.h

@@ -414,6 +414,11 @@ class IntrusiveList : public IntrusiveListBase
         ConstIterator(IntrusiveListBase::ConstIteratorBase && base) : IntrusiveListBase::ConstIteratorBase(std::move(base)) {}
         const T * operator->() { return Hook::ToObject(mCurrent); }
         const T & operator*() { return *Hook::ToObject(mCurrent); }
+
+        ConstIterator & operator++() { return static_cast<ConstIterator &>(IntrusiveListBase::ConstIteratorBase::operator++()); }
+        ConstIterator operator++(int) { return IntrusiveListBase::ConstIteratorBase::operator++(1); }
+        ConstIterator & operator--() { return static_cast<ConstIterator &>(IntrusiveListBase::ConstIteratorBase::operator--()); }
+        ConstIterator operator--(int) { return IntrusiveListBase::ConstIteratorBase::operator--(1); }
     };
 
     class Iterator : public IntrusiveListBase::IteratorBase
@@ -426,6 +431,11 @@ class IntrusiveList : public IntrusiveListBase
         Iterator(IntrusiveListBase::IteratorBase && base) : IntrusiveListBase::IteratorBase(std::move(base)) {}
         T * operator->() { return Hook::ToObject(mCurrent); }
         T & operator*() { return *Hook::ToObject(mCurrent); }
+
+        Iterator & operator++() { return static_cast<Iterator &>(IntrusiveListBase::IteratorBase::operator++()); }
+        Iterator operator++(int) { return IntrusiveListBase::IteratorBase::operator++(1); }
+        Iterator & operator--() { return static_cast<Iterator &>(IntrusiveListBase::IteratorBase::operator--()); }
+        Iterator operator--(int) { return IntrusiveListBase::IteratorBase::operator--(1); }
     };
 
     ConstIterator begin() const { return IntrusiveListBase::begin(); }

src/platform/Darwin/PlatformManagerImpl.cpp

@@ -35,7 +35,11 @@
 #include <platform/PlatformManager.h>
 
 // Include the non-inline definitions for the GenericPlatformManagerImpl<> template,
+#if CHIP_SYSTEM_CONFIG_USE_DISPATCH
 #include <platform/internal/GenericPlatformManagerImpl.ipp>
+#else
+#include <platform/internal/GenericPlatformManagerImpl_POSIX.ipp>
+#endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH
 
 #include <CoreFoundation/CoreFoundation.h>
 #include <tracing/metric_event.h>
@@ -64,7 +68,7 @@ CHIP_ERROR PlatformManagerImpl::_InitChipStack()
     ReturnErrorOnFailure(Internal::PosixConfig::Init());
 #endif // CHIP_DISABLE_PLATFORM_KVS
 
-#if !CHIP_SYSTEM_CONFIG_USE_LIBEV
+#if CHIP_SYSTEM_CONFIG_USE_DISPATCH
     // Ensure there is a dispatch queue available
     static_cast<System::LayerSocketsLoop &>(DeviceLayer::SystemLayer()).SetDispatchQueue(GetWorkQueue());
 #endif
@@ -83,6 +87,7 @@ CHIP_ERROR PlatformManagerImpl::_InitChipStack()
     return CHIP_NO_ERROR;
 }
 
+#if CHIP_SYSTEM_CONFIG_USE_DISPATCH
 CHIP_ERROR PlatformManagerImpl::_StartEventLoopTask()
 {
     auto expected = WorkQueueState::kSuspended;
@@ -128,12 +133,6 @@ void PlatformManagerImpl::_RunEventLoop()
     mRunLoopSem = nullptr;
 }
 
-void PlatformManagerImpl::_Shutdown()
-{
-    // Call up to the base class _Shutdown() to perform the bulk of the shutdown.
-    GenericPlatformManagerImpl<ImplClass>::_Shutdown();
-}
-
 CHIP_ERROR PlatformManagerImpl::_PostEvent(const ChipDeviceEvent * event)
 {
     const ChipDeviceEvent eventCopy = *event;
@@ -142,6 +141,7 @@ CHIP_ERROR PlatformManagerImpl::_PostEvent(const ChipDeviceEvent * event)
     });
     return CHIP_NO_ERROR;
 }
+#endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH
 
 #if CHIP_STACK_LOCK_TRACKING_ENABLED
 bool PlatformManagerImpl::_IsChipStackLockedByCurrentThread() const

src/platform/Darwin/PlatformManagerImpl.h

@@ -25,7 +25,12 @@
 
 #include <lib/core/Global.h>
 #include <platform/Darwin/BleScannerDelegate.h>
+
+#if CHIP_SYSTEM_CONFIG_USE_DISPATCH
 #include <platform/internal/GenericPlatformManagerImpl.h>
+#else
+#include <platform/internal/GenericPlatformManagerImpl_POSIX.h>
+#endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH
 
 #include <atomic>
 #include <dispatch/dispatch.h>
@@ -38,7 +43,12 @@ class BleScannerDelegate;
 /**
  * Concrete implementation of the PlatformManager singleton object for Darwin platforms.
  */
-class PlatformManagerImpl final : public PlatformManager, public Internal::GenericPlatformManagerImpl<PlatformManagerImpl>
+class PlatformManagerImpl final : public PlatformManager,
+#if CHIP_SYSTEM_CONFIG_USE_DISPATCH
+                                  public Internal::GenericPlatformManagerImpl<PlatformManagerImpl>
+#else
+                                  public Internal::GenericPlatformManagerImpl_POSIX<PlatformManagerImpl>
+#endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH
 {
     // Allow the PlatformManager interface class to delegate method calls to
     // the implementation methods provided by this class.
@@ -58,8 +68,8 @@ class PlatformManagerImpl final : public PlatformManager, public Internal::Gener
 private:
     // ===== Methods that implement the PlatformManager abstract interface.
     CHIP_ERROR _InitChipStack();
-    void _Shutdown();
 
+#if CHIP_SYSTEM_CONFIG_USE_DISPATCH
     CHIP_ERROR _StartChipTimer(System::Clock::Timeout delay) { return CHIP_ERROR_NOT_IMPLEMENTED; };
     CHIP_ERROR _StartEventLoopTask();
     CHIP_ERROR _StopEventLoopTask();
@@ -69,6 +79,7 @@ class PlatformManagerImpl final : public PlatformManager, public Internal::Gener
     bool _TryLockChipStack() { return false; };
     void _UnlockChipStack(){};
     CHIP_ERROR _PostEvent(const ChipDeviceEvent * event);
+#endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH
 
 #if CHIP_STACK_LOCK_TRACKING_ENABLED
     bool _IsChipStackLockedByCurrentThread() const;

src/platform/Darwin/SystemPlatformConfig.h

@@ -34,14 +34,6 @@ struct ChipDeviceEvent;
 
 // ==================== Platform Adaptations ====================
 
-#if !CHIP_SYSTEM_CONFIG_USE_LIBEV
-// FIXME: these should not be hardcoded here, it is set via build config
-// Need to exclude these for now in libev case
-#define CHIP_SYSTEM_CONFIG_POSIX_LOCKING 0
-#define CHIP_SYSTEM_CONFIG_FREERTOS_LOCKING 0
-#define CHIP_SYSTEM_CONFIG_NO_LOCKING 1
-#endif
-
 #define CHIP_SYSTEM_CONFIG_PLATFORM_PROVIDES_TIME 1
 #define CHIP_SYSTEM_CONFIG_USE_POSIX_TIME_FUNCTS 1
 #define CHIP_SYSTEM_CONFIG_POOL_USE_HEAP 1

src/system/SystemLayer.h

@@ -41,6 +41,7 @@
 #include <system/SystemEvent.h>
 
 #if CHIP_SYSTEM_CONFIG_USE_SOCKETS
+#include <lib/support/IntrusiveList.h>
 #include <system/SocketEvents.h>
 #endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS
 
@@ -243,6 +244,7 @@ class LayerSockets : public Layer
      * Initialize watching for events on a file descriptor.
      *
      * Returns an opaque token through @a tokenOut that must be passed to subsequent operations for this file descriptor.
+     * Multiple calls to start watching the same file descriptor will return the same token.
      * StopWatchingSocket() must be called before closing the file descriptor.
      */
     virtual CHIP_ERROR StartWatchingSocket(int fd, SocketWatchToken * tokenOut) = 0;
@@ -288,6 +290,44 @@ class LayerSockets : public Layer
     virtual SocketWatchToken InvalidSocketWatchToken() = 0;
 };
 
+class LayerSocketsLoop;
+
+/**
+ * EventLoopHandlers can be registered with a LayerSocketsLoop instance to enable
+ * participation of those handlers in the processing cycle of the event loop. This makes
+ * it possible to implement adapters that allow components utilizing a third-party event
+ * loop API to participate in the Matter event loop, instead of having to run an entirely
+ * separate event loop on another thread.
+ *
+ * Specifically, the `PrepareEvents` and `HandleEvents` methods of registered event loop
+ * handlers will be called from the LayerSocketsLoop methods of the same names.
+ *
+ * @see LayerSocketsLoop::PrepareEvents
+ * @see LayerSocketsLoop::HandleEvents
+ */
+class EventLoopHandler : public chip::IntrusiveListNodeBase<>
+{
+public:
+    virtual ~EventLoopHandler() {}
+
+    /**
+     * Prepares events and returns the next requested wake time.
+     */
+    virtual Clock::Timestamp PrepareEvents(Clock::Timestamp now) { return Clock::Timestamp::max(); }
+
+    /**
+     * Handles / dispatches pending events.
+     * Every call to this method will have been preceded by a call to `PrepareEvents`.
+     */
+    virtual void HandleEvents() = 0;
+
+private:
+    // mState is provided exclusively for use by the LayerSocketsLoop implementation
+    // sub-class and can be accessed by it via the LayerSocketsLoop::LoopHandlerState() helper.
+    friend class LayerSocketsLoop;
+    intptr_t mState = 0;
+};
+
 class LayerSocketsLoop : public LayerSockets
 {
 public:
@@ -298,13 +338,22 @@ class LayerSocketsLoop : public LayerSockets
     virtual void HandleEvents()    = 0;
     virtual void EventLoopEnds()   = 0;
 
+#if !CHIP_SYSTEM_CONFIG_USE_DISPATCH
+    virtual void AddLoopHandler(EventLoopHandler & handler)    = 0;
+    virtual void RemoveLoopHandler(EventLoopHandler & handler) = 0;
+#endif // !CHIP_SYSTEM_CONFIG_USE_DISPATCH
+
 #if CHIP_SYSTEM_CONFIG_USE_DISPATCH
     virtual void SetDispatchQueue(dispatch_queue_t dispatchQueue) = 0;
     virtual dispatch_queue_t GetDispatchQueue()                   = 0;
 #elif CHIP_SYSTEM_CONFIG_USE_LIBEV
     virtual void SetLibEvLoop(struct ev_loop * aLibEvLoopP) = 0;
     virtual struct ev_loop * GetLibEvLoop()                 = 0;
 #endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH/LIBEV
+
+protected:
+    // Expose EventLoopHandler.mState as a non-const reference to sub-classes
+    decltype(EventLoopHandler::mState) & LoopHandlerState(EventLoopHandler & handler) { return handler.mState; }
 };
 
 #endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS

src/system/SystemLayerImplSelect.cpp

@@ -28,6 +28,7 @@
 #include <system/SystemLayer.h>
 #include <system/SystemLayerImplSelect.h>
 
+#include <algorithm>
 #include <errno.h>
 
 // Choose an approximation of PTHREAD_NULL if pthread.h doesn't define one.
@@ -370,8 +371,9 @@ CHIP_ERROR LayerImplSelect::StartWatchingSocket(int fd, SocketWatchToken * token
     {
         if (w.mFD == fd)
         {
-            // Duplicate registration is an error.
-            return CHIP_ERROR_INVALID_ARGUMENT;
+            // Already registered, return the existing token
+            *tokenOut = reinterpret_cast<SocketWatchToken>(&w);
+            return CHIP_NO_ERROR;
         }
         if ((w.mFD == kInvalidFd) && (watch == nullptr))
         {
@@ -608,6 +610,32 @@ SocketEvents LayerImplSelect::SocketEventsFromFDs(int socket, const fd_set & rea
     return res;
 }
 
+#if !CHIP_SYSTEM_CONFIG_USE_DISPATCH
+enum : intptr_t
+{
+    kLoopHandlerInactive = 0, // default value for EventLoopHandler::mState
+    kLoopHandlerPending,
+    kLoopHandlerActive,
+};
+
+void LayerImplSelect::AddLoopHandler(EventLoopHandler & handler)
+{
+    // Add the handler as pending because this method can be called at any point
+    // in a PrepareEvents() / WaitForEvents() / HandleEvents() sequence.
+    // It will be marked active when we call PrepareEvents() on it for the first time.
+    auto & state = LoopHandlerState(handler);
+    VerifyOrDie(state == kLoopHandlerInactive);
+    state = kLoopHandlerPending;
+    mLoopHandlers.PushBack(&handler);
+}
+
+void LayerImplSelect::RemoveLoopHandler(EventLoopHandler & handler)
+{
+    mLoopHandlers.Remove(&handler);
+    LoopHandlerState(handler) = kLoopHandlerInactive;
+}
+#endif // !CHIP_SYSTEM_CONFIG_USE_DISPATCH
+
 void LayerImplSelect::PrepareEvents()
 {
     assertChipStackLockedByCurrentThread();
@@ -616,10 +644,28 @@ void LayerImplSelect::PrepareEvents()
     Clock::Timestamp awakenTime        = currentTime + kDefaultMinSleepPeriod;
 
     TimerList::Node * timer = mTimerList.Earliest();
-    if (timer && timer->AwakenTime() < awakenTime)
+    if (timer)
+    {
+        awakenTime = std::min(awakenTime, timer->AwakenTime());
+    }
+
+#if !CHIP_SYSTEM_CONFIG_USE_DISPATCH
+    // Activate added EventLoopHandlers and call PrepareEvents on active handlers.
+    auto loopIter = mLoopHandlers.begin();
+    while (loopIter != mLoopHandlers.end())
     {
-        awakenTime = timer->AwakenTime();
+        auto & loop = *loopIter++; // advance before calling out, in case a list modification clobbers the `next` pointer
+        switch (auto & state = LoopHandlerState(loop))
+        {
+        case kLoopHandlerPending:
+            state = kLoopHandlerActive;
+            [[fallthrough]];
+        case kLoopHandlerActive:
+            awakenTime = std::min(awakenTime, loop.PrepareEvents(currentTime));
+            break;
+        }
     }
+#endif // !CHIP_SYSTEM_CONFIG_USE_DISPATCH
 
     const Clock::Timestamp sleepTime = (awakenTime > currentTime) ? (awakenTime - currentTime) : Clock::kZero;
     Clock::ToTimeval(sleepTime, mNextTimeout);
@@ -683,18 +729,35 @@ void LayerImplSelect::HandleEvents()
         mTimerPool.Invoke(timer);
     }
 
-    for (auto & w : mSocketWatchPool)
+    // Process socket events, if any
+    if (mSelectResult > 0)
     {
-        if (w.mFD != kInvalidFd)
+        for (auto & w : mSocketWatchPool)
         {
-            SocketEvents events = SocketEventsFromFDs(w.mFD, mSelected.mReadSet, mSelected.mWriteSet, mSelected.mErrorSet);
-            if (events.HasAny() && w.mCallback != nullptr)
+            if (w.mFD != kInvalidFd && w.mCallback != nullptr)
             {
-                w.mCallback(events, w.mCallbackData);
+                SocketEvents events = SocketEventsFromFDs(w.mFD, mSelected.mReadSet, mSelected.mWriteSet, mSelected.mErrorSet);
+                if (events.HasAny())
+                {
+                    w.mCallback(events, w.mCallbackData);
+                }
             }
         }
     }
 
+#if !CHIP_SYSTEM_CONFIG_USE_DISPATCH
+    // Call HandleEvents for active loop handlers
+    auto loopIter = mLoopHandlers.begin();
+    while (loopIter != mLoopHandlers.end())
+    {
+        auto & loop = *loopIter++; // advance before calling out, in case a list modification clobbers the `next` pointer
+        if (LoopHandlerState(loop) == kLoopHandlerActive)
+        {
+            loop.HandleEvents();
+        }
+    }
+#endif // !CHIP_SYSTEM_CONFIG_USE_DISPATCH
+
 #if CHIP_SYSTEM_CONFIG_POSIX_LOCKING
     mHandleSelectThread = PTHREAD_NULL;
 #endif // CHIP_SYSTEM_CONFIG_POSIX_LOCKING

src/system/SystemLayerImplSelect.h

@@ -87,6 +87,11 @@ class LayerImplSelect : public LayerSocketsLoop
     void HandleEvents() override;
     void EventLoopEnds() override {}
 
+#if !CHIP_SYSTEM_CONFIG_USE_DISPATCH
+    void AddLoopHandler(EventLoopHandler & handler) override;
+    void RemoveLoopHandler(EventLoopHandler & handler) override;
+#endif // !CHIP_SYSTEM_CONFIG_USE_DISPATCH
+
 #if CHIP_SYSTEM_CONFIG_USE_DISPATCH
     void SetDispatchQueue(dispatch_queue_t dispatchQueue) override { mDispatchQueue = dispatchQueue; };
     dispatch_queue_t GetDispatchQueue() override { return mDispatchQueue; };
@@ -135,6 +140,10 @@ class LayerImplSelect : public LayerSocketsLoop
     TimerList mExpiredTimers;
     timeval mNextTimeout;
 
+#if !CHIP_SYSTEM_CONFIG_USE_DISPATCH
+    IntrusiveList<EventLoopHandler> mLoopHandlers;
+#endif
+
     // Members for select loop
     struct SelectSets
     {