Revert project-chip#8456 Move WakeIOThread() functionality to Watchab…

…leEventManager #### Problem TestPlatformMgr reports errors, ``` CHIP:CSL: System wake event notify failed: OS Error 0x02000009: Bad file descriptor ``` (although it still passes, which obscured the problem). This might be behind intermittent core dumps in CI. #### Change overview Revert project-chip#8456 Move WakeIOThread() functionality to WatchableEventManager #### Testing No error reported by TestPlatformMgr after reverting.
kpschoedel · Jul 21, 2021 · 45b23ba · 45b23ba
1 parent 62083da
commit 45b23ba
Show file tree

Hide file tree

Showing 27 changed files with 506 additions and 1,308 deletions.
diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp
@@ -549,11 +549,11 @@ CHIP_ERROR DeviceController::ServiceEventSignal()
 {
     VerifyOrReturnError(mState == State::Initialized, CHIP_ERROR_INCORRECT_STATE);
 
-#if CONFIG_DEVICE_LAYER && (CHIP_SYSTEM_CONFIG_USE_SOCKETS || CHIP_SYSTEM_CONFIG_USE_NETWORK_FRAMEWORK)
-    DeviceLayer::SystemLayer.WatchableEvents().Signal();
+#if CONFIG_DEVICE_LAYER && CHIP_SYSTEM_CONFIG_USE_IO_THREAD
+    DeviceLayer::SystemLayer.WakeIOThread();
 #else
     ReturnErrorOnFailure(CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE);
-#endif // CONFIG_DEVICE_LAYER && (CHIP_SYSTEM_CONFIG_USE_SOCKETS || CHIP_SYSTEM_CONFIG_USE_NETWORK_FRAMEWORK)
+#endif // CONFIG_DEVICE_LAYER && CHIP_SYSTEM_CONFIG_USE_IO_THREAD
 
     return CHIP_NO_ERROR;
 }

diff --git a/src/controller/java/CHIPDeviceController-JNI.cpp b/src/controller/java/CHIPDeviceController-JNI.cpp
@@ -206,7 +206,7 @@ void JNI_OnUnload(JavaVM * jvm, void * reserved)
     if (sIOThread != PTHREAD_NULL)
     {
         sShutdown = true;
-        sSystemLayer.WatchableEvents().Signal();
+        sSystemLayer.WakeIOThread();
 
         StackUnlockGuard unlockGuard(JniReferences::GetInstance().GetStackLock());
         pthread_join(sIOThread, NULL);

diff --git a/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.cpp b/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.cpp
@@ -127,7 +127,9 @@ void GenericPlatformManagerImpl_POSIX<ImplClass>::_PostEvent(const ChipDeviceEve
 {
     mChipEventQueue.Push(*event);
 
-    SystemLayer.WatchableEvents().Signal(); // Trigger wake select on CHIP thread
+#if CHIP_SYSTEM_CONFIG_USE_IO_THREAD
+    SystemLayer.WakeIOThread(); // Trigger wake select on CHIP thread
+#endif                          // CHIP_SYSTEM_CONFIG_USE_IO_THREAD
 }
 
 template <class ImplClass>
@@ -257,7 +259,7 @@ CHIP_ERROR GenericPlatformManagerImpl_POSIX<ImplClass>::_StopEventLoopTask()
         // SystemLayer.
         //
         Impl()->LockChipStack();
-        SystemLayer.WatchableEvents().Signal();
+        SystemLayer.WakeIOThread();
         Impl()->UnlockChipStack();
 
         pthread_mutex_lock(&mStateLock);

diff --git a/src/include/platform/internal/GenericPlatformManagerImpl_Zephyr.cpp b/src/include/platform/internal/GenericPlatformManagerImpl_Zephyr.cpp
@@ -109,7 +109,7 @@ void GenericPlatformManagerImpl_Zephyr<ImplClass>::_PostEvent(const ChipDeviceEv
     // k_msgq_put takes `void*` instead of `const void*`. Nonetheless, it should be safe to
     // const_cast here and there are components in Zephyr itself which do the same.
     if (k_msgq_put(&mChipEventQueue, const_cast<ChipDeviceEvent *>(event), K_NO_WAIT) == 0)
-        SystemLayer.WatchableEvents().Signal(); // Trigger wake on CHIP thread
+        SystemLayer.WakeIOThread(); // Trigger wake on CHIP thread
     else
         ChipLogError(DeviceLayer, "Failed to post event to CHIP Platform event queue");
 }

diff --git a/src/inet/EndPointBasis.h b/src/inet/EndPointBasis.h
@@ -35,7 +35,7 @@
 #include <support/DLLUtil.h>
 
 #if CHIP_SYSTEM_CONFIG_USE_SOCKETS
-#include <system/WatchableSocket.h>
+#include <system/SystemSockets.h>
 #endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS
 
 #if CHIP_SYSTEM_CONFIG_USE_NETWORK_FRAMEWORK

diff --git a/src/inet/RawEndPoint.cpp b/src/inet/RawEndPoint.cpp
@@ -48,7 +48,7 @@
 #endif                             // CHIP_SYSTEM_CONFIG_USE_LWIP
 
 #if CHIP_SYSTEM_CONFIG_USE_SOCKETS
-#include <system/WatchableSocket.h>
+#include <system/SystemSockets.h>
 #if HAVE_SYS_SOCKET_H
 #include <sys/socket.h>
 #endif // HAVE_SYS_SOCKET_H

diff --git a/src/inet/TCPEndPoint.cpp b/src/inet/TCPEndPoint.cpp
@@ -833,10 +833,10 @@ void TCPEndPoint::EnableReceive()
 
     DriveReceiving();
 
-#if CHIP_SYSTEM_CONFIG_USE_SOCKETS || CHIP_SYSTEM_CONFIG_USE_NETWORK_FRAMEWORK
+#if CHIP_SYSTEM_CONFIG_USE_IO_THREAD
     // Wake the thread waiting for I/O so that it can include the socket.
-    SystemLayer().WatchableEvents().Signal();
-#endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS || CHIP_SYSTEM_CONFIG_USE_NETWORK_FRAMEWORK
+    SystemLayer().WakeIOThread();
+#endif // CHIP_SYSTEM_CONFIG_USE_IO_THREAD
 }
 
 /**

diff --git a/src/platform/Linux/MdnsImpl.h b/src/platform/Linux/MdnsImpl.h
@@ -34,7 +34,7 @@
 #include <avahi-common/watch.h>
 
 #include "lib/mdns/platform/Mdns.h"
-#include "system/WatchableSocket.h"
+#include "system/SystemSockets.h"
 
 struct AvahiWatch
 {

diff --git a/src/platform/mbed/PlatformManagerImpl.cpp b/src/platform/mbed/PlatformManagerImpl.cpp
@@ -46,9 +46,8 @@ CHIP_ERROR PlatformManagerImpl::_InitChipStack(void)
         mQueue.~EventQueue();
         new (&mQueue) events::EventQueue(event_size * CHIP_DEVICE_CONFIG_MAX_EVENT_QUEUE_SIZE);
 
-        mQueue.background([&](int t) {
-            MbedEventTimeout::AttachTimeout([&] { SystemLayer.WatchableEvents().Signal(); }, std::chrono::milliseconds{ t });
-        });
+        mQueue.background(
+            [&](int t) { MbedEventTimeout::AttachTimeout([&] { SystemLayer.WakeIOThread(); }, std::chrono::milliseconds{ t }); });
 
         // Reinitialize the Mutexes
         mThisStateMutex.~Mutex();
@@ -212,7 +211,7 @@ CHIP_ERROR PlatformManagerImpl::_StopEventLoopTask()
 
     // Wake from select so it unblocks processing
     LockChipStack();
-    SystemLayer.WatchableEvents().Signal();
+    SystemLayer.WakeIOThread();
     UnlockChipStack();
 
     osStatus err = osOK;

diff --git a/src/system/BUILD.gn b/src/system/BUILD.gn
@@ -96,7 +96,6 @@ buildconfig_header("system_buildconfig") {
   }
 
   if (chip_system_config_use_sockets) {
-    defines += [ "CHIP_SYSTEM_WATCHABLE_EVENT_MANAGER_CONFIG_FILE=<system/WatchableEventManager${chip_system_config_sockets_event_loop}.h>" ]
     defines += [ "CHIP_SYSTEM_WATCHABLE_SOCKET_CONFIG_FILE=<system/WatchableSocket${chip_system_config_sockets_event_loop}.h>" ]
   }
 }
@@ -143,17 +142,15 @@ static_library("system") {
     "SystemObject.h",
     "SystemPacketBuffer.cpp",
     "SystemPacketBuffer.h",
+    "SystemSockets.cpp",
+    "SystemSockets.h",
     "SystemStats.cpp",
     "SystemStats.h",
     "SystemTimer.cpp",
     "SystemTimer.h",
     "TLVPacketBufferBackingStore.cpp",
     "TLVPacketBufferBackingStore.h",
     "TimeSource.h",
-    "WakeEvent.cpp",
-    "WakeEvent.h",
-    "WatchableEventManager.h",
-    "WatchableSocket.h",
   ]
 
   cflags = [ "-Wconversion" ]
@@ -168,8 +165,6 @@ static_library("system") {
 
   if (chip_system_config_use_sockets) {
     sources += [
-      "WatchableEventManager${chip_system_config_sockets_event_loop}.cpp",
-      "WatchableEventManager${chip_system_config_sockets_event_loop}.h",
       "WatchableSocket${chip_system_config_sockets_event_loop}.cpp",
       "WatchableSocket${chip_system_config_sockets_event_loop}.h",
     ]

diff --git a/src/system/SystemConfig.h b/src/system/SystemConfig.h
@@ -111,6 +111,14 @@
 
 // clang-format off
 
+#ifndef CHIP_SYSTEM_CONFIG_USE_IO_THREAD
+#if CHIP_SYSTEM_CONFIG_USE_SOCKETS || CHIP_SYSTEM_CONFIG_USE_NETWORK_FRAMEWORK
+#define CHIP_SYSTEM_CONFIG_USE_IO_THREAD 1
+#else
+#define CHIP_SYSTEM_CONFIG_USE_IO_THREAD 0
+#endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS || CHIP_SYSTEM_CONFIG_USE_NETWORK_FRAMEWORK
+#endif // CHIP_SYSTEM_CONFIG_USE_IO_THREAD
+
 /**
  *  @def CHIP_SYSTEM_CONFIG_TRANSFER_INETLAYER_PROJECT_CONFIGURATION
  *

diff --git a/src/system/SystemLayer.cpp b/src/system/SystemLayer.cpp
@@ -133,6 +133,11 @@ CHIP_ERROR Layer::Init()
     this->AddEventHandlerDelegate(sSystemEventHandlerDelegate);
 #endif // CHIP_SYSTEM_CONFIG_USE_LWIP
 
+#if CHIP_SYSTEM_CONFIG_USE_SOCKETS || CHIP_SYSTEM_CONFIG_USE_NETWORK_FRAMEWORK
+    // Create an event to allow an arbitrary thread to wake the thread in the select loop.
+    ReturnErrorOnFailure(this->mWakeEvent.Open(mWatchableEvents));
+#endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS || CHIP_SYSTEM_CONFIG_USE_NETWORK_FRAMEWORK
+
     this->mLayerState = kLayerState_Initialized;
 
     return CHIP_NO_ERROR;
@@ -143,6 +148,10 @@ CHIP_ERROR Layer::Shutdown()
     if (this->mLayerState == kLayerState_NotInitialized)
         return CHIP_ERROR_INCORRECT_STATE;
 
+#if CHIP_SYSTEM_CONFIG_USE_SOCKETS || CHIP_SYSTEM_CONFIG_USE_NETWORK_FRAMEWORK
+    ReturnErrorOnFailure(mWakeEvent.Close());
+#endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS || CHIP_SYSTEM_CONFIG_USE_NETWORK_FRAMEWORK
+
     for (size_t i = 0; i < Timer::sPool.Size(); ++i)
     {
         Timer * lTimer = Timer::sPool.Get(*this, i);
@@ -446,6 +455,42 @@ void Layer::HandleTimeout()
 
 #endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS || CHIP_SYSTEM_CONFIG_USE_NETWORK_FRAMEWORK
 
+#if CHIP_SYSTEM_CONFIG_USE_IO_THREAD
+
+/**
+ * Wake up the I/O thread by writing a single byte to the wake pipe.
+ *
+ *  @note
+ *      If @p WakeIOThread() is being called from within an I/O event callback, then writing to the wake pipe can be skipped,
+ * since the I/O thread is already awake.
+ *
+ *      Furthermore, we don't care if this write fails as the only reasonably likely failure is that the pipe is full, in which
+ *      case the select calling thread is going to wake up anyway.
+ */
+void Layer::WakeIOThread()
+{
+    CHIP_ERROR lReturn;
+
+    if (this->State() != kLayerState_Initialized)
+        return;
+
+#if CHIP_SYSTEM_CONFIG_POSIX_LOCKING
+    if (pthread_equal(this->mHandleSelectThread, pthread_self()))
+    {
+        return;
+    }
+#endif // CHIP_SYSTEM_CONFIG_POSIX_LOCKING
+
+    // Send notification to wake up the select call.
+    lReturn = this->mWakeEvent.Notify();
+    if (lReturn != CHIP_NO_ERROR)
+    {
+        ChipLogError(chipSystemLayer, "System wake event notify failed: %s", ErrorStr(lReturn));
+    }
+}
+
+#endif // CHIP_SYSTEM_CONFIG_USE_IO_THREAD
+
 #if CHIP_SYSTEM_CONFIG_USE_LWIP
 LwIPEventHandlerDelegate Layer::sSystemEventHandlerDelegate;
 

diff --git a/src/system/SystemLayer.h b/src/system/SystemLayer.h
@@ -39,8 +39,7 @@
 
 // Include dependent headers
 #if CHIP_SYSTEM_CONFIG_USE_SOCKETS
-#include <system/WakeEvent.h>
-#include <system/WatchableEventManager.h>
+#include <system/SystemSockets.h>
 #endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS
 
 #if CHIP_SYSTEM_CONFIG_POSIX_LOCKING
@@ -137,11 +136,14 @@ class DLL_EXPORT Layer
 
     Clock & GetClock() { return mClock; }
 
-#if CHIP_SYSTEM_CONFIG_USE_SOCKETS || CHIP_SYSTEM_CONFIG_USE_NETWORK_FRAMEWORK
+#if CHIP_SYSTEM_CONFIG_USE_SOCKETS
     WatchableEventManager & WatchableEvents() { return mWatchableEvents; }
     bool GetTimeout(struct timeval & aSleepTime); // TODO(#5556): Integrate timer platform details with WatchableEventManager.
     void HandleTimeout();                         // TODO(#5556): Integrate timer platform details with WatchableEventManager.
-#endif                                            // CHIP_SYSTEM_CONFIG_USE_SOCKETS || CHIP_SYSTEM_CONFIG_USE_NETWORK_FRAMEWORK
+#endif                                            // CHIP_SYSTEM_CONFIG_USE_SOCKETS
+#if CHIP_SYSTEM_CONFIG_USE_IO_THREAD
+    void WakeIOThread();
+#endif // CHIP_SYSTEM_CONFIG_USE_IO_THREAD
 
 #if CHIP_SYSTEM_CONFIG_USE_LWIP
     typedef CHIP_ERROR (*EventHandler)(Object & aTarget, EventType aEventType, uintptr_t aArgument);
@@ -170,8 +172,8 @@ class DLL_EXPORT Layer
 
 #if CHIP_SYSTEM_CONFIG_USE_SOCKETS || CHIP_SYSTEM_CONFIG_USE_NETWORK_FRAMEWORK
     WatchableEventManager mWatchableEvents;
+    WakeEvent mWakeEvent;
 #if CHIP_SYSTEM_CONFIG_POSIX_LOCKING
-    friend class WatchableEventManager;
     std::atomic<pthread_t> mHandleSelectThread;
 #endif // CHIP_SYSTEM_CONFIG_POSIX_LOCKING
 #endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS || CHIP_SYSTEM_CONFIG_USE_NETWORK_FRAMEWORK

diff --git a/src/system/SystemTimer.cpp b/src/system/SystemTimer.cpp
@@ -184,9 +184,9 @@ CHIP_ERROR Timer::Start(uint32_t aDelayMilliseconds, OnCompleteFunct aOnComplete
     }
 #endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH
 
-#if CHIP_SYSTEM_CONFIG_USE_SOCKETS || CHIP_SYSTEM_CONFIG_USE_NETWORK_FRAMEWORK
-    lLayer.WatchableEvents().Signal();
-#endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS || CHIP_SYSTEM_CONFIG_USE_NETWORK_FRAMEWORK
+#if CHIP_SYSTEM_CONFIG_USE_IO_THREAD
+    lLayer.WakeIOThread();
+#endif // CHIP_SYSTEM_CONFIG_USE_IO_THREAD
 #endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS || CHIP_SYSTEM_CONFIG_USE_NETWORK_FRAMEWORK
 
     return CHIP_NO_ERROR;
@@ -220,7 +220,7 @@ CHIP_ERROR Timer::ScheduleWork(OnCompleteFunct aOnComplete, void * aAppState)
     else
     {
 #endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH
-        lLayer.WatchableEvents().Signal();
+        lLayer.WakeIOThread();
 #if CHIP_SYSTEM_CONFIG_USE_DISPATCH
     }
 #endif // CHIP_SYSTEM_CONFIG_USE_DISPATCH