Deliver SIGINT to the shell thread to interrupt read() (#36533)

* Deliver SIGINT to the sell thread to interrupt read() * Verify that TV apps exit cleanly * TV casting app with proper shutdown on SIGTERM * Fix import * Revert sigaction() usage, as it seems not to work on Darwin * Remove ifdefs * Use signal() instead of sigaction() on Darwin * Restyled by clang-format --------- Co-authored-by: Restyled.io <[email protected]>
project-chip · Nov 28, 2024 · 7805664 · 7805664
1 parent 70f2f3e
commit 7805664
Show file tree

Hide file tree

Showing 10 changed files with 178 additions and 40 deletions.
diff --git a/examples/platform/linux/AppMain.cpp b/examples/platform/linux/AppMain.cpp
@@ -305,7 +305,7 @@ void StopMainEventLoop()
     else
     {
         Server::GetInstance().GenerateShutDownEvent();
-        PlatformMgr().ScheduleWork([](intptr_t) { PlatformMgr().StopEventLoopTask(); });
+        SystemLayer().ScheduleLambda([]() { PlatformMgr().StopEventLoopTask(); });
     }
 }
 
@@ -318,18 +318,13 @@ void Cleanup()
     // TODO(16968): Lifecycle management of storage-using components like GroupDataProvider, etc
 }
 
-// TODO(#20664) REPL test will fail if signal SIGINT is not caught, temporarily keep following logic.
-
-// when the shell is enabled, don't intercept signals since it prevents the user from
-// using expected commands like CTRL-C to quit the application. (see issue #17845)
-// We should stop using signals for those faults, and move to a different notification
-// means, like a pipe. (see issue #19114)
-#if !defined(ENABLE_CHIP_SHELL)
 void StopSignalHandler(int /* signal */)
 {
+#if defined(ENABLE_CHIP_SHELL)
+    Engine::Root().StopMainLoop();
+#endif
     StopMainEventLoop();
 }
-#endif // !defined(ENABLE_CHIP_SHELL)
 
 } // namespace
 
@@ -409,6 +404,18 @@ int ChipLinuxAppInit(int argc, char * const argv[], OptionSet * customOptions,
     SuccessOrExit(err);
 #endif
 
+#if defined(ENABLE_CHIP_SHELL)
+    /* Block SIGINT and SIGTERM. Other threads created by the main thread
+     * will inherit the signal mask. Then we can explicitly unblock signals
+     * in the shell thread to handle them, so the read(stdin) call can be
+     * interrupted by a signal. */
+    sigset_t set;
+    sigemptyset(&set);
+    sigaddset(&set, SIGINT);
+    sigaddset(&set, SIGTERM);
+    pthread_sigmask(SIG_BLOCK, &set, nullptr);
+#endif
+
     err = DeviceLayer::PlatformMgr().InitChipStack();
     SuccessOrExit(err);
 
@@ -531,11 +538,18 @@ void ChipLinuxAppMainLoop(AppMainLoopImplementation * impl)
 
 #if defined(ENABLE_CHIP_SHELL)
     Engine::Root().Init();
+    Shell::RegisterCommissioneeCommands();
     std::thread shellThread([]() {
+        sigset_t set;
+        sigemptyset(&set);
+        sigaddset(&set, SIGINT);
+        sigaddset(&set, SIGTERM);
+        // Unblock SIGINT and SIGTERM, so that the shell thread can handle
+        // them - we need read() call to be interrupted.
+        pthread_sigmask(SIG_UNBLOCK, &set, nullptr);
         Engine::Root().RunMainLoop();
         StopMainEventLoop();
     });
-    Shell::RegisterCommissioneeCommands();
 #endif
     initParams.operationalServicePort        = CHIP_PORT;
     initParams.userDirectedCommissioningPort = CHIP_UDC_PORT;
@@ -670,12 +684,22 @@ void ChipLinuxAppMainLoop(AppMainLoopImplementation * impl)
 
     ApplicationInit();
 
-#if !defined(ENABLE_CHIP_SHELL)
+    // NOTE: For some reason, on Darwin, the signal handler is not called if the signal is
+    //       registered with sigaction() call and TSAN is enabled. The problem seems to be
+    //       related with the dispatch_semaphore_wait() function in the RunEventLoop() method.
+    //       If this call is commented out, the signal handler is called as expected...
+#if defined(__APPLE__)
     // NOLINTBEGIN(bugprone-signal-handler)
     signal(SIGINT, StopSignalHandler);
     signal(SIGTERM, StopSignalHandler);
     // NOLINTEND(bugprone-signal-handler)
-#endif // !defined(ENABLE_CHIP_SHELL)
+#else
+    struct sigaction sa                        = {};
+    sa.sa_handler                              = StopSignalHandler;
+    sa.sa_flags                                = SA_RESETHAND;
+    sigaction(SIGINT, &sa, nullptr);
+    sigaction(SIGTERM, &sa, nullptr);
+#endif
 
     if (impl != nullptr)
     {

diff --git a/examples/tv-casting-app/linux/main.cpp b/examples/tv-casting-app/linux/main.cpp
@@ -16,6 +16,8 @@
  *    limitations under the License.
  */
 
+#include <signal.h>
+
 #include "commands/clusters/SubscriptionsCommands.h"
 #include "commands/common/Commands.h"
 #include "commands/example/ExampleCredentialIssuerCommands.h"
@@ -105,17 +107,56 @@ CHIP_ERROR ProcessClusterCommand(int argc, char ** argv)
     return CHIP_NO_ERROR;
 }
 
+void StopMainEventLoop()
+{
+    Server::GetInstance().GenerateShutDownEvent();
+    DeviceLayer::SystemLayer().ScheduleLambda([]() { DeviceLayer::PlatformMgr().StopEventLoopTask(); });
+}
+
+void StopSignalHandler(int /* signal */)
+{
+#if defined(ENABLE_CHIP_SHELL)
+    Engine::Root().StopMainLoop();
+#endif
+    StopMainEventLoop();
+}
+
 int main(int argc, char * argv[])
 {
-    ChipLogProgress(AppServer, "chip_casting_simplified = 0"); // this file is built/run only if chip_casting_simplified = 0
+    // This file is built/run only if chip_casting_simplified = 0
+    ChipLogProgress(AppServer, "chip_casting_simplified = 0");
+
+#if defined(ENABLE_CHIP_SHELL)
+    /* Block SIGINT and SIGTERM. Other threads created by the main thread
+     * will inherit the signal mask. Then we can explicitly unblock signals
+     * in the shell thread to handle them, so the read(stdin) call can be
+     * interrupted by a signal. */
+    sigset_t set;
+    sigemptyset(&set);
+    sigaddset(&set, SIGINT);
+    sigaddset(&set, SIGTERM);
+    pthread_sigmask(SIG_BLOCK, &set, nullptr);
+#endif
+
     VerifyOrDie(CHIP_NO_ERROR == chip::Platform::MemoryInit());
     VerifyOrDie(CHIP_NO_ERROR == chip::DeviceLayer::PlatformMgr().InitChipStack());
 
 #if defined(ENABLE_CHIP_SHELL)
     Engine::Root().Init();
-    std::thread shellThread([]() { Engine::Root().RunMainLoop(); });
     Shell::RegisterCastingCommands();
+    std::thread shellThread([]() {
+        sigset_t set_;
+        sigemptyset(&set_);
+        sigaddset(&set_, SIGINT);
+        sigaddset(&set_, SIGTERM);
+        // Unblock SIGINT and SIGTERM, so that the shell thread can handle
+        // them - we need read() call to be interrupted.
+        pthread_sigmask(SIG_UNBLOCK, &set_, nullptr);
+        Engine::Root().RunMainLoop();
+        StopMainEventLoop();
+    });
 #endif
+
     CHIP_ERROR err = CHIP_NO_ERROR;
 
     DeviceLayer::PersistedStorage::KeyValueStoreMgrImpl().Init(CHIP_CONFIG_KVS_PATH);
@@ -172,11 +213,25 @@ int main(int argc, char * argv[])
         ProcessClusterCommand(argc, argv);
     }
 
+    {
+        struct sigaction sa = {};
+        sa.sa_handler       = StopSignalHandler;
+        sa.sa_flags         = SA_RESETHAND;
+        sigaction(SIGINT, &sa, nullptr);
+        sigaction(SIGTERM, &sa, nullptr);
+    }
+
     DeviceLayer::PlatformMgr().RunEventLoop();
+
 exit:
+
 #if defined(ENABLE_CHIP_SHELL)
     shellThread.join();
 #endif
+
+    chip::Server::GetInstance().Shutdown();
+    chip::DeviceLayer::PlatformMgr().Shutdown();
+
     if (err != CHIP_NO_ERROR)
     {
         ChipLogError(AppServer, "Failed to run TV Casting App: %s", ErrorStr(err));

diff --git a/examples/tv-casting-app/linux/simple-app.cpp b/examples/tv-casting-app/linux/simple-app.cpp
@@ -16,6 +16,8 @@
  *    limitations under the License.
  */
 
+#include <signal.h>
+
 #include "simple-app-helper.h"
 
 #include "core/CastingPlayer.h"
@@ -89,9 +91,37 @@ class CommonCaseDeviceServerInitParamsProvider : public ServerInitParamsProvider
     }
 };
 
+void StopMainEventLoop()
+{
+    chip::Server::GetInstance().GenerateShutDownEvent();
+    chip::DeviceLayer::SystemLayer().ScheduleLambda([]() { chip::DeviceLayer::PlatformMgr().StopEventLoopTask(); });
+}
+
+void StopSignalHandler(int /* signal */)
+{
+#if defined(ENABLE_CHIP_SHELL)
+    chip::Shell::Engine::Root().StopMainLoop();
+#endif
+    StopMainEventLoop();
+}
+
 int main(int argc, char * argv[])
 {
-    ChipLogProgress(AppServer, "chip_casting_simplified = 1"); // this file is built/run only if chip_casting_simplified = 1
+    // This file is built/run only if chip_casting_simplified = 1
+    ChipLogProgress(AppServer, "chip_casting_simplified = 1");
+
+#if defined(ENABLE_CHIP_SHELL)
+    /* Block SIGINT and SIGTERM. Other threads created by the main thread
+     * will inherit the signal mask. Then we can explicitly unblock signals
+     * in the shell thread to handle them, so the read(stdin) call can be
+     * interrupted by a signal. */
+    sigset_t set;
+    sigemptyset(&set);
+    sigaddset(&set, SIGINT);
+    sigaddset(&set, SIGTERM);
+    pthread_sigmask(SIG_BLOCK, &set, nullptr);
+#endif
+
     // Create AppParameters that need to be passed to CastingApp.Initialize()
     AppParameters appParameters;
     RotatingDeviceIdUniqueIdProvider rotatingDeviceIdUniqueIdProvider;
@@ -122,8 +152,18 @@ int main(int argc, char * argv[])
 
 #if defined(ENABLE_CHIP_SHELL)
     chip::Shell::Engine::Root().Init();
-    std::thread shellThread([]() { chip::Shell::Engine::Root().RunMainLoop(); });
     RegisterCommands();
+    std::thread shellThread([]() {
+        sigset_t set_;
+        sigemptyset(&set_);
+        sigaddset(&set_, SIGINT);
+        sigaddset(&set_, SIGTERM);
+        // Unblock SIGINT and SIGTERM, so that the shell thread can handle
+        // them - we need read() call to be interrupted.
+        pthread_sigmask(SIG_UNBLOCK, &set_, nullptr);
+        chip::Shell::Engine::Root().RunMainLoop();
+        StopMainEventLoop();
+    });
 #endif
 
     CastingPlayerDiscovery::GetInstance()->SetDelegate(DiscoveryDelegateImpl::GetInstance());
@@ -133,7 +173,20 @@ int main(int argc, char * argv[])
     VerifyOrReturnValue(err == CHIP_NO_ERROR, -1,
                         ChipLogError(AppServer, "CastingPlayerDiscovery::StartDiscovery failed %" CHIP_ERROR_FORMAT, err.Format()));
 
+    struct sigaction sa = {};
+    sa.sa_handler       = StopSignalHandler;
+    sa.sa_flags         = SA_RESETHAND;
+    sigaction(SIGINT, &sa, nullptr);
+    sigaction(SIGTERM, &sa, nullptr);
+
     chip::DeviceLayer::PlatformMgr().RunEventLoop();
 
+#if defined(ENABLE_CHIP_SHELL)
+    shellThread.join();
+#endif
+
+    chip::Server::GetInstance().Shutdown();
+    chip::DeviceLayer::PlatformMgr().Shutdown();
+
     return 0;
 }
diff --git a/scripts/tests/run_tv_casting_test.py b/scripts/tests/run_tv_casting_test.py
@@ -17,7 +17,6 @@
 import glob
 import logging
 import os
-import signal
 import sys
 import tempfile
 import time
@@ -32,7 +31,7 @@
 """
 This script can be used to validate the casting experience between the Linux tv-casting-app and the Linux tv-app.
 
-It runs a series of test sequences that check for expected output lines from the tv-casting-app and the tv-app in 
+It runs a series of test sequences that check for expected output lines from the tv-casting-app and the tv-app in
 a deterministic order. If these lines are not found, it indicates an issue with the casting experience.
 """
 
@@ -92,24 +91,14 @@ def stop_app(test_sequence_name: str, app_name: str, app: ProcessOutputCapture):
             None,
         )
 
-    if app_exit_code >= 0:
+    if app_exit_code != 0:
         raise TestStepException(
             f"{test_sequence_name}: {app_name} exited with unexpected exit code {app_exit_code}.",
             test_sequence_name,
             None,
         )
 
-    signal_number = -app_exit_code
-    if signal_number != signal.SIGTERM.value:
-        raise TestStepException(
-            f"{test_sequence_name}: {app_name} stopped by signal {signal_number} instead of {signal.SIGTERM.value} (SIGTERM).",
-            test_sequence_name,
-            None,
-        )
-
-    logging.info(
-        f"{test_sequence_name}: {app_name} stopped by {signal_number} (SIGTERM) signal."
-    )
+    logging.info(f"{test_sequence_name}: {app_name} stopped.")
 
 
 def parse_output_msg_in_subprocess(

diff --git a/src/lib/shell/Engine.h b/src/lib/shell/Engine.h
@@ -119,6 +119,13 @@ class Engine
      */
     void RunMainLoop();
 
+    /**
+     * Stop the shell mainloop on the next iteration.
+     *
+     * @note This method can be called from a signal handler to stop the main loop.
+     */
+    void StopMainLoop() { mRunning = false; }
+
     /**
      * Initialize the Shell::Engine.
      *
@@ -130,6 +137,7 @@ class Engine
 
 private:
     static void ProcessShellLineTask(intptr_t context);
+    bool mRunning = true;
 };
 
 } // namespace Shell

diff --git a/src/lib/shell/MainLoopAmeba.cpp b/src/lib/shell/MainLoopAmeba.cpp
@@ -135,7 +135,7 @@ namespace Shell {
 void Engine::RunMainLoop()
 {
     char line[CHIP_SHELL_MAX_LINE_SIZE];
-    while (true)
+    while (mRunning)
     {
         memset(line, 0, CHIP_SHELL_MAX_LINE_SIZE);
         if (ReadLine(line, CHIP_SHELL_MAX_LINE_SIZE) > 0u)