Make chip-tool run on a single event loop (#7824)

* Make it possible to run chip-tool on a single event loop. * Cleaning up the state lock and conditional vars in the POSIX implementation of PlatformManager in _StopEventLoopTask() was too over-eager - this resulted in complaints from TSAN that it was still being accessed by RunEventLoop() when it finally came out of the runloop due to _StopEventLoopTask() signalling its termination. Fix: Move that clean-up to _Shutdown(), where it arguably should have been in the first place. This is nicely symmetric to its initialization in InitChipTask(). * Fix typo in artifact name * Address review comments Co-authored-by: Boris Zbarsky <[email protected]>
project-chip · Jul 14, 2021 · 1308808 · 1308808
1 parent c300fc9
commit 1308808
Show file tree

Hide file tree

Showing 6 changed files with 113 additions and 48 deletions.
diff --git a/.github/workflows/tests.yaml b/.github/workflows/tests.yaml
@@ -31,6 +31,10 @@ jobs:
         strategy:
           matrix:
             type: [debug, tsan]
+            eventloop: [eventloop_same, eventloop_separate]
+        env:
+            USE_SEPARATE_EVENTLOOP: ${{ matrix.eventloop == 'eventloop_separate' }}
+            USE_TSAN: ${{ matrix.type == 'tsan' }}
 
         if: github.actor != 'restyled-io[bot]'
         runs-on: ubuntu-latest
@@ -58,30 +62,18 @@ jobs:
               uses: actions/upload-artifact@v2
               if: ${{ always() }}
               with:
-                  name: bootstrap-logs-${{ matrix.type }}
+                  name: bootstrap-logs-linux-${{ matrix.type }}-${{ matrix.eventloop }}
                   path: |
                    .environment/gn_out/.ninja_log
                    .environment/pigweed-venv/*.log
             - name: Build all clusters app
               timeout-minutes: 5
               run: |
-                  case ${{ matrix.type }} in
-                    "debug") GN_ARGS='is_tsan=false';;
-                    "tsan") GN_ARGS='is_tsan=true';;
-                    *) ;;
-                  esac
-              
-                  scripts/examples/gn_build_example.sh examples/all-clusters-app/linux out/debug/standalone/ chip_config_network_layer_ble=false "$GN_ARGS"
+                  scripts/examples/gn_build_example.sh examples/all-clusters-app/linux out/debug/standalone/ chip_config_network_layer_ble=false is_tsan=${USE_TSAN}
             - name: Build chip-tool
               timeout-minutes: 5
               run: |
-                  case ${{ matrix.type }} in
-                    "debug") GN_ARGS='is_tsan=false';;
-                    "tsan") GN_ARGS='is_tsan=true';;
-                    *) ;;
-                  esac
-                
-                  scripts/examples/gn_build_example.sh examples/chip-tool out/debug/standalone/ "$GN_ARGS"
+                  scripts/examples/gn_build_example.sh examples/chip-tool out/debug/standalone/ is_tsan=${USE_TSAN} config_use_separate_eventloop=${USE_SEPARATE_EVENTLOOP}
             - name: Copy objdir
               run: |
                   # The idea is to not upload our objdir unless builds have
@@ -96,15 +88,15 @@ jobs:
               uses: actions/upload-artifact@v2
               if: ${{ failure() }}
               with:
-                  name: crash-core-linux-${{ matrix.type }}
+                  name: crash-core-linux-${{ matrix.type }}-${{ matrix.eventloop }}
                   path: /tmp/cores/
                   # Cores are big; don't hold on to them too long.
                   retention-days: 5
             - name: Uploading objdir for debugging
               uses: actions/upload-artifact@v2
               if: ${{ failure() }}
               with:
-                  name: crash-objdir-linux-${{ matrix.type }}
+                  name: crash-objdir-linux-${{ matrix.type }}-${{ matrix.eventloop }}
                   path: objdir-clone/
                   # objdirs are big; don't hold on to them too long.
                   retention-days: 5
@@ -115,6 +107,10 @@ jobs:
         strategy:
           matrix:
             type: [debug, tsan]
+            eventloop: [eventloop_same, eventloop_separate]
+        env:
+            USE_SEPARATE_EVENTLOOP: ${{ matrix.eventloop == 'eventloop_separate' }}
+            USE_TSAN: ${{ matrix.type == 'tsan' }}
 
         if: github.actor != 'restyled-io[bot]'
         runs-on: macos-latest
@@ -147,30 +143,18 @@ jobs:
               uses: actions/upload-artifact@v2
               if: ${{ always() }}
               with:
-                  name: bootstrap-logs-${{ matrix.type }}
+                  name: bootstrap-logs-darwin-${{ matrix.type }}-${{ matrix.eventloop }}
                   path: |
                    .environment/gn_out/.ninja_log
                    .environment/pigweed-venv/*.log
             - name: Run Build Test Server
               timeout-minutes: 5
               run: |
-                  case ${{ matrix.type }} in
-                    "debug") GN_ARGS='is_tsan=false';;
-                    "tsan") GN_ARGS='is_tsan=true';;
-                    *) ;;
-                  esac
-
-                  scripts/examples/gn_build_example.sh examples/all-clusters-app/linux out/debug/standalone/ chip_config_network_layer_ble=false "$GN_ARGS"
+                  scripts/examples/gn_build_example.sh examples/all-clusters-app/linux out/debug/standalone/ chip_config_network_layer_ble=false is_tsan=${USE_TSAN}
             - name: Build chip-tool
               timeout-minutes: 5
               run: |
-                  case ${{ matrix.type }} in
-                    "debug") GN_ARGS='is_tsan=false';;
-                    "tsan") GN_ARGS='is_tsan=true';;
-                    *) ;;
-                  esac
-
-                  scripts/examples/gn_build_example.sh examples/chip-tool out/debug/standalone/ "$GN_ARGS"
+                  scripts/examples/gn_build_example.sh examples/chip-tool out/debug/standalone/ is_tsan=${USE_TSAN} config_use_separate_eventloop=${USE_SEPARATE_EVENTLOOP}
             - name: Copy objdir
               run: |
                   # The idea is to not upload our objdir unless builds have
@@ -184,21 +168,21 @@ jobs:
               uses: actions/upload-artifact@v2
               if: ${{ failure() }}
               with:
-                  name: crash-core-darwin-${{ matrix.type }}
+                  name: crash-core-darwin-${{ matrix.type }}-${{ matrix.eventloop }}
                   path: /cores/
                   # Cores are big; don't hold on to them too long.
                   retention-days: 5
             - name: Uploading diagnostic logs
               uses: actions/upload-artifact@v2
               if: ${{ failure() }}
               with:
-                  name: crash-log-darwin-${{ matrix.type }}
+                  name: crash-log-darwin-${{ matrix.type }}-${{ matrix.eventloop }}
                   path: ~/Library/Logs/DiagnosticReports/
             - name: Uploading objdir for debugging
               uses: actions/upload-artifact@v2
               if: ${{ failure() }}
               with:
-                  name: crash-objdir-darwin-${{ matrix.type }}
+                  name: crash-objdir-darwin-${{ matrix.type }}-${{ matrix.eventloop }}
                   path: objdir-clone/
                   # objdirs are big; don't hold on to them too long.
                   retention-days: 5
diff --git a/examples/chip-tool/BUILD.gn b/examples/chip-tool/BUILD.gn
@@ -19,6 +19,11 @@ import("${chip_root}/build/chip/tools.gni")
 
 assert(chip_build_tools)
 
+declare_args() {
+  # Use a separate eventloop for CHIP tasks
+  config_use_separate_eventloop = true
+}
+
 executable("chip-tool") {
   sources = [
     "commands/clusters/ModelCommand.cpp",
@@ -36,6 +41,8 @@ executable("chip-tool") {
     "main.cpp",
   ]
 
+  defines = [ "CONFIG_USE_SEPARATE_EVENTLOOP=${config_use_separate_eventloop}" ]
+
   deps = [
     "${chip_root}/src/controller/data_model",
     "${chip_root}/src/lib",

diff --git a/examples/chip-tool/commands/common/Command.cpp b/examples/chip-tool/commands/common/Command.cpp
@@ -17,6 +17,7 @@
  */
 
 #include "Command.h"
+#include "platform/PlatformManager.h"
 
 #include <netdb.h>
 #include <sstream>
@@ -378,20 +379,61 @@ const char * Command::GetAttribute(void) const
 
 void Command::UpdateWaitForResponse(bool value)
 {
+#if CONFIG_USE_SEPARATE_EVENTLOOP
     {
         std::lock_guard<std::mutex> lk(cvWaitingForResponseMutex);
         mWaitingForResponse = value;
     }
     cvWaitingForResponse.notify_all();
+#else  // CONFIG_USE_SEPARATE_EVENTLOOP
+    if (value == false)
+    {
+        if (mCommandExitStatus != CHIP_NO_ERROR)
+        {
+            ChipLogError(chipTool, "Run command failure: %s", chip::ErrorStr(mCommandExitStatus));
+        }
+
+        chip::DeviceLayer::PlatformMgr().StopEventLoopTask();
+    }
+#endif // CONFIG_USE_SEPARATE_EVENTLOOP
 }
 
-void Command::WaitForResponse(uint16_t duration)
+#if CONFIG_USE_SEPARATE_EVENTLOOP
+
+void Command::WaitForResponse(uint16_t seconds)
 {
-    std::chrono::seconds waitingForResponseTimeout(duration);
+    std::chrono::seconds waitingForResponseTimeout(seconds);
     std::unique_lock<std::mutex> lk(cvWaitingForResponseMutex);
     auto waitingUntil = std::chrono::system_clock::now() + waitingForResponseTimeout;
     if (!cvWaitingForResponse.wait_until(lk, waitingUntil, [this]() { return !this->mWaitingForResponse; }))
     {
         ChipLogError(chipTool, "No response from device");
     }
 }
+
+#else // CONFIG_USE_SEPARATE_EVENTLOOP
+
+static void OnResponseTimeout(chip::System::Layer *, void *, chip::System::Error)
+{
+    ChipLogError(chipTool, "No response from device");
+
+    chip::DeviceLayer::PlatformMgr().StopEventLoopTask();
+}
+
+CHIP_ERROR Command::ScheduleWaitForResponse(uint16_t seconds)
+{
+    chip::System::Timer * timer = nullptr;
+
+    CHIP_ERROR err = chip::DeviceLayer::SystemLayer.NewTimer(timer);
+    if (err == CHIP_NO_ERROR)
+    {
+        timer->Start(seconds * 1000, OnResponseTimeout, this);
+    }
+    else
+    {
+        ChipLogError(chipTool, "Failed to allocate timer");
+    }
+    return err;
+}
+
+#endif // CONFIG_USE_SEPARATE_EVENTLOOP
diff --git a/examples/chip-tool/commands/common/Command.h b/examples/chip-tool/commands/common/Command.h
@@ -187,7 +187,18 @@ class Command
     }
 
     void UpdateWaitForResponse(bool value);
-    void WaitForResponse(uint16_t duration);
+
+    // There is a certain symmetry between the single-event-loop and
+    // separate-event-loop approaches.  With a separate event loop, we schedule
+    // our work on that event loop and synchronously wait (block) waiting for a
+    // response. When using a single event loop, we ask for an async response
+    // notification and then block processing work on the event loop
+    // synchronously until that notification happens.
+#if CONFIG_USE_SEPARATE_EVENTLOOP
+    void WaitForResponse(uint16_t seconds);
+#else  // CONFIG_USE_SEPARATE_EVENTLOOP
+    CHIP_ERROR ScheduleWaitForResponse(uint16_t seconds);
+#endif // CONFIG_USE_SEPARATE_EVENTLOOP
 
 protected:
     ExecutionContext * GetExecContext() { return mExecContext; }
@@ -202,7 +213,9 @@ class Command
     const char * mName            = nullptr;
     std::vector<Argument> mArgs;
 
+#if CONFIG_USE_SEPARATE_EVENTLOOP
     std::condition_variable cvWaitingForResponse;
     std::mutex cvWaitingForResponseMutex;
     bool mWaitingForResponse{ false };
+#endif // CONFIG_USE_SEPARATE_EVENTLOOP
 };
diff --git a/examples/chip-tool/commands/common/Commands.cpp b/examples/chip-tool/commands/common/Commands.cpp
@@ -70,16 +70,34 @@ int Commands::Run(NodeId localId, NodeId remoteId, int argc, char ** argv)
     err = mController.Init(localId, initParams);
     VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init failure! Commissioner: %s", chip::ErrorStr(err)));
 
+#if CONFIG_USE_SEPARATE_EVENTLOOP
+    // ServiceEvents() calls StartEventLoopTask(), which is paired with the
+    // StopEventLoopTask() below.
     err = mController.ServiceEvents();
     VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(Controller, "Init failure! Run Loop: %s", chip::ErrorStr(err)));
+#endif // CONFIG_USE_SEPARATE_EVENTLOOP
 
     err = RunCommand(localId, remoteId, argc, argv, &command);
     SuccessOrExit(err);
 
+#if !CONFIG_USE_SEPARATE_EVENTLOOP
+    chip::DeviceLayer::PlatformMgr().RunEventLoop();
+#endif // !CONFIG_USE_SEPARATE_EVENTLOOP
+
+    if (command)
+    {
+        err = command->GetCommandExitStatus();
+    }
+
 exit:
-#if CONFIG_DEVICE_LAYER
+    if (err != CHIP_NO_ERROR)
+    {
+        ChipLogError(chipTool, "Run command failure: %s", chip::ErrorStr(err));
+    }
+
+#if CONFIG_USE_SEPARATE_EVENTLOOP
     chip::DeviceLayer::PlatformMgr().StopEventLoopTask();
-#endif
+#endif // CONFIG_USE_SEPARATE_EVENTLOOP
 
     if (command)
     {
@@ -176,14 +194,14 @@ CHIP_ERROR Commands::RunCommand(NodeId localId, NodeId remoteId, int argc, char
         // set the variable to true, which will cause it to block indefinitely.
         //
         command->UpdateWaitForResponse(true);
+#if CONFIG_USE_SEPARATE_EVENTLOOP
         chip::DeviceLayer::PlatformMgr().ScheduleWork(RunQueuedCommand, reinterpret_cast<intptr_t>(command));
         command->WaitForResponse(command->GetWaitDurationInSeconds());
-        err = command->GetCommandExitStatus();
-        if (err != CHIP_NO_ERROR)
-        {
-            ChipLogError(chipTool, "Run command failure: %s", chip::ErrorStr(err));
-            ExitNow();
-        }
+#else  // CONFIG_USE_SEPARATE_EVENTLOOP
+        err = command->Run();
+        SuccessOrExit(err);
+        command->ScheduleWaitForResponse(command->GetWaitDurationInSeconds());
+#endif // CONFIG_USE_SEPARATE_EVENTLOOP
     }
 
 exit:

diff --git a/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.cpp b/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.cpp
@@ -298,15 +298,16 @@ CHIP_ERROR GenericPlatformManagerImpl_POSIX<ImplClass>::_StopEventLoopTask()
     }
 
 exit:
-    pthread_mutex_destroy(&mStateLock);
-    pthread_cond_destroy(&mEventQueueStoppedCond);
     mHasValidChipTask = false;
     return System::MapErrorPOSIX(err);
 }
 
 template <class ImplClass>
 CHIP_ERROR GenericPlatformManagerImpl_POSIX<ImplClass>::_Shutdown()
 {
+    pthread_mutex_destroy(&mStateLock);
+    pthread_cond_destroy(&mEventQueueStoppedCond);
+
     //
     // Call up to the base class _Shutdown() to perform the actual stack de-initialization
     // and clean-up