From 7805664ee3705ec8abe076eebe49450ae0ccf19d Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Thu, 28 Nov 2024 13:03:32 +0100 Subject: [PATCH] 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 --- examples/platform/linux/AppMain.cpp | 48 ++++++++++++---- examples/tv-casting-app/linux/main.cpp | 59 +++++++++++++++++++- examples/tv-casting-app/linux/simple-app.cpp | 57 ++++++++++++++++++- scripts/tests/run_tv_casting_test.py | 17 +----- src/lib/shell/Engine.h | 8 +++ src/lib/shell/MainLoopAmeba.cpp | 2 +- src/lib/shell/MainLoopDefault.cpp | 21 +++++-- src/lib/shell/MainLoopESP32.cpp | 2 +- src/lib/shell/MainLoopMW320.cpp | 2 +- src/lib/shell/MainLoopSilabs.cpp | 2 +- 10 files changed, 178 insertions(+), 40 deletions(-) diff --git a/examples/platform/linux/AppMain.cpp b/examples/platform/linux/AppMain.cpp index 65763dc12d3475..b9c0cdf771b772 100644 --- 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 index 3a41f15d531c30..11e1c56e9f0d89 100644 --- 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 + #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 index 5526d6a3f6df07..a0863fa9e4e35b 100644 --- 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 + #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 index 7b530c7a4a1780..fb02b2ad2f97c7 100755 --- 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 index a0d7cfa6fa71fe..6307384db60a76 100644 --- 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 index b8476a5dd9a9f0..4f5ea91beca656 100644 --- 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) diff --git a/src/lib/shell/MainLoopDefault.cpp b/src/lib/shell/MainLoopDefault.cpp index dba76397927f2c..6a3a6b300b53f3 100644 --- a/src/lib/shell/MainLoopDefault.cpp +++ b/src/lib/shell/MainLoopDefault.cpp @@ -15,13 +15,15 @@ * limitations under the License. */ -#include "streamer.h" +#include +#include +#include + #include #include #include -#include -#include +#include "streamer.h" using chip::FormatCHIPError; using chip::Platform::MemoryAlloc; @@ -46,11 +48,18 @@ size_t ReadLine(char * buffer, size_t max) break; } - if (streamer_read(streamer_get(), buffer + line_sz, 1) != 1) + auto ret = streamer_read(streamer_get(), buffer + line_sz, 1); + if (ret == -1 && errno == EINTR) { - continue; + streamer_printf(streamer_get(), "^C\r\n"); + // In case of EINTR (Ctrl-C) reset the buffer and start over. + buffer[line_sz = 0] = '\0'; + break; } + if (ret != 1) + continue; + // Process character we just read. switch (buffer[line_sz]) { @@ -188,7 +197,7 @@ void Engine::RunMainLoop() { streamer_printf(streamer_get(), CHIP_SHELL_PROMPT); - while (true) + while (mRunning) { char * line = static_cast(Platform::MemoryAlloc(CHIP_SHELL_MAX_LINE_SIZE)); if (ReadLine(line, CHIP_SHELL_MAX_LINE_SIZE) == 0u) diff --git a/src/lib/shell/MainLoopESP32.cpp b/src/lib/shell/MainLoopESP32.cpp index aecca35774a07e..1c36a5ccf4fe81 100644 --- a/src/lib/shell/MainLoopESP32.cpp +++ b/src/lib/shell/MainLoopESP32.cpp @@ -70,7 +70,7 @@ namespace Shell { void Engine::RunMainLoop() { - while (true) + while (mRunning) { const char * prompt = LOG_COLOR_I "> " LOG_RESET_COLOR; char * line = linenoise(prompt); diff --git a/src/lib/shell/MainLoopMW320.cpp b/src/lib/shell/MainLoopMW320.cpp index ea7ca5de0a70df..833c87a3bc5e7b 100644 --- a/src/lib/shell/MainLoopMW320.cpp +++ b/src/lib/shell/MainLoopMW320.cpp @@ -273,7 +273,7 @@ void Engine::RunMainLoop() Engine::Root().RegisterDefaultCommands(); RegisterMetaCommands(); - while (true) + while (mRunning) { streamer_printf(streamer_get(), CHIP_SHELL_PROMPT); diff --git a/src/lib/shell/MainLoopSilabs.cpp b/src/lib/shell/MainLoopSilabs.cpp index 8613c48b552f6f..63c9d0ad6f2005 100644 --- a/src/lib/shell/MainLoopSilabs.cpp +++ b/src/lib/shell/MainLoopSilabs.cpp @@ -218,7 +218,7 @@ void Engine::RunMainLoop() { streamer_printf(streamer_get(), kShellPrompt); - while (true) + while (mRunning) { char * line = static_cast(Platform::MemoryAlloc(CHIP_SHELL_MAX_LINE_SIZE)); ReadLine(line, CHIP_SHELL_MAX_LINE_SIZE);