Generalize socket-based event loop

[kpschoedel]

Problem

Socket event management is currently tied closely to select(2),
and does not integrate well with other event loops.

See issue #5556 rework system layer event loop

Change overview

The primary purpose of this change is to provide a clear interface for
monitoring socket events, that can be used both within the CHIP SDK
and by an application using the SDK, and can be implemented either
inside or outside the SDK. Functionality does not change.

• Defines an interface, in system/SystemSockets.h.
• Converts most existing uses of socket event monitoring (except mDNS,
  to be done as a followup).
• Converts the existing select(2)-based event management into an
  implementation of the new interface.
• Adds a second implementation of the interface, using libevent,
  to verify that the interface is general enough.

Testing

• Converted the applicable unit tests from select(2) to this interface,
  and ran them with both the select and libevent implementations.
  (CI remains select-only for now.)
• Integration tests involving socket-based platforms exercise this code
  and confirm no change to functionality.
• Manual verification of controller/device communication

[gerickson]

It's unclear from the current arrangement of the code whether CHIP_SYSTEM_CONFIG_USE_IO_THREAD == (CHIP_SYSTEM_CONFIG_USE_SOCKETS == 1) && (CHIP_SYSTEM_CONFIG_SOCKETS_USE_SELECT == 0) or if CHIP_SYSTEM_CONFIG_USE_IO_THREAD is completely independent of either of those.

[kpschoedel]

Yes, this is a part that needs to be fleshed out. It's independent of the interface for file descriptor watching, but dependent on the implementation. Some may require a thread (as select does), some may require there not be one (e.g. using a CHIP-consuming application's mechanism instead), and some may go either way. Actual #if tests should end up much reduced, since I can probably abstract WakeIOThread() (previously WakeSelect()) a bit more.

[gerickson]

Any reason to not make IsReadable() as a convenience inline for Has(System::SocketEventFlag::kRead)?

[kpschoedel]

The pattern for most other flag sets in the code is not to have them, so I didn't re-add them when moving SocketEvents to use the type-safe BitFlags when I had to move it from inet to system. Easy enough to replace. (A bit sad that enum class can't have methods, not to mention a bitwise mixin.)

[gerickson]

Any reason to not make IsWriteable() as a convenience inline for Has(System::SocketEventFlag::kWrite)?

[gerickson]

Probably worth factoring this out as a simple one-liner PR.

[kpschoedel]

Yes. (The reason I touched this actually no longer exists in this PR. Most uses of CanCastTo are testing dynamically computer values, but I think there are a small number of tests that could be replaced with a static_assert. CanCastTo couldn't be constexpr in C++11, but can be now in C++14.)

[gerickson]

See prior comments about CHIP_SYSTEM_CONFIG_USE_IO_THREAD. There might be an opportunity to detect and flag incorrect configurations.

[gerickson]

See prior comments on convenience inlines.

[gerickson]

Will check this out. With CHIP, one of my integration goals would be / is to make the CHIP stack an easy add-on as a CoreFoundation run loop source. I'll be looking at this through that integration lens.

[gerickson]

Will check this out. With CHIP, one of my integration goals would be / is to make the CHIP stack an easy add-on as a CoreFoundation run loop source. I'll be looking at this through that integration lens.

The PR seems reasonable; however, not having ever worked with Darwin libnetwork, I'm probably not the best person to assess that integration goal (but the code doesn't seem harmful toward that end at all and certainly seems to help). As for the CoreFoundation run loop, at minimum, such an integration would seem to require an import/export of sockets, which is likely no different / worse than integrating CoreFoundation run loops with a pure BSD sockets implementation, as opposed to CFNetwork sources.

[msandstedt]

I had previously found that for the posix platform, the following was sufficient to allow driving the task function from an existing event loop:

diff --git a/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.cpp b/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.cpp
index 5d34c339bb..abe2087683 100644
--- a/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.cpp
+++ b/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.cpp
@@ -155,8 +155,11 @@ void GenericPlatformManagerImpl_POSIX<ImplClass>::SysProcess()
     nextTimeoutMs = mNextTimeout.tv_sec * 1000 + mNextTimeout.tv_usec / 1000;
     _StartChipTimer(nextTimeoutMs);
 
+    // override; select must not block.
+    struct timeval immediate = { .tv_sec = 0, .tv_usec = 0 };
+
     Impl()->UnlockChipStack();
-    selectRes = select(mMaxFd + 1, &mReadSet, &mWriteSet, &mErrorSet, &mNextTimeout);
+    selectRes = select(mMaxFd + 1, &mReadSet, &mWriteSet, &mErrorSet, &immediate);
     Impl()->LockChipStack();
 
     if (selectRes < 0)
@@ -192,7 +195,7 @@ void GenericPlatformManagerImpl_POSIX<ImplClass>::_RunEventLoop()
     {
         SysUpdate();
         SysProcess();
-    } while (mShouldRunEventLoop.load(std::memory_order_relaxed));
+    } while (0); // mShouldRunEventLoop.load(std::memory_order_relaxed));
 
     Impl()->UnlockChipStack();
 }

One of the main sales pitches here is that this set of changes also can allow such a thing. But if the above is so concise, why are the proposed changes in this PR better? I'm not asking this as a rhetorical question. It is implied what's in this PR is better and I want to understand why.

I can see some other problems with select, but I don't think we have a C10K problem to solve. I'm therefore assuming performance isn't a primary motivator.

[mspang]

I had previously found that for the posix platform, the following was sufficient to allow driving the task function from an existing event loop:

diff --git a/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.cpp b/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.cpp
index 5d34c339bb..abe2087683 100644
--- a/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.cpp
+++ b/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.cpp
@@ -155,8 +155,11 @@ void GenericPlatformManagerImpl_POSIX<ImplClass>::SysProcess()
     nextTimeoutMs = mNextTimeout.tv_sec * 1000 + mNextTimeout.tv_usec / 1000;
     _StartChipTimer(nextTimeoutMs);
 
+    // override; select must not block.
+    struct timeval immediate = { .tv_sec = 0, .tv_usec = 0 };
+
     Impl()->UnlockChipStack();
-    selectRes = select(mMaxFd + 1, &mReadSet, &mWriteSet, &mErrorSet, &mNextTimeout);
+    selectRes = select(mMaxFd + 1, &mReadSet, &mWriteSet, &mErrorSet, &immediate);
     Impl()->LockChipStack();
 
     if (selectRes < 0)
@@ -192,7 +195,7 @@ void GenericPlatformManagerImpl_POSIX<ImplClass>::_RunEventLoop()
     {
         SysUpdate();
         SysProcess();
-    } while (mShouldRunEventLoop.load(std::memory_order_relaxed));
+    } while (0); // mShouldRunEventLoop.load(std::memory_order_relaxed));
 
     Impl()->UnlockChipStack();
 }

That code does not seem sufficient; I don't even see how you would correctly combine this approach with polling other (non-CHIP) file descriptors on the same thread.

To fix the problem, CHIP must register its file descriptors to be included in application's poll set, so that the thread can block itself pending IO on either CHIP owned or application owned file descriptors. Properly there can only be one blocking I/O poll on any given thread. If instead CHIP does a non-blocking poll, and the application does a blocking one, then we won't wake immediately when e.g. UDP packets arrive for CHIP. It may appear to work if there's ongoing activity, but it risks unbounded additional response latency.

This is not about scalability, it's about having the flexibility to use an applications' preferred event loop for the file descriptors managed by CHIP.

One of the main sales pitches here is that this set of changes also can allow such a thing. But if the above is so concise, why are the proposed changes in this PR better? I'm not asking this as a rhetorical question. It is implied what's in this PR is better and I want to understand why.

I can see some other problems with select, but I don't think we have a C10K problem to solve. I'm therefore assuming performance isn't a primary motivator.

[mspang]

CHIP_SYSTEM_CONFIG_USE_LWIP -> CHIP_SYSTEM_CONFIG_USE_SOCKETS ?

[kpschoedel]

Yes. I will definitely fix and add substantially more documentation.

[mspang]

Should this be event_base_loop?

[kpschoedel]

Probably. I've gone back and forth on what exactly HandleEvents() should mean, so the second opinion is welcome.

[mspang]

Will we able to remove this discrepancy and make both work with SetCallback & remove HandleSelectResult?

[kpschoedel]

Yes, I think that should be easy; I just put it off. The remaining #if-select() code would be the PrepareSelect/HandleSelectResult stuff in //system, which looks a bit harder to refactor, but probably possible — if anybody's still going to want to use select() in the long run.

[mspang]

We could replace this class with

using SystemSocketWatcherCallback = void(int socket, SocketEvents events, void * data);

?

[kpschoedel]

Yes. The tentative reason for the otherwise-vacuous base class is to have an easily-found place to document the SystemSocketWatcher interface.

[kpschoedel]

Actually, after further investigation, I do want to have a base class containing the file descriptor and active-event flags — essentially, changing the consumers' members from int mSocket; SocketEvents mPendingIO; SystemSocketWatcher mWatcher; as here, to WatchableSocket mSocket.

[gerickson]

@mspang and @msandstedt both raise good points here.

Ideally, I can have a single-threaded, simple application with multiple "sources" on the main select loop. CHIP should be one potential integration source in such an application. Perhaps Avahi has some inspiring APIs in this regard in terms of how Avahi can be added as such an integration source. It's been a decade-plus since I've looked at that code; however, I think it amounted to being able to import/export the descriptors Avahi uses for its passive/active sockets and being able to patch those in/out of the main select loop with appropriate callbacks when those particular descriptors have "work" to do.

IIRC, Apple's mDNS/DNS-SD CFNetworkService code works similarly but in a CFRunLoop-centric way (which is just, at its core, a select loop as hand-waved above). There's a main application CFRunLoop and CFNetworkService is patched in as a source of work on that run loop. IIRC, there was/is no need to maintain a separate mDNS/DNS-SD work thread and manage complexities around multiple threads.

If the CHIP stack had the ability, on POSIX-like systems, to export/import descriptors similarly, then it would be possible to have a CFRunLoop adapter, a GLib adapter, etc. for the stack to integrate with each of those run loop types.