From a8e9fc711b44e3f084e316d39724a2011046d799 Mon Sep 17 00:00:00 2001 From: Pawel Pyszko Date: Thu, 2 Jul 2026 11:28:31 +0200 Subject: [PATCH 01/22] mw::log: Add tracking ticket to missing transitive dependencies GIT_ORIGIN_SPP_REV_ID: 243bfaba861fbab5b7a7ea9b6af0bba5b4cdadda --- score/datarouter/BUILD | 32 ++++++++----------- .../src/daemon/persistentlogging_config.cpp | 3 +- .../src/unix_domain/unix_domain_common.cpp | 5 --- .../test/ut/ut_logging/test_logparser.cpp | 6 ---- .../test/utils/data_router_test_utils.h | 2 +- .../detail/common/helper_functions_test.cpp | 2 ++ .../data_router_message_client_impl.cpp | 1 - 7 files changed, 17 insertions(+), 34 deletions(-) diff --git a/score/datarouter/BUILD b/score/datarouter/BUILD index 8747689d..38ee1e1f 100644 --- a/score/datarouter/BUILD +++ b/score/datarouter/BUILD @@ -208,8 +208,8 @@ cc_library( ":datarouter_types", ":dltprotocol", ":logparser_interface", + "//score/mw/log", "//score/mw/log/detail/data_router/shared_memory:reader", - "@score_baselibs//score/mw/log", "@score_baselibs//score/mw/log/configuration:nvconfig", "@score_baselibs//score/static_reflection_with_serialization/serialization", ], @@ -383,7 +383,7 @@ cc_library( strip_include_prefix = "src/applications", visibility = ["//score/datarouter/test:__subpackages__"], deps = [ - "@score_baselibs//score/mw/log", + "//score/mw/log", ], ) @@ -400,7 +400,7 @@ cc_library( "@score_logging//score/datarouter:__subpackages__", ], deps = [ - "@score_baselibs//score/mw/log", + "//score/mw/log", ], ) @@ -420,10 +420,10 @@ cc_library( ":logparser_factory_interface", ":message_passing_server", ":unixdomain_server", + "//score/mw/log", "//score/mw/log/detail/data_router/shared_memory:reader", "@score_baselibs//score/concurrency:synchronized", "@score_baselibs//score/language/futurecpp", - "@score_baselibs//score/mw/log", ], ) @@ -444,8 +444,8 @@ cc_library( ":logparser_factory_interface", ":message_passing_server", ":unixdomain_mock", + "//score/mw/log", "@score_baselibs//score/concurrency:synchronized", - "@score_baselibs//score/mw/log", ], ) @@ -467,8 +467,8 @@ cc_library( ":logparser_testing", ":message_passing_server", ":unixdomain_mock", + "//score/mw/log", "@score_baselibs//score/concurrency:synchronized", - "@score_baselibs//score/mw/log", ], ) @@ -627,8 +627,8 @@ cc_library( ":unixdomain_server", "//score/datarouter/network:vlan", "//score/datarouter/src/persistent_logging/persistent_logging_stub:sysedr_stub", + "//score/mw/log", "@score_baselibs//score/language/futurecpp", - "@score_baselibs//score/mw/log", "@score_baselibs//score/mw/log/configuration:nvconfig", "@score_baselibs//score/os:socket", "@score_baselibs//score/os:stat", @@ -656,7 +656,7 @@ cc_library( ], deps = [ ":dltserver_common", - "@score_baselibs//score/mw/log", + "//score/mw/log", "@score_baselibs//score/mw/log/configuration:nvconfig", "@score_baselibs//score/os:socket", "@score_baselibs//score/os:stat", @@ -716,8 +716,8 @@ cc_library( ":logchannel_utility", ":socketserver_config_helpers", "//score/datarouter/src/persistency:interface", + "//score/mw/log", "@rapidjson", - "@score_baselibs//score/mw/log", ], ) @@ -866,7 +866,7 @@ cc_library( visibility = ["//score/datarouter/test:__subpackages__"], deps = [ ":datarouter_options", - "@score_baselibs//score/mw/log", + "//score/mw/log", ] + deps, ) for (name, deps, test_only) in [ @@ -901,15 +901,9 @@ cc_binary( "@score_baselibs//score/os:errno_logging", "@score_baselibs//score/os:pthread", "@score_baselibs//score/os/utils:path", - "@score_baselibs//score/mw/log:console", - "@score_baselibs//score/mw/log:frontend", - "//score/mw/log/backend:file", - "//score/mw/log/backend:remote", + "//score/mw/log", # "//third_party/jemalloc", # Ticket-231781 - ] + select({ - "@platforms//os:qnx": ["//score/mw/log/backend:slog"], - "//conditions:default": [], - }), + ], ) cc_library( @@ -1109,7 +1103,7 @@ cc_library( "@score_baselibs//score/os:stdio", "@score_baselibs//score/os/utils:path", "@score_baselibs//score/os/utils:thread", - "@score_baselibs//score/mw/log", + "//score/mw/log", "@score_baselibs//score/mw/log/configuration:nvconfig", "//score/datarouter/dlt_filetransfer_trigger_lib:filetransfer_message_types", "//score/datarouter/src/persistent_logging/persistent_logging_stub:sysedr_stub", diff --git a/score/datarouter/src/daemon/persistentlogging_config.cpp b/score/datarouter/src/daemon/persistentlogging_config.cpp index 5a585c1f..397656e3 100644 --- a/score/datarouter/src/daemon/persistentlogging_config.cpp +++ b/score/datarouter/src/daemon/persistentlogging_config.cpp @@ -39,10 +39,9 @@ const std::string kDefaultPersistentLoggingJsonFilepath = "etc/persistent-loggin PersistentLoggingConfig ReadPersistentLoggingConfig(const std::string& file_path) { using ReadResult = PersistentLoggingConfig::ReadResult; - using FileCloseFn = int(*)(std::FILE*); PersistentLoggingConfig config; - using UniqueFileT = std::unique_ptr; + using UniqueFileT = std::unique_ptr; UniqueFileT fp(std::fopen(file_path.c_str(), "r"), &fclose); if (nullptr == fp) { diff --git a/score/datarouter/src/unix_domain/unix_domain_common.cpp b/score/datarouter/src/unix_domain/unix_domain_common.cpp index 5785a913..8771585a 100644 --- a/score/datarouter/src/unix_domain/unix_domain_common.cpp +++ b/score/datarouter/src/unix_domain/unix_domain_common.cpp @@ -240,12 +240,7 @@ void SendSocketMessage(std::int32_t connection_file_descriptor, std::int32_t* fdptr = CmsgDataSuppressWarning(cmsg); if (num_fds > 0) { - // Disable maybe uninitialized warning for optional. - // Initialization is already checked using `has_value`. - DISABLE_WARNING_PUSH - DISABLE_WARNING_MAYBE_UNINITIALIZED fdptr[0] = file_handle.value(); - DISABLE_WARNING_POP } #endif // USE_SECURE_FILE_HANDLE_IPC } diff --git a/score/datarouter/test/ut/ut_logging/test_logparser.cpp b/score/datarouter/test/ut/ut_logging/test_logparser.cpp index b752e900..b2f51966 100644 --- a/score/datarouter/test/ut/ut_logging/test_logparser.cpp +++ b/score/datarouter/test/ut/ut_logging/test_logparser.cpp @@ -114,12 +114,6 @@ TEST(LogParserTest, SingleMessageHandler) TEST(LogParserTest, FilterForwarderWithSingleForwarder) { - RecordProperty("PartiallyVerifies", "comp_req__data_router__dlt_verbose_messages"); - RecordProperty("ASIL", "B"); - RecordProperty("Description", "Verifies that the log parser correctly forwards a single message through the filter forwarder"); - RecordProperty("TestType", "requirements-based"); - RecordProperty("DerivationTechnique", "requirements-analysis"); - using namespace std::chrono_literals; const std::string type_params = MakeTypeParams(DltidT{"ECU4"}, DltidT{"APP0"}); diff --git a/score/datarouter/test/utils/data_router_test_utils.h b/score/datarouter/test/utils/data_router_test_utils.h index 02a5ac1e..faabd541 100644 --- a/score/datarouter/test/utils/data_router_test_utils.h +++ b/score/datarouter/test/utils/data_router_test_utils.h @@ -16,7 +16,7 @@ #include "score/datarouter/datarouter/data_router.h" -#include "score/static_reflection_with_serialization/serialization/include/serialization/for_logging.h" +#include "common/serialization/include/serialization/for_logging.h" namespace test::utils { diff --git a/score/mw/log/detail/common/helper_functions_test.cpp b/score/mw/log/detail/common/helper_functions_test.cpp index afa9398d..c5c761ac 100644 --- a/score/mw/log/detail/common/helper_functions_test.cpp +++ b/score/mw/log/detail/common/helper_functions_test.cpp @@ -11,6 +11,7 @@ * SPDX-License-Identifier: Apache-2.0 ********************************************************************************/ #include "score/mw/log/detail/common/helper_functions.h" +#include #include "gtest/gtest.h" @@ -19,6 +20,7 @@ namespace helper using testing::Types; TEST(SumTest, SumNoOverflow) { + std::numeric_limits::max(); const std::size_t expected_value = 6U; std::size_t result = Sum(1U, 2U, 3U); EXPECT_EQ(result, expected_value); diff --git a/score/mw/log/detail/data_router/data_router_message_client_impl.cpp b/score/mw/log/detail/data_router/data_router_message_client_impl.cpp index 26fa28f9..e98491d9 100644 --- a/score/mw/log/detail/data_router/data_router_message_client_impl.cpp +++ b/score/mw/log/detail/data_router/data_router_message_client_impl.cpp @@ -201,7 +201,6 @@ bool DatarouterMessageClientImpl::StartReceiver() auto* this_ptr = this; auto connect_callback = [this_ptr](score::message_passing::IServerConnection& connection) noexcept -> std::uintptr_t { const auto result = SignalHandling::PThreadBlockSigTerm(this_ptr->utils_.GetSignal()); - static_cast(result); const pid_t client_pid = connection.GetClientIdentity().pid; return static_cast(client_pid); }; From c663bff7cc74798b26099091031568666f7004e1 Mon Sep 17 00:00:00 2001 From: Jianjun Wen Date: Mon, 25 May 2026 06:19:33 +0200 Subject: [PATCH 02/22] improve design around IsOutputEnabled method GIT_ORIGIN_SPP_REV_ID: 1de1429b675ff1facae779b5e9784ce264f49be9 --- .../include/daemon/dlt_log_server.h | 13 +++------- .../include/daemon/i_dlt_log_server.h | 4 +++ .../datarouter/include/logparser/logparser.h | 22 +++++++++++++--- .../mocks/daemon/dlt_log_server_mock.h | 1 + .../datarouter/src/daemon/dlt_log_server.cpp | 16 ++++++------ score/datarouter/src/daemon/socketserver.cpp | 21 +++++++++++----- score/datarouter/src/logparser/logparser.cpp | 25 +++++++++++-------- .../test/ut/ut_logging/test_dltserver.cpp | 18 ++++++------- .../test_file_transfer_handler_factory.cpp | 8 +++--- .../test/ut/ut_logging/test_logparser.cpp | 1 + 10 files changed, 78 insertions(+), 51 deletions(-) diff --git a/score/datarouter/include/daemon/dlt_log_server.h b/score/datarouter/include/daemon/dlt_log_server.h index 598121ab..2f0dd6c7 100644 --- a/score/datarouter/include/daemon/dlt_log_server.h +++ b/score/datarouter/include/daemon/dlt_log_server.h @@ -119,11 +119,6 @@ class DltLogServer : score::platform::datarouter::DltNonverboseHandlerType::IOut enabled_callback_ = enabled_callback; } - void SetDltOutputEnabled(bool enabled) - { - dlt_output_enabled_.store(enabled, std::memory_order_release); - } - void Flush() { for (auto& channel : channels_) @@ -170,8 +165,6 @@ class DltLogServer : score::platform::datarouter::DltNonverboseHandlerType::IOut } // LCOV_EXCL_STOP - bool GetDltEnabled() const noexcept; - std::string ReadLogChannelNames() const override; std::string ResetToDefault() override; std::string StoreDltConfig() override; @@ -187,6 +180,10 @@ class DltLogServer : score::platform::datarouter::DltNonverboseHandlerType::IOut AssignmentAction assignment_flag) override; std::string SetDltOutputEnable(bool enable) override; + /// Returns the current output-enable state. + /// Thread-safe: reads std::atomic with acquire ordering. + bool IsOutputEnabled() const noexcept override final; + // This is used for test purpose only in google tests, to have an access to the private members. // Do not use this calls in implementation except unit tests. class DltLogServerTest; @@ -311,8 +308,6 @@ class DltLogServer : score::platform::datarouter::DltNonverboseHandlerType::IOut std::unique_ptr sysedr_handler_; - bool IsOutputEnabled() const noexcept override final; - void SendNonVerbose(const score::mw::log::config::NvMsgDescriptor& desc, uint32_t tmsp, const void* data, diff --git a/score/datarouter/include/daemon/i_dlt_log_server.h b/score/datarouter/include/daemon/i_dlt_log_server.h index fdcdd1cb..e9ea3f9f 100644 --- a/score/datarouter/include/daemon/i_dlt_log_server.h +++ b/score/datarouter/include/daemon/i_dlt_log_server.h @@ -60,6 +60,10 @@ class IDltLogServer AssignmentAction assignment_flag) = 0; virtual std::string SetDltOutputEnable(bool enable) = 0; + /// Returns the current output-enable state. + /// Thread-safe: backed by std::atomic in the concrete implementation. + virtual bool IsOutputEnabled() const noexcept = 0; + virtual ~IDltLogServer() = default; }; diff --git a/score/datarouter/include/logparser/logparser.h b/score/datarouter/include/logparser/logparser.h index af1ed2b9..a5004979 100644 --- a/score/datarouter/include/logparser/logparser.h +++ b/score/datarouter/include/logparser/logparser.h @@ -16,6 +16,7 @@ #include "logparser/i_logparser.h" +#include #include #include #include @@ -58,8 +59,19 @@ class LogParser : public ILogParser public: using HandleRequestMap = std::unordered_multimap; + /// Maximum number of TypeHandlers that can be bound to a single type name. + /// DltLogServer registers exactly 1 handler per type (vhandler_, fthandler_, + /// sysedr_handler_). Increase this constant if a second handler per type is added. + static constexpr std::size_t kMaxHandlersPerType{1U}; + + /// Maximum number of global (AnyHandler) handlers. + /// DltLogServer registers exactly 2: sysedr_handler_ + nvhandler_. + /// Increase this constant if a new global handler is added. + static constexpr std::size_t kMaxGlobalHandlers{2U}; + explicit LogParser(const score::mw::log::INvConfig& nv_config, - std::vector global_handlers = {}, + std::array global_handlers = {}, + std::size_t global_handler_count = 0U, HandleRequestMap handle_request_map = {}); ~LogParser() = default; @@ -75,21 +87,23 @@ class LogParser : public ILogParser public: TypeInfo info; - explicit IndexParser(TypeInfo type_info) : info{type_info}, handlers_{} {} + explicit IndexParser(TypeInfo type_info) : info{type_info}, handlers_{}, handler_count_{0U} {} void AddHandler(TypeHandler* handler); void Parse(const TimestampT timestamp, const char* const data, const BufsizeT size); private: - std::vector handlers_; + std::array handlers_; + std::size_t handler_count_; }; const HandleRequestMap handle_request_map_; std::unordered_map index_parser_map_; - const std::vector global_handlers_; + const std::array global_handlers_; + const std::size_t global_handler_count_; const score::mw::log::INvConfig& nv_config_; }; diff --git a/score/datarouter/mocks/daemon/dlt_log_server_mock.h b/score/datarouter/mocks/daemon/dlt_log_server_mock.h index f03bca53..da509f13 100644 --- a/score/datarouter/mocks/daemon/dlt_log_server_mock.h +++ b/score/datarouter/mocks/daemon/dlt_log_server_mock.h @@ -44,6 +44,7 @@ class DltLogServerMock : public IDltLogServer (score::platform::DltidT, score::platform::DltidT, score::platform::DltidT, AssignmentAction), (override)); MOCK_METHOD(std::string, SetDltOutputEnable, (bool), (override)); + MOCK_METHOD(bool, IsOutputEnabled, (), (const, noexcept, override)); }; } // namespace mock diff --git a/score/datarouter/src/daemon/dlt_log_server.cpp b/score/datarouter/src/daemon/dlt_log_server.cpp index 8f6b1b45..cd2d23d3 100644 --- a/score/datarouter/src/daemon/dlt_log_server.cpp +++ b/score/datarouter/src/daemon/dlt_log_server.cpp @@ -213,25 +213,23 @@ void DltLogServer::InitLogChannelsDefault(const bool reloading) void DltLogServer::SetOutputEnabled(const bool enabled) { - const bool update = (dlt_output_enabled_.load(std::memory_order_acquire) != enabled); - - if (update) + // compare_exchange_strong makes the load+conditional-store a single atomic + // operation, preventing the check-then-act race where two concurrent callers + // could both observe the old value and both fire enabled_callback_. + bool expected = !enabled; + if (dlt_output_enabled_.compare_exchange_strong( + expected, enabled, std::memory_order_acq_rel, std::memory_order_acquire)) { - dlt_output_enabled_.store(enabled, std::memory_order_release); if (enabled_callback_) { enabled_callback_(enabled); } } } -bool DltLogServer::GetDltEnabled() const noexcept -{ - return dlt_output_enabled_.load(std::memory_order_acquire); -} bool DltLogServer::IsOutputEnabled() const noexcept { - return GetDltEnabled(); + return dlt_output_enabled_.load(std::memory_order_acquire); } void DltLogServer::SaveDatabase() diff --git a/score/datarouter/src/daemon/socketserver.cpp b/score/datarouter/src/daemon/socketserver.cpp index b936338d..8dc6c606 100644 --- a/score/datarouter/src/daemon/socketserver.cpp +++ b/score/datarouter/src/daemon/socketserver.cpp @@ -169,14 +169,23 @@ std::unique_ptr SocketServer::Crea std::unique_ptr Create(const score::mw::log::NvConfig& nv_config) override { - auto global_handlers = dlt_server_.GetGlobalHandlers(); - score::platform::internal::LogParser::HandleRequestMap handle_request_map; + using LogParser = score::platform::internal::LogParser; + + const auto handler_vec = dlt_server_.GetGlobalHandlers(); + std::array global_handlers{}; + const std::size_t handler_count = std::min(handler_vec.size(), LogParser::kMaxGlobalHandlers); + for (std::size_t i = 0U; i < handler_count; ++i) + { + global_handlers[i] = handler_vec[i]; + } + + LogParser::HandleRequestMap handle_request_map; for (auto& binding : dlt_server_.GetTypeHandlerBindings()) { handle_request_map.emplace(std::move(binding.type_name), binding.handler); } - return std::make_unique( - nv_config, std::move(global_handlers), std::move(handle_request_map)); + return std::make_unique( + nv_config, global_handlers, handler_count, std::move(handle_request_map)); } private: @@ -226,7 +235,7 @@ std::function SocketServer::CreateEnableHandler(DataRouter& router, score::mw::log::LogWarn() << "Changing output enable to " << enable; WriteDltEnabled(enable, persistent_dictionary); router.ForEachSource(enable); - dlt_server.SetDltOutputEnabled(enable); + dlt_server.SetDltOutputEnable(enable); }; } @@ -294,7 +303,7 @@ std::unique_ptr SocketServer::CreateMessagePassi const auto fd = maybe_fd.value(); const auto quota = dlt_server.GetQuota(appid); const auto quota_enforcement_enabled = dlt_server.GetQuotaEnforcementEnabled(); - const bool is_dlt_enabled = dlt_server.GetDltEnabled(); + const bool is_dlt_enabled = dlt_server.IsOutputEnabled(); auto source_session = router.NewSourceSession( fd, appid, is_dlt_enabled, std::move(handle), quota, quota_enforcement_enabled, client_pid, nv_config); // The reason for banning is, because it's error-prone to use. One should use abstractions e.g. provided by diff --git a/score/datarouter/src/logparser/logparser.cpp b/score/datarouter/src/logparser/logparser.cpp index 7dd9cd88..3abb4ad0 100644 --- a/score/datarouter/src/logparser/logparser.cpp +++ b/score/datarouter/src/logparser/logparser.cpp @@ -60,26 +60,31 @@ namespace internal { LogParser::LogParser(const score::mw::log::INvConfig& nv_config, - std::vector global_handlers, + std::array global_handlers, + std::size_t global_handler_count, HandleRequestMap handle_request_map) : ILogParser(), handle_request_map_{std::move(handle_request_map)}, index_parser_map_{}, - global_handlers_{std::move(global_handlers)}, + global_handlers_{global_handlers}, + global_handler_count_{global_handler_count}, nv_config_(nv_config) { } void LogParser::IndexParser::AddHandler(TypeHandler* handler) { - handlers_.push_back(handler); + if (handler_count_ < handlers_.size()) + { + handlers_[handler_count_++] = handler; + } } void LogParser::IndexParser::Parse(const TimestampT timestamp, const char* const data, const BufsizeT size) { - for (const auto& handler : handlers_) + for (std::size_t i = 0U; i < handler_count_; ++i) { - handler->Handle(timestamp, data, size); + handlers_[i]->Handle(timestamp, data, size); } } @@ -147,9 +152,9 @@ void LogParser::Parse(TimestampT timestamp, const char* data, BufsizeT size) index_parser.Parse(timestamp, data, size); const auto& type_info = index_parser.info; - for (auto* const handler : global_handlers_) + for (std::size_t i = 0U; i < global_handler_count_; ++i) { - handler->Handle(type_info, timestamp, data, size); + global_handlers_[i]->Handle(type_info, timestamp, data, size); } } @@ -181,10 +186,10 @@ void LogParser::ParseSharedMemoryRecord(const score::mw::log::detail::SharedMemo index_parser.Parse(record.header.time_stamp, payload_ptr, payload_length_buf_size); - const auto& type_info = index_parser.info; - for (auto* const handler : global_handlers_) + const auto& type_info = index_parser_entry->second.info; + for (std::size_t i = 0U; i < global_handler_count_; ++i) { - handler->Handle(type_info, record.header.time_stamp, payload_ptr, payload_length_buf_size); + global_handlers_[i]->Handle(type_info, record.header.time_stamp, payload_ptr, payload_length_buf_size); } } diff --git a/score/datarouter/test/ut/ut_logging/test_dltserver.cpp b/score/datarouter/test/ut/ut_logging/test_dltserver.cpp index 7eab7882..5208bcc2 100644 --- a/score/datarouter/test/ut/ut_logging/test_dltserver.cpp +++ b/score/datarouter/test/ut/ut_logging/test_dltserver.cpp @@ -151,7 +151,7 @@ TEST_F(DltServerCreatedWithoutConfigFixture, WhenCreatedDefault) TEST_F(DltServerCreatedWithoutConfigFixture, WhenCreatedDefaultDltEnabledTrue) { DltLogServer dlt_server(s_config, read_callback.AsStdFunction(), write_callback.AsStdFunction(), true); - const auto dlt_enabled = dlt_server.GetDltEnabled(); + const auto dlt_enabled = dlt_server.IsOutputEnabled(); EXPECT_TRUE(dlt_enabled); } @@ -1006,7 +1006,7 @@ TEST_F(DltServerCreatedWithoutConfigFixture, SendFTVerboseAppIdNoCoreChannelExpe dlt_server.SendFtVerbose({}, score::mw::log::LogLevel::kVerbose, app_id, ctx_id, 0U, 100U); } -TEST_F(DltServerCreatedWithoutConfigFixture, SetDltOutputEnabledToTrueExpectDltOutputEnabledFlagTrue) +TEST_F(DltServerCreatedWithoutConfigFixture, SetDltOutputEnableToTrueExpectIsOutputEnabledTrue) { EXPECT_CALL(read_callback, Call()).Times(0); EXPECT_CALL(write_callback, Call(_)).Times(0); @@ -1014,8 +1014,8 @@ TEST_F(DltServerCreatedWithoutConfigFixture, SetDltOutputEnabledToTrueExpectDltO score::logging::dltserver::DltLogServer::DltLogServerTest dlt_server( s_config, read_callback.AsStdFunction(), write_callback.AsStdFunction(), true); - dlt_server.SetDltOutputEnabled(true); - EXPECT_TRUE(dlt_server.GetDltEnabled()); + dlt_server.SetDltOutputEnable(true); + EXPECT_TRUE(dlt_server.IsOutputEnabled()); } TEST_F(DltServerCreatedWithConfigFixture, SetLogChannelThresholdChannelMissingDirectCallReturnsError) @@ -1150,20 +1150,20 @@ TEST_F(DltServerCreatedWithConfigFixture, SetDltOutputEnableDirectCall) auto response = dlt_server.SetDltOutputEnable(true); EXPECT_EQ(response.size(), kCommandResponseSize); EXPECT_EQ(response[0], static_cast(config::kRetOk)); - EXPECT_TRUE(dlt_server.GetDltEnabled()); + EXPECT_TRUE(dlt_server.IsOutputEnabled()); // Test disabling output through public method response = dlt_server.SetDltOutputEnable(false); EXPECT_EQ(response.size(), kCommandResponseSize); EXPECT_EQ(response[0], static_cast(config::kRetOk)); - EXPECT_FALSE(dlt_server.GetDltEnabled()); + EXPECT_FALSE(dlt_server.IsOutputEnabled()); } TEST_F(DltServerCreatedWithConfigFixture, SetDltOutputEnableBehaviorBlocksAllSends) { // Prove that enabling/disabling output affects the observable server state. // Note: sendVerbose()/sendNonVerbose() are not gated by this flag in the current implementation; - // the flag controls the DLT output enable state exposed via GetDltEnabled(). + // the flag controls the DLT output enable state exposed via IsOutputEnabled(). EXPECT_CALL(read_callback, Call()).Times(1).WillOnce(Return(p_config)); EXPECT_CALL(write_callback, Call(_)).Times(0); @@ -1179,13 +1179,13 @@ TEST_F(DltServerCreatedWithConfigFixture, SetDltOutputEnableBehaviorBlocksAllSen const auto disable_resp = dlt_server.SetDltOutputEnable(false); EXPECT_EQ(disable_resp.size(), kCommandResponseSize); EXPECT_EQ(disable_resp[0], static_cast(config::kRetOk)); - EXPECT_FALSE(dlt_server.GetDltEnabled()); + EXPECT_FALSE(dlt_server.IsOutputEnabled()); // Re-enable output: sending should resume. const auto enable_resp = dlt_server.SetDltOutputEnable(true); EXPECT_EQ(enable_resp.size(), kCommandResponseSize); EXPECT_EQ(enable_resp[0], static_cast(config::kRetOk)); - EXPECT_TRUE(dlt_server.GetDltEnabled()); + EXPECT_TRUE(dlt_server.IsOutputEnabled()); // Basic sanity: calling sendVerbose still forwards to the log sender (2 channels). EXPECT_CALL(*log_sender_mock_raw_ptr, SendVerbose(_, _, _)).Times(2); diff --git a/score/datarouter/test/ut/ut_logging/test_file_transfer_handler_factory.cpp b/score/datarouter/test/ut/ut_logging/test_file_transfer_handler_factory.cpp index 40d34da3..93e34151 100644 --- a/score/datarouter/test/ut/ut_logging/test_file_transfer_handler_factory.cpp +++ b/score/datarouter/test/ut/ut_logging/test_file_transfer_handler_factory.cpp @@ -22,6 +22,7 @@ #include #include +#include #include namespace score @@ -51,15 +52,14 @@ TEST(FileTransferStreamHandlerFactoryTest, CreateWithFileTransferFeatureEnabledShallReturnConcreteFileTransferStreamHandler) { - Output output; #if defined(DLT_FILE_TRANSFER_FEATURE) + std::atomic output_enabled{true}; + Output output{output_enabled}; output.SendFtVerbose(score::cpp::span{}, mw::log::LogLevel::kInfo, DltidT{}, DltidT{}, 0, 0); -#endif EXPECT_TRUE(output.IsOutputEnabled()); - -#if defined(DLT_FILE_TRANSFER_FEATURE) FileTransferStreamHandlerFactory factory(output); #else + Output output; StubFileTransferHandlerFactory factory(output); #endif std::unique_ptr stream_handler = factory.Create(); diff --git a/score/datarouter/test/ut/ut_logging/test_logparser.cpp b/score/datarouter/test/ut/ut_logging/test_logparser.cpp index b2f51966..ea0ae7d9 100644 --- a/score/datarouter/test/ut/ut_logging/test_logparser.cpp +++ b/score/datarouter/test/ut/ut_logging/test_logparser.cpp @@ -104,6 +104,7 @@ TEST(LogParserTest, SingleMessageHandler) const std::string message = MakeMessage(kTestMessageIndex, TestMessage{2345}); LogParser parser(CreateTestNvConfig(), {&any_handler}, + 1U, LogParser::HandleRequestMap{{"test::TestMessage", &type_handler_yes}, {"test::notTestMessage", &type_handler_no}}); From 1e01444bacdbf351f96603e1af8b0e876d78319e Mon Sep 17 00:00:00 2001 From: Jianjun Wen Date: Tue, 2 Jun 2026 04:24:56 +0200 Subject: [PATCH 03/22] fix review comments GIT_ORIGIN_SPP_REV_ID: 3ce4a16139ba4989d519542a3956e8b94b7b5a2a --- .../datarouter/include/daemon/dlt_log_server.h | 7 +++++++ score/datarouter/src/daemon/dlt_log_server.cpp | 17 ++++++++++------- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/score/datarouter/include/daemon/dlt_log_server.h b/score/datarouter/include/daemon/dlt_log_server.h index 2f0dd6c7..3c86fe1f 100644 --- a/score/datarouter/include/daemon/dlt_log_server.h +++ b/score/datarouter/include/daemon/dlt_log_server.h @@ -119,6 +119,11 @@ class DltLogServer : score::platform::datarouter::DltNonverboseHandlerType::IOut enabled_callback_ = enabled_callback; } + void SetDltOutputEnabled(bool enabled) + { + dlt_output_enabled_.store(enabled, std::memory_order_release); + } + void Flush() { for (auto& channel : channels_) @@ -165,6 +170,8 @@ class DltLogServer : score::platform::datarouter::DltNonverboseHandlerType::IOut } // LCOV_EXCL_STOP + bool GetDltEnabled() const noexcept; + std::string ReadLogChannelNames() const override; std::string ResetToDefault() override; std::string StoreDltConfig() override; diff --git a/score/datarouter/src/daemon/dlt_log_server.cpp b/score/datarouter/src/daemon/dlt_log_server.cpp index cd2d23d3..2f666f82 100644 --- a/score/datarouter/src/daemon/dlt_log_server.cpp +++ b/score/datarouter/src/daemon/dlt_log_server.cpp @@ -213,13 +213,11 @@ void DltLogServer::InitLogChannelsDefault(const bool reloading) void DltLogServer::SetOutputEnabled(const bool enabled) { - // compare_exchange_strong makes the load+conditional-store a single atomic - // operation, preventing the check-then-act race where two concurrent callers - // could both observe the old value and both fire enabled_callback_. - bool expected = !enabled; - if (dlt_output_enabled_.compare_exchange_strong( - expected, enabled, std::memory_order_acq_rel, std::memory_order_acquire)) + const bool update = (dlt_output_enabled_.load(std::memory_order_acquire) != enabled); + + if (update) { + dlt_output_enabled_.store(enabled, std::memory_order_release); if (enabled_callback_) { enabled_callback_(enabled); @@ -227,11 +225,16 @@ void DltLogServer::SetOutputEnabled(const bool enabled) } } -bool DltLogServer::IsOutputEnabled() const noexcept +bool DltLogServer::GetDltEnabled() const noexcept { return dlt_output_enabled_.load(std::memory_order_acquire); } +bool DltLogServer::IsOutputEnabled() const noexcept +{ + return GetDltEnabled(); +} + void DltLogServer::SaveDatabase() { PersistentConfig config; From 2de9d796f99cd306b6bd3b4608682dceb3c6d9b6 Mon Sep 17 00:00:00 2001 From: Jianjun Wen Date: Sun, 21 Jun 2026 23:05:42 +0200 Subject: [PATCH 04/22] fix review comments GIT_ORIGIN_SPP_REV_ID: 131518ccd71157043c1b0d52462bdac390462c9a --- .../datarouter/include/logparser/logparser.h | 22 +- .../datarouter/src/daemon/dlt_log_server.cpp | 11 +- score/datarouter/src/daemon/socketserver.cpp | 16 +- score/datarouter/src/logparser/logparser.cpp | 25 +-- .../test/ut/ut_logging/test_dltserver.cpp | 188 ++++++++++++++++++ .../test_file_transfer_handler_factory.cpp | 8 +- .../test/ut/ut_logging/test_logparser.cpp | 1 - 7 files changed, 216 insertions(+), 55 deletions(-) diff --git a/score/datarouter/include/logparser/logparser.h b/score/datarouter/include/logparser/logparser.h index a5004979..af1ed2b9 100644 --- a/score/datarouter/include/logparser/logparser.h +++ b/score/datarouter/include/logparser/logparser.h @@ -16,7 +16,6 @@ #include "logparser/i_logparser.h" -#include #include #include #include @@ -59,19 +58,8 @@ class LogParser : public ILogParser public: using HandleRequestMap = std::unordered_multimap; - /// Maximum number of TypeHandlers that can be bound to a single type name. - /// DltLogServer registers exactly 1 handler per type (vhandler_, fthandler_, - /// sysedr_handler_). Increase this constant if a second handler per type is added. - static constexpr std::size_t kMaxHandlersPerType{1U}; - - /// Maximum number of global (AnyHandler) handlers. - /// DltLogServer registers exactly 2: sysedr_handler_ + nvhandler_. - /// Increase this constant if a new global handler is added. - static constexpr std::size_t kMaxGlobalHandlers{2U}; - explicit LogParser(const score::mw::log::INvConfig& nv_config, - std::array global_handlers = {}, - std::size_t global_handler_count = 0U, + std::vector global_handlers = {}, HandleRequestMap handle_request_map = {}); ~LogParser() = default; @@ -87,23 +75,21 @@ class LogParser : public ILogParser public: TypeInfo info; - explicit IndexParser(TypeInfo type_info) : info{type_info}, handlers_{}, handler_count_{0U} {} + explicit IndexParser(TypeInfo type_info) : info{type_info}, handlers_{} {} void AddHandler(TypeHandler* handler); void Parse(const TimestampT timestamp, const char* const data, const BufsizeT size); private: - std::array handlers_; - std::size_t handler_count_; + std::vector handlers_; }; const HandleRequestMap handle_request_map_; std::unordered_map index_parser_map_; - const std::array global_handlers_; - const std::size_t global_handler_count_; + const std::vector global_handlers_; const score::mw::log::INvConfig& nv_config_; }; diff --git a/score/datarouter/src/daemon/dlt_log_server.cpp b/score/datarouter/src/daemon/dlt_log_server.cpp index 2f666f82..8d3145da 100644 --- a/score/datarouter/src/daemon/dlt_log_server.cpp +++ b/score/datarouter/src/daemon/dlt_log_server.cpp @@ -213,18 +213,19 @@ void DltLogServer::InitLogChannelsDefault(const bool reloading) void DltLogServer::SetOutputEnabled(const bool enabled) { - const bool update = (dlt_output_enabled_.load(std::memory_order_acquire) != enabled); - - if (update) + bool expected = !enabled; + // Atomically flips only if the current value differs — avoids the TOCTOU + // window between load() and store() for the flag itself. + if (dlt_output_enabled_.compare_exchange_strong( + expected, enabled, std::memory_order_acq_rel, std::memory_order_acquire)) { - dlt_output_enabled_.store(enabled, std::memory_order_release); + // Entered only by the thread that won the CAS; callback fires exactly once. if (enabled_callback_) { enabled_callback_(enabled); } } } - bool DltLogServer::GetDltEnabled() const noexcept { return dlt_output_enabled_.load(std::memory_order_acquire); diff --git a/score/datarouter/src/daemon/socketserver.cpp b/score/datarouter/src/daemon/socketserver.cpp index 8dc6c606..8d73e3c5 100644 --- a/score/datarouter/src/daemon/socketserver.cpp +++ b/score/datarouter/src/daemon/socketserver.cpp @@ -169,23 +169,15 @@ std::unique_ptr SocketServer::Crea std::unique_ptr Create(const score::mw::log::NvConfig& nv_config) override { - using LogParser = score::platform::internal::LogParser; + auto global_handlers = dlt_server_.GetGlobalHandlers(); + score::platform::internal::LogParser::HandleRequestMap handle_request_map; - const auto handler_vec = dlt_server_.GetGlobalHandlers(); - std::array global_handlers{}; - const std::size_t handler_count = std::min(handler_vec.size(), LogParser::kMaxGlobalHandlers); - for (std::size_t i = 0U; i < handler_count; ++i) - { - global_handlers[i] = handler_vec[i]; - } - - LogParser::HandleRequestMap handle_request_map; for (auto& binding : dlt_server_.GetTypeHandlerBindings()) { handle_request_map.emplace(std::move(binding.type_name), binding.handler); } - return std::make_unique( - nv_config, global_handlers, handler_count, std::move(handle_request_map)); + return std::make_unique( + nv_config, std::move(global_handlers), std::move(handle_request_map)); } private: diff --git a/score/datarouter/src/logparser/logparser.cpp b/score/datarouter/src/logparser/logparser.cpp index 3abb4ad0..7dd9cd88 100644 --- a/score/datarouter/src/logparser/logparser.cpp +++ b/score/datarouter/src/logparser/logparser.cpp @@ -60,31 +60,26 @@ namespace internal { LogParser::LogParser(const score::mw::log::INvConfig& nv_config, - std::array global_handlers, - std::size_t global_handler_count, + std::vector global_handlers, HandleRequestMap handle_request_map) : ILogParser(), handle_request_map_{std::move(handle_request_map)}, index_parser_map_{}, - global_handlers_{global_handlers}, - global_handler_count_{global_handler_count}, + global_handlers_{std::move(global_handlers)}, nv_config_(nv_config) { } void LogParser::IndexParser::AddHandler(TypeHandler* handler) { - if (handler_count_ < handlers_.size()) - { - handlers_[handler_count_++] = handler; - } + handlers_.push_back(handler); } void LogParser::IndexParser::Parse(const TimestampT timestamp, const char* const data, const BufsizeT size) { - for (std::size_t i = 0U; i < handler_count_; ++i) + for (const auto& handler : handlers_) { - handlers_[i]->Handle(timestamp, data, size); + handler->Handle(timestamp, data, size); } } @@ -152,9 +147,9 @@ void LogParser::Parse(TimestampT timestamp, const char* data, BufsizeT size) index_parser.Parse(timestamp, data, size); const auto& type_info = index_parser.info; - for (std::size_t i = 0U; i < global_handler_count_; ++i) + for (auto* const handler : global_handlers_) { - global_handlers_[i]->Handle(type_info, timestamp, data, size); + handler->Handle(type_info, timestamp, data, size); } } @@ -186,10 +181,10 @@ void LogParser::ParseSharedMemoryRecord(const score::mw::log::detail::SharedMemo index_parser.Parse(record.header.time_stamp, payload_ptr, payload_length_buf_size); - const auto& type_info = index_parser_entry->second.info; - for (std::size_t i = 0U; i < global_handler_count_; ++i) + const auto& type_info = index_parser.info; + for (auto* const handler : global_handlers_) { - global_handlers_[i]->Handle(type_info, record.header.time_stamp, payload_ptr, payload_length_buf_size); + handler->Handle(type_info, record.header.time_stamp, payload_ptr, payload_length_buf_size); } } diff --git a/score/datarouter/test/ut/ut_logging/test_dltserver.cpp b/score/datarouter/test/ut/ut_logging/test_dltserver.cpp index 5208bcc2..5866614f 100644 --- a/score/datarouter/test/ut/ut_logging/test_dltserver.cpp +++ b/score/datarouter/test/ut/ut_logging/test_dltserver.cpp @@ -16,6 +16,10 @@ #include "daemon/dlt_log_server.h" #include "score/datarouter/include/daemon/configurator_commands.h" #include "score/datarouter/mocks/daemon/log_sender_mock.h" +#include +#include +#include +#include #include "gtest/gtest.h" @@ -1254,4 +1258,188 @@ TEST_F(DltServerCreatedWithConfigFixture, ReadLogChannelNamesDirectCallContainsE EXPECT_NE(response_str.find("CORE"), std::string::npos) << "Response should contain CORE channel"; } +TEST_F(DltServerCreatedWithoutConfigFixture, ConcurrentEnableSameValueNoCallbackFired) +{ + EXPECT_CALL(read_callback, Call()).Times(0); + EXPECT_CALL(write_callback, Call(_)).Times(0); + + DltLogServer::DltLogServerTest server( + s_config, read_callback.AsStdFunction(), write_callback.AsStdFunction(), /*enabled=*/true); + + std::atomic callback_count{0}; + server.SetEnabledCallback([&](bool /*enabled*/) { + callback_count.fetch_add(1, std::memory_order_relaxed); + }); + + std::thread t1([&] { + server.SetDltOutputEnable(true); + }); + std::thread t2([&] { + server.SetDltOutputEnable(true); + }); + t1.join(); + t2.join(); + + EXPECT_TRUE(server.IsOutputEnabled()); + EXPECT_EQ(callback_count.load(), 0) << "No callback expected: both threads attempted a redundant enable"; +} + +TEST_F(DltServerCreatedWithoutConfigFixture, ConcurrentDisableSameValueCallbackFiredOnce) +{ + EXPECT_CALL(read_callback, Call()).Times(0); + EXPECT_CALL(write_callback, Call(_)).Times(0); + + DltLogServer::DltLogServerTest server( + s_config, read_callback.AsStdFunction(), write_callback.AsStdFunction(), /*enabled=*/true); + + std::atomic callback_count{0}; + server.SetEnabledCallback([&](bool /*enabled*/) { + callback_count.fetch_add(1, std::memory_order_relaxed); + }); + + std::thread t1([&] { + server.SetDltOutputEnable(false); + }); + std::thread t2([&] { + server.SetDltOutputEnable(false); + }); + t1.join(); + t2.join(); + + EXPECT_FALSE(server.IsOutputEnabled()); + EXPECT_EQ(callback_count.load(), 1) << "Exactly one callback expected: only one thread wins the true->false CAS"; +} + +TEST_F(DltServerCreatedWithoutConfigFixture, ConcurrentOppositeValuesCallbackCountOneOrTwo) +{ + EXPECT_CALL(read_callback, Call()).Times(0); + EXPECT_CALL(write_callback, Call(_)).Times(0); + + DltLogServer::DltLogServerTest server( + s_config, read_callback.AsStdFunction(), write_callback.AsStdFunction(), /*enabled=*/true); + + std::atomic callback_count{0}; + server.SetEnabledCallback([&](bool /*enabled*/) { + callback_count.fetch_add(1, std::memory_order_relaxed); + }); + + std::thread t_disable([&] { + server.SetDltOutputEnable(false); + }); + std::thread t_enable([&] { + server.SetDltOutputEnable(true); + }); + t_disable.join(); + t_enable.join(); + + const int count = callback_count.load(); + EXPECT_GE(count, 1) << "At least one genuine transition must have fired"; + EXPECT_LE(count, 2) << "At most two genuine transitions are possible"; + + // Flag must be consistent with whichever CAS won last (no corruption). + const bool flag = server.IsOutputEnabled(); + EXPECT_TRUE(flag == true || flag == false); +} + +TEST_F(DltServerCreatedWithoutConfigFixture, ConcurrentToggleStormFlagRemainsConsistent) +{ + EXPECT_CALL(read_callback, Call()).Times(0); + EXPECT_CALL(write_callback, Call(_)).Times(0); + + DltLogServer::DltLogServerTest server( + s_config, read_callback.AsStdFunction(), write_callback.AsStdFunction(), /*enabled=*/true); + + std::atomic callback_count{0}; + server.SetEnabledCallback([&](bool /*enabled*/) { + callback_count.fetch_add(1, std::memory_order_relaxed); + }); + + constexpr int kThreads = 8; + constexpr int kTogglesEach = 100; + + std::vector threads; + threads.reserve(kThreads); + for (int i = 0; i < kThreads; ++i) + { + const bool start_with_enable = (i % 2 == 0); + threads.emplace_back([&server, start_with_enable]() { + for (int j = 0; j < kTogglesEach; ++j) + { + server.SetDltOutputEnable((j % 2 == 0) ? start_with_enable : !start_with_enable); + } + }); + } + for (auto& t : threads) + { + t.join(); + } + + // IsOutputEnabled() returns std::atomic::load(), so no torn read + // is possible; we only assert the callback bound. + static_cast(server.IsOutputEnabled()); + EXPECT_LE(callback_count.load(), kThreads * kTogglesEach); +} + +TEST_F(DltServerCreatedWithoutConfigFixture, CallbackReceivesCorrectBooleanValuePerTransition) +{ + EXPECT_CALL(read_callback, Call()).Times(0); + EXPECT_CALL(write_callback, Call(_)).Times(0); + + DltLogServer::DltLogServerTest server( + s_config, read_callback.AsStdFunction(), write_callback.AsStdFunction(), /*enabled=*/true); + + std::vector observed_values; + std::mutex values_mutex; + server.SetEnabledCallback([&](bool enabled) { + std::lock_guard lock(values_mutex); + observed_values.push_back(enabled); + }); + + server.SetDltOutputEnable(false); // genuine: true→false + server.SetDltOutputEnable(false); // redundant: no callback + server.SetDltOutputEnable(true); // genuine: false→true + server.SetDltOutputEnable(true); // redundant: no callback + + ASSERT_EQ(observed_values.size(), 2U); + EXPECT_FALSE(observed_values[0]) << "First transition was true->false"; + EXPECT_TRUE(observed_values[1]) << "Second transition was false->true"; +} + +TEST_F(DltServerCreatedWithoutConfigFixture, ConcurrentReaderSeesOnlyValidBooleanValues) +{ + EXPECT_CALL(read_callback, Call()).Times(0); + EXPECT_CALL(write_callback, Call(_)).Times(0); + + DltLogServer::DltLogServerTest server( + s_config, read_callback.AsStdFunction(), write_callback.AsStdFunction(), /*enabled=*/true); + + std::atomic stop_reader{false}; + + std::thread reader([&] { + // static_assert documents that bool can only be true or false; + // the acquire-load is still exercised so TSan can observe ordering. + static_assert(sizeof(bool) == 1U, "bool must be exactly one byte"); + while (!stop_reader.load(std::memory_order_relaxed)) + { + // Load with acquire semantics to pair with the release-store in + // SetOutputEnabled; the result is intentionally unused here — + // the goal is to exercise the memory-ordering path under TSan. + static_cast(server.IsOutputEnabled()); + } + }); + + std::thread writer([&] { + for (int i = 0; i < 500; ++i) + { + server.SetDltOutputEnable(i % 2 == 0); + } + }); + + writer.join(); + stop_reader.store(true, std::memory_order_relaxed); + reader.join(); + // If TSan is enabled it will have reported any acquire/release violation + // during the run; no additional runtime assertion is needed here. +} + } // namespace test diff --git a/score/datarouter/test/ut/ut_logging/test_file_transfer_handler_factory.cpp b/score/datarouter/test/ut/ut_logging/test_file_transfer_handler_factory.cpp index 93e34151..40d34da3 100644 --- a/score/datarouter/test/ut/ut_logging/test_file_transfer_handler_factory.cpp +++ b/score/datarouter/test/ut/ut_logging/test_file_transfer_handler_factory.cpp @@ -22,7 +22,6 @@ #include #include -#include #include namespace score @@ -52,14 +51,15 @@ TEST(FileTransferStreamHandlerFactoryTest, CreateWithFileTransferFeatureEnabledShallReturnConcreteFileTransferStreamHandler) { + Output output; #if defined(DLT_FILE_TRANSFER_FEATURE) - std::atomic output_enabled{true}; - Output output{output_enabled}; output.SendFtVerbose(score::cpp::span{}, mw::log::LogLevel::kInfo, DltidT{}, DltidT{}, 0, 0); +#endif EXPECT_TRUE(output.IsOutputEnabled()); + +#if defined(DLT_FILE_TRANSFER_FEATURE) FileTransferStreamHandlerFactory factory(output); #else - Output output; StubFileTransferHandlerFactory factory(output); #endif std::unique_ptr stream_handler = factory.Create(); diff --git a/score/datarouter/test/ut/ut_logging/test_logparser.cpp b/score/datarouter/test/ut/ut_logging/test_logparser.cpp index ea0ae7d9..b2f51966 100644 --- a/score/datarouter/test/ut/ut_logging/test_logparser.cpp +++ b/score/datarouter/test/ut/ut_logging/test_logparser.cpp @@ -104,7 +104,6 @@ TEST(LogParserTest, SingleMessageHandler) const std::string message = MakeMessage(kTestMessageIndex, TestMessage{2345}); LogParser parser(CreateTestNvConfig(), {&any_handler}, - 1U, LogParser::HandleRequestMap{{"test::TestMessage", &type_handler_yes}, {"test::notTestMessage", &type_handler_no}}); From ca02ba415ea391d93d4b670b38f45de0504007c4 Mon Sep 17 00:00:00 2001 From: vaishnavi yendole Date: Wed, 1 Jul 2026 10:39:40 +0530 Subject: [PATCH 05/22] Sync score changes to spp GIT_ORIGIN_SPP_REV_ID: 155ca77ddfcb19319e82feee8132c29909f87201 --- score/datarouter/src/unix_domain/unix_domain_common.cpp | 5 +++++ score/mw/log/detail/common/helper_functions_test.cpp | 2 -- .../detail/data_router/data_router_message_client_impl.cpp | 3 ++- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/score/datarouter/src/unix_domain/unix_domain_common.cpp b/score/datarouter/src/unix_domain/unix_domain_common.cpp index 8771585a..5785a913 100644 --- a/score/datarouter/src/unix_domain/unix_domain_common.cpp +++ b/score/datarouter/src/unix_domain/unix_domain_common.cpp @@ -240,7 +240,12 @@ void SendSocketMessage(std::int32_t connection_file_descriptor, std::int32_t* fdptr = CmsgDataSuppressWarning(cmsg); if (num_fds > 0) { + // Disable maybe uninitialized warning for optional. + // Initialization is already checked using `has_value`. + DISABLE_WARNING_PUSH + DISABLE_WARNING_MAYBE_UNINITIALIZED fdptr[0] = file_handle.value(); + DISABLE_WARNING_POP } #endif // USE_SECURE_FILE_HANDLE_IPC } diff --git a/score/mw/log/detail/common/helper_functions_test.cpp b/score/mw/log/detail/common/helper_functions_test.cpp index c5c761ac..afa9398d 100644 --- a/score/mw/log/detail/common/helper_functions_test.cpp +++ b/score/mw/log/detail/common/helper_functions_test.cpp @@ -11,7 +11,6 @@ * SPDX-License-Identifier: Apache-2.0 ********************************************************************************/ #include "score/mw/log/detail/common/helper_functions.h" -#include #include "gtest/gtest.h" @@ -20,7 +19,6 @@ namespace helper using testing::Types; TEST(SumTest, SumNoOverflow) { - std::numeric_limits::max(); const std::size_t expected_value = 6U; std::size_t result = Sum(1U, 2U, 3U); EXPECT_EQ(result, expected_value); diff --git a/score/mw/log/detail/data_router/data_router_message_client_impl.cpp b/score/mw/log/detail/data_router/data_router_message_client_impl.cpp index e98491d9..7a6f27c6 100644 --- a/score/mw/log/detail/data_router/data_router_message_client_impl.cpp +++ b/score/mw/log/detail/data_router/data_router_message_client_impl.cpp @@ -23,6 +23,7 @@ #include "score/mw/log/detail/utils/signal_handling/signal_handling.h" #include #include +#include namespace score { @@ -200,7 +201,7 @@ bool DatarouterMessageClientImpl::StartReceiver() auto* this_ptr = this; auto connect_callback = [this_ptr](score::message_passing::IServerConnection& connection) noexcept -> std::uintptr_t { - const auto result = SignalHandling::PThreadBlockSigTerm(this_ptr->utils_.GetSignal()); + std::ignore = SignalHandling::PThreadBlockSigTerm(this_ptr->utils_.GetSignal()); const pid_t client_pid = connection.GetClientIdentity().pid; return static_cast(client_pid); }; From e4531d9f6b83ee328c7a6b389d63d09b08c8d1cf Mon Sep 17 00:00:00 2001 From: Raghavendra Maddikery Date: Fri, 3 Jul 2026 10:28:02 +0200 Subject: [PATCH 06/22] Adds destailed design for datarouter session (acquire_response) - client_session_interaction_sequence using inotify - activity_watchdog_session_lifecycle handling stale sessions - sequence_early_disconnect_race handling any race during session startup GIT_ORIGIN_SPP_REV_ID: a98614e80bc35fa95c4c63c7936e159f61057b9b --- score/datarouter/doc/design/logging_architecture.md | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/score/datarouter/doc/design/logging_architecture.md b/score/datarouter/doc/design/logging_architecture.md index d24c00b6..58bbb1db 100644 --- a/score/datarouter/doc/design/logging_architecture.md +++ b/score/datarouter/doc/design/logging_architecture.md @@ -143,13 +143,15 @@ The Adaptive AUTOSAR logging interface implementation follows standard specifica The ring buffer operates in shared memory to minimize copy overhead. Shared memory IPC provides optimal speed and flexibility for this implementation. -### Message passing interaction +### Datarouter-Client Session -The message passing connection transmits initial connection information, notifications, and handles disconnections. The connection lifecycle proceeds as follows: +The Datarouter-Client Session uses [message_passing](https://github.com/eclipse-score/communication/tree/main/score/message_passing) IPC for the initial connection, buffer acquire requests, notifications, and disconnections. The `DataRouterRecorder` sets up the session when it is created and closes it when it is destroyed. In between, the datarouter keeps one `IServerConnection` handle per client and drives the log acquisition from the server side. -- The `DataRouterRecorder` constructor creates a `DatarouterMessageClient` instance, spawning a thread that attempts server connections every 100ms. -- Upon successful connection, the client transmits a message containing application information to the server. -- The `DataRouterRecorder` destructor notifies the thread, causing normal termination. +The main idea is to keep clients independent, so that one non-responsive client should not stall the datarouter. This matters because the datarouter serves all clients from a single thread, one after another on each periodic tick, so a single blocking call would hold up every other client. To avoid this, the datarouter asks for buffers (to read) using non-blocking QNX pulses (`IServerConnection::Notify()`), which return right away and never wait on the client. So if a client is slow or stuck, its pulse simply stays unanswered while the healthy clients keep getting served. See [session_sequence_diagram](uml/client_session_interaction_sequence.puml) for the full flow. + +However, with such a design there are risks of stale sessions. A small per-session watchdog limits how long the datarouter waits for an acquire response. If a client keeps missing its deadline, the watchdog cleans up the stale session, but first it does a best-effort read of the client's last buffer so no logs are lost unnecessarily. See [activity_watchdog_session](uml/activity_watchdog_session_lifecycle.puml). + +The setup also handles an early-disconnect race, wherein if a client crashes while the datarouter is still building its session, the connect and disconnect paths work together to throw away the half-built session instead of keeping a dangling connection pointer. The client can then reconnect cleanly afterwards. See [early_disconnect_race](uml/sequence_early_disconnect_race.puml). ### datarouter From 0c7d05c6d05c3556cfb7ed44382a31bf1055006f Mon Sep 17 00:00:00 2001 From: Raghavendra Maddikery Date: Sun, 15 Feb 2026 19:07:18 +0100 Subject: [PATCH 07/22] mw/log: Enables IPC via message_passing notify support Increase message-passing notify limits to allow server->client non-blocking notifications as described in the Notify-based acquire design. GIT_ORIGIN_SPP_REV_ID: 71ef3ab5e83849c989f3b95747a75c2029213f66 --- .../test/ut/ut_logging/test_message_passing_server.cpp | 5 +++-- score/mw/log/detail/data_router/message_passing_config.h | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp b/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp index da8447dc..628f4c6e 100644 --- a/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp +++ b/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp @@ -79,6 +79,7 @@ constexpr pid_t kClienT0Pid = 1000; constexpr pid_t kClienT1Pid = 1001; constexpr pid_t kClienT2Pid = 1002; constexpr std::uint32_t kMaxSendBytes{17U}; +constexpr std::uint32_t kMaxNotifyBytes{1U}; std::uint32_t gKReceiverQueueMaxSize = 0; @@ -155,7 +156,7 @@ class MessagePassingServerFixture : public ::testing::Test server_factory_mock = std::make_shared>(); client_factory_mock = std::make_shared>(); - const score::message_passing::IServerFactory::ServerConfig server_config{gKReceiverQueueMaxSize, 0U, 0U}; + const score::message_passing::IServerFactory::ServerConfig server_config{gKReceiverQueueMaxSize, 0U, 1U}; auto server_ptr = score::cpp::pmr::make_unique>( score::cpp::pmr::get_default_resource()); @@ -163,7 +164,7 @@ class MessagePassingServerFixture : public ::testing::Test EXPECT_CALL( *server_factory_mock, - Create(CompareServiceProtocol(ServiceProtocolConfig{"/logging.datarouter_recv", kMaxSendBytes, 0U, 0U}), + Create(CompareServiceProtocol(ServiceProtocolConfig{"/logging.datarouter_recv", kMaxSendBytes, 0U, kMaxNotifyBytes}), CompareServerConfig(server_config))) .WillOnce(Return(ByMove(std::move(server_ptr)))); } diff --git a/score/mw/log/detail/data_router/message_passing_config.h b/score/mw/log/detail/data_router/message_passing_config.h index 7242bd99..5ec6ddfb 100644 --- a/score/mw/log/detail/data_router/message_passing_config.h +++ b/score/mw/log/detail/data_router/message_passing_config.h @@ -39,13 +39,13 @@ struct MessagePassingConfig static constexpr std::uint32_t kPreAllocConnections{0U}; /// \brief Maximum number of queued notifications. - static constexpr std::uint32_t kMaxQueuedNotifies{0U}; + static constexpr std::uint32_t kMaxQueuedNotifies{1U}; /// \brief Maximum reply size in bytes. static constexpr std::uint32_t kMaxReplySize{0U}; /// \brief Maximum notification size in bytes. - static constexpr std::uint32_t kMaxNotifySize{0U}; + static constexpr std::uint32_t kMaxNotifySize{1U}; /// \brief The DataRouter receiver endpoint identifier. static constexpr const char* kDatarouterReceiverIdentifier{"/logging.datarouter_recv"}; From 406da41194fca286ff1c6a462c6dc4719c9cb4d6 Mon Sep 17 00:00:00 2001 From: Raghavendra Maddikery Date: Sun, 15 Feb 2026 19:13:02 +0100 Subject: [PATCH 08/22] pas/logging: Send acquire via Notify Replace per-client AcquireRequest delivery via client sender Send() with server-side IServerConnection::Notify(). This avoids reply-blocked MsgSend/writev on a shared client engine thread and prevents cross-client head-of-line blocking. GIT_ORIGIN_SPP_REV_ID: 36526ff0ea5d08488960a7a22418fb1bb810bd99 --- .../include/daemon/message_passing_server.h | 47 +++--- .../src/daemon/message_passing_server.cpp | 110 ++++++------- .../test_message_passing_server.cpp | 150 ++++++++---------- 3 files changed, 140 insertions(+), 167 deletions(-) diff --git a/score/datarouter/include/daemon/message_passing_server.h b/score/datarouter/include/daemon/message_passing_server.h index 6f897541..722c4a55 100644 --- a/score/datarouter/include/daemon/message_passing_server.h +++ b/score/datarouter/include/daemon/message_passing_server.h @@ -71,20 +71,16 @@ class MessagePassingServer : public IMessagePassingServerSessionWrapper class SessionHandle : public daemon::ISessionHandle { public: - SessionHandle(pid_t pid, - MessagePassingServer* server, - score::cpp::pmr::unique_ptr sender) - : daemon::ISessionHandle(), sender_(std::move(sender)), pid_(pid), server_(server), sender_state_{} + SessionHandle(pid_t pid, MessagePassingServer* server) + : daemon::ISessionHandle(), pid_(pid), server_(server) { } bool AcquireRequest() const override; private: - score::cpp::pmr::unique_ptr sender_; pid_t pid_; MessagePassingServer* server_; - mutable std::optional sender_state_; }; class ISession @@ -111,27 +107,36 @@ class MessagePassingServer : public IMessagePassingServerSessionWrapper class MessagePassingServerForTest; private: + friend class SessionHandle; + + bool NotifyAcquireRequest(pid_t pid); + bool NotifyAcquireRequestWhileLocked(pid_t pid); void NotifyAcquireRequestFailed(std::int32_t pid); - void MessageCallback(const score::cpp::span message, const pid_t pid); - void OnConnectRequest(const score::cpp::span message, const pid_t pid); - void OnAcquireResponse(const score::cpp::span message, const pid_t pid); + void MessageCallback(score::message_passing::IServerConnection& connection, score::cpp::span message); + void OnConnectRequest(score::message_passing::IServerConnection& connection, + const score::cpp::span message, + pid_t pid); + void OnAcquireResponse(score::message_passing::IServerConnection& connection, + const score::cpp::span message, + pid_t pid); using TimestampT = std::chrono::steady_clock::time_point; struct SessionWrapper { - SessionWrapper(IMessagePassingServerSessionWrapper* server_instance, - pid_t process_id, - std::unique_ptr session_instance) - : server(server_instance), - pid(process_id), - session(std::move(session_instance)), - enqueued(false), - running(false), - to_delete(false), - closed_by_peer(false), - to_force_finish(false) + SessionWrapper(IMessagePassingServerSessionWrapper* server_instance, + pid_t process_id, + std::unique_ptr session_instance) + : server(server_instance), + pid(process_id), + session(std::move(session_instance)), + connection(nullptr), + enqueued(false), + running(false), + to_delete(false), + closed_by_peer(false), + to_force_finish(false) { } @@ -165,6 +170,8 @@ class MessagePassingServer : public IMessagePassingServerSessionWrapper pid_t pid; std::unique_ptr session; + score::message_passing::IServerConnection* connection; + bool enqueued; bool running; bool to_delete; diff --git a/score/datarouter/src/daemon/message_passing_server.cpp b/score/datarouter/src/daemon/message_passing_server.cpp index 4539bfdc..d7ffb6bb 100644 --- a/score/datarouter/src/daemon/message_passing_server.cpp +++ b/score/datarouter/src/daemon/message_passing_server.cpp @@ -19,6 +19,7 @@ #include "score/memory.hpp" #include +#include #include #include #include @@ -154,8 +155,7 @@ MessagePassingServer::MessagePassingServer(MessagePassingServer::SessionFactory auto received_send_message_callback = [this_ptr = this]( score::message_passing::IServerConnection& connection, const score::cpp::span message) noexcept -> score::cpp::blank { - const pid_t client_pid = connection.GetClientIdentity().pid; - this_ptr->MessageCallback(message, client_pid); + this_ptr->MessageCallback(connection, message); return {}; }; auto received_send_message_with_reply_callback = @@ -364,8 +364,10 @@ void MessagePassingServer::FinishPreviousSessionWhileLocked( }); } -void MessagePassingServer::MessageCallback(const score::cpp::span message, const pid_t pid) +void MessagePassingServer::MessageCallback(score::message_passing::IServerConnection& connection, + score::cpp::span message) { + const pid_t pid = connection.GetClientIdentity().pid; if (message.empty()) { std::cerr << "MessagePassingServer: Empty message received from " << pid; @@ -377,10 +379,10 @@ void MessagePassingServer::MessageCallback(const score::cpp::span message, const pid_t pid) +void MessagePassingServer::OnConnectRequest(score::message_passing::IServerConnection& connection, + const score::cpp::span message, + const pid_t pid) { score::mw::log::detail::ConnectMessageFromClient conn; @@ -415,38 +419,6 @@ void MessagePassingServer::OnConnectRequest(const score::cpp::spanCreate(protocol_config, client_config); - // check for timeout or exit request if (stop_source_.stop_requested()) { @@ -462,8 +434,9 @@ void MessagePassingServer::OnConnectRequest(const score::cpp::span session_handle{ - ::score::cpp::pmr::make_unique(memory_resource, pid, this, std::move(sender))}; + ::score::cpp::pmr::make_unique(memory_resource, pid, this)}; auto session = factory_(pid, conn, std::move(session_handle)); if (session) { @@ -482,12 +455,15 @@ void MessagePassingServer::OnConnectRequest(const score::cpp::span lock(mutex_); auto emplace_result = pid_session_map_.emplace(pid, SessionWrapper{this, pid, std::move(session)}); + emplace_result.first->second.connection = &connection; // enqueue the tick to speed up processing connection emplace_result.first->second.EnqueueTickWhileLocked(); } } -void MessagePassingServer::OnAcquireResponse(const score::cpp::span message, const pid_t pid) +void MessagePassingServer::OnAcquireResponse(score::message_passing::IServerConnection& connection, + const score::cpp::span message, + const pid_t pid) { std::lock_guard lock(mutex_); const auto found = pid_session_map_.find(pid); @@ -495,6 +471,10 @@ void MessagePassingServer::OnAcquireResponse(const score::cpp::spansecond; + if (wrapper.connection == nullptr) + { + return false; + } + + constexpr std::array message{score::cpp::to_underlying(DatarouterMessageIdentifier::kAcquireRequest)}; + auto ret = wrapper.connection->Notify(message); + if (!ret) + { + wrapper.EnqueueForDeleteWhileLocked(true); + } + return true; +} + +bool MessagePassingServer::NotifyAcquireRequest(const pid_t pid) +{ + std::lock_guard lock(mutex_); + return NotifyAcquireRequestWhileLocked(pid); +} + void MessagePassingServer::NotifyAcquireRequestFailed(std::int32_t pid) { std::lock_guard lock(mutex_); @@ -532,28 +541,11 @@ void MessagePassingServer::NotifyAcquireRequestFailed(std::int32_t pid) bool MessagePassingServer::SessionHandle::AcquireRequest() const { - if (!sender_state_.has_value()) - { - sender_->Start(score::message_passing::IClientConnection::StateCallback{}, - score::message_passing::IClientConnection::NotifyCallback{}); - } - - sender_state_ = sender_->GetState(); - if (sender_state_ != score::message_passing::IClientConnection::State::kReady) + if (server_ == nullptr) { return false; } - constexpr std::array kMessage{score::cpp::to_underlying(DatarouterMessageIdentifier::kAcquireRequest)}; - auto ret = sender_->Send(kMessage); - if (!ret) - { - if (server_ != nullptr) - { - server_->NotifyAcquireRequestFailed(pid_); - return true; - } - } - return true; + return server_->NotifyAcquireRequest(pid_); } } // namespace internal diff --git a/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp b/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp index 628f4c6e..5e228581 100644 --- a/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp +++ b/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp @@ -28,6 +28,7 @@ #include #include +#include #include #include @@ -213,11 +214,6 @@ class MessagePassingServerFixture : public ::testing::Test return session; }; } - void ExpectClientDestruction(StrictMock<::score::message_passing::ClientConnectionMock>* client_mock) - { - EXPECT_CALL(*client_mock, Destruct()).Times(AnyNumber()); - } - void ExpectServerDestruction() const { EXPECT_CALL(*server_mock, Destruct()).Times(AnyNumber()); @@ -272,27 +268,22 @@ class MessagePassingServerFixture : public ::testing::Test return message; } - StrictMock<::score::message_passing::ClientConnectionMock>* ExpectConnectCallBackCalledAndClientCreated( - const pid_t pid) + StrictMock<::score::message_passing::ServerConnectionMock>* ConnectClientAndSendConnectMessage(const pid_t pid) { - auto client = score::cpp::pmr::make_unique>( - score::cpp::pmr::get_default_resource()); - - auto* client_mock = client.get(); + auto connection = std::make_unique>(); + auto* connection_ptr = connection.get(); + auto emplace_result = connections_.emplace(pid, std::move(connection)); + EXPECT_TRUE(emplace_result.second); - EXPECT_CALL(*client_factory_mock, - Create(Matcher(_), - Matcher(_))) - .WillOnce(Return(ByMove(std::move(client)))); - - StrictMock<::score::message_passing::ServerConnectionMock> connection; - score::message_passing::ClientIdentity client_identity{pid, 0, 0}; - EXPECT_CALL(connection, GetClientIdentity()).Times(AnyNumber()).WillRepeatedly(ReturnRef(client_identity)); + connection_identities_.insert_or_assign(pid, score::message_passing::ClientIdentity{pid, 0, 0}); + EXPECT_CALL(*connection_ptr, GetClientIdentity()) + .Times(AnyNumber()) + .WillRepeatedly(ReturnRef(connection_identities_.at(pid))); auto message = CreateConnectMessageSample(pid); - sent_callback(connection, message); + sent_callback(*connection_ptr, message); - return client_mock; + return connection_ptr; } void UninstantiateServer() @@ -305,11 +296,11 @@ class MessagePassingServerFixture : public ::testing::Test EXPECT_CALL(*unistd_mock, getpid()).WillRepeatedly(Return(kOurPid)); } - void ExpectMessageSendInSequence(const DatarouterMessageIdentifier& id, - ::testing::Sequence& seq, - StrictMock<::score::message_passing::ClientConnectionMock>* client_mock) + void ExpectAcquireNotifyInSequence(const DatarouterMessageIdentifier& id, + ::testing::Sequence& seq, + StrictMock<::score::message_passing::ServerConnectionMock>* connection_mock) { - EXPECT_CALL(*client_mock, Send(An>())) + EXPECT_CALL(*connection_mock, Notify(An>())) .InSequence(seq) .WillOnce([id](const auto m) { score::cpp::expected_blank ret{}; @@ -321,9 +312,9 @@ class MessagePassingServerFixture : public ::testing::Test }); } - void ExpectAndFailShortMessageSend(StrictMock<::score::message_passing::ClientConnectionMock>* client_mock) + void ExpectAndFailAcquireNotify(StrictMock<::score::message_passing::ServerConnectionMock>* connection_mock) { - EXPECT_CALL(*client_mock, Send(Matcher>(_))) + EXPECT_CALL(*connection_mock, Notify(Matcher>(_))) .WillOnce(Return(score::cpp::make_unexpected(score::os::Error::createFromErrno(EINVAL)))); } @@ -338,6 +329,9 @@ class MessagePassingServerFixture : public ::testing::Test score::message_passing::MessageCallback sent_callback; score::message_passing::MessageCallback sent_with_reply_callback; + std::unordered_map>> connections_; + std::map connection_identities_; + std::mutex map_mutex; std::condition_variable map_cond; // currently only used for destruction std::unordered_map session_map; @@ -428,40 +422,29 @@ TEST_F(MessagePassingServerFixture, TestOneConnectAcquireRelease) EXPECT_EQ(tick_count, 0); EXPECT_EQ(construct_count, 0); - auto* client = ExpectConnectCallBackCalledAndClientCreated(kClienT0Pid); + auto* connection_ptr = ConnectClientAndSendConnectMessage(kClienT0Pid); EXPECT_EQ(construct_count, 1); - EXPECT_CALL(*client, - Start(Matcher(_), - Matcher(_))); - - EXPECT_CALL(*client, GetState()).WillRepeatedly(Return(score::message_passing::IClientConnection::State::kReady)); - ::testing::Sequence seq; - ExpectMessageSendInSequence(DatarouterMessageIdentifier::kAcquireRequest, seq, client); + ExpectAcquireNotifyInSequence(DatarouterMessageIdentifier::kAcquireRequest, seq, connection_ptr); session_map.at(kClienT0Pid).handle->AcquireRequest(); EXPECT_EQ(acquire_response_count, 0); - StrictMock<::score::message_passing::ServerConnectionMock> connection; - score::message_passing::ClientIdentity client_identity{kClienT0Pid, 0, 0}; - EXPECT_CALL(connection, GetClientIdentity()).Times(AnyNumber()).WillRepeatedly(ReturnRef(client_identity)); - score::mw::log::detail::ReadAcquireResult acquire_result{0U}; std::array message{}; message[0] = score::cpp::to_underlying(DatarouterMessageIdentifier::kAcquireResponse); std::memcpy(&message[1], &acquire_result, sizeof(acquire_result)); - sent_callback(connection, message); + sent_callback(*connection_ptr, message); EXPECT_EQ(acquire_response_count, 1); EXPECT_EQ(closed_by_peer_count, 0); EXPECT_FALSE(session_map.empty()); - ExpectAndFailShortMessageSend(client); + ExpectAndFailAcquireNotify(connection_ptr); ExpectServerDestruction(); - ExpectClientDestruction(client); session_map.at(kClienT0Pid).handle->AcquireRequest(); { // let the worker thread process the fault; wait until it erases the client @@ -485,15 +468,15 @@ TEST_F(MessagePassingServerFixture, TestTripleConnectDifferentPids) EXPECT_EQ(construct_count, 0); - auto* client0 = ExpectConnectCallBackCalledAndClientCreated(kClienT0Pid); - auto* client1 = ExpectConnectCallBackCalledAndClientCreated(kClienT1Pid); - auto* client2 = ExpectConnectCallBackCalledAndClientCreated(kClienT2Pid); + auto* connection0 = ConnectClientAndSendConnectMessage(kClienT0Pid); + auto* connection1 = ConnectClientAndSendConnectMessage(kClienT1Pid); + auto* connection2 = ConnectClientAndSendConnectMessage(kClienT2Pid); EXPECT_EQ(construct_count, 3); ExpectServerDestruction(); - ExpectClientDestruction(client0); - ExpectClientDestruction(client1); - ExpectClientDestruction(client2); + std::ignore = connection0; + std::ignore = connection1; + std::ignore = connection2; EXPECT_EQ(closed_by_peer_count, 0); EXPECT_EQ(destruct_count, 0); @@ -516,22 +499,25 @@ TEST_F(MessagePassingServerFixture, TestTripleConnectSamePid) EXPECT_EQ(construct_count, 0); // Recieving new connect with old pid means that old pid owner died and disconnect_callback was called. - auto* client0 = ExpectConnectCallBackCalledAndClientCreated(kClienT0Pid); + auto* connection0 = ConnectClientAndSendConnectMessage(kClienT0Pid); EXPECT_CALL(connection, GetClientIdentity()).WillOnce(ReturnRef(client_identity)); - ExpectClientDestruction(client0); this->disconnect_callback(connection); using namespace std::chrono_literals; std::this_thread::sleep_for(100ms); - auto* client1 = ExpectConnectCallBackCalledAndClientCreated(kClienT0Pid); + connections_.erase(kClienT0Pid); + auto* connection1 = ConnectClientAndSendConnectMessage(kClienT0Pid); EXPECT_CALL(connection, GetClientIdentity()).WillOnce(ReturnRef(client_identity)); - ExpectClientDestruction(client1); this->disconnect_callback(connection); std::this_thread::sleep_for(100ms); - auto* client2 = ExpectConnectCallBackCalledAndClientCreated(kClienT0Pid); - ExpectClientDestruction(client2); + connections_.erase(kClienT0Pid); + auto* connection2 = ConnectClientAndSendConnectMessage(kClienT0Pid); EXPECT_EQ(construct_count, 3); + std::ignore = connection0; + std::ignore = connection1; + std::ignore = connection2; + ExpectServerDestruction(); EXPECT_EQ(closed_by_peer_count, 2); @@ -554,14 +540,13 @@ TEST_F(MessagePassingServerFixture, TestSamePidWhileRunning) tick_blocker = true; EXPECT_EQ(tick_count, 0); EXPECT_EQ(construct_count, 0); - auto* client0 = ExpectConnectCallBackCalledAndClientCreated(kClienT0Pid); - auto* client1 = ExpectConnectCallBackCalledAndClientCreated(kClienT1Pid); - auto* client2 = ExpectConnectCallBackCalledAndClientCreated(kClienT2Pid); + auto* connection0 = ConnectClientAndSendConnectMessage(kClienT0Pid); + auto* connection1 = ConnectClientAndSendConnectMessage(kClienT1Pid); + auto* connection2 = ConnectClientAndSendConnectMessage(kClienT2Pid); EXPECT_EQ(construct_count, 3); ExpectServerDestruction(); - // ExpectClientDestruction(client0); // wait until CLIENT0 is blocked inside the first tick session_map.at(kClienT0Pid).WaitStartOfFirstTick(); @@ -574,12 +559,13 @@ TEST_F(MessagePassingServerFixture, TestSamePidWhileRunning) StrictMock<::score::message_passing::ServerConnectionMock> connection; score::message_passing::ClientIdentity client_identity{kClienT0Pid, 0, 0}; EXPECT_CALL(connection, GetClientIdentity()).WillOnce(ReturnRef(client_identity)); - ExpectClientDestruction(client0); this->disconnect_callback(connection); std::this_thread::sleep_for(100ms); - auto* new_client = ExpectConnectCallBackCalledAndClientCreated(kClienT0Pid); - ExpectClientDestruction(new_client); + connections_.erase(kClienT0Pid); + + auto* new_connection = ConnectClientAndSendConnectMessage(kClienT0Pid); + std::ignore = new_connection; }); EXPECT_EQ(destruct_count, 0); // no destruction while we are still in the tick @@ -592,8 +578,9 @@ TEST_F(MessagePassingServerFixture, TestSamePidWhileRunning) EXPECT_EQ(destruct_count, 1); EXPECT_GE(tick_count, 2); - ExpectClientDestruction(client1); - ExpectClientDestruction(client2); + std::ignore = connection0; + std::ignore = connection1; + std::ignore = connection2; UninstantiateServer(); EXPECT_EQ(closed_by_peer_count, 1); @@ -609,9 +596,9 @@ TEST_F(MessagePassingServerFixture, TestSamePidWhileQueued) tick_blocker = true; EXPECT_EQ(tick_count, 0); EXPECT_EQ(construct_count, 0); - auto* client0 = ExpectConnectCallBackCalledAndClientCreated(kClienT0Pid); - auto* client1 = ExpectConnectCallBackCalledAndClientCreated(kClienT1Pid); - auto* client2 = ExpectConnectCallBackCalledAndClientCreated(kClienT2Pid); + auto* connection0 = ConnectClientAndSendConnectMessage(kClienT0Pid); + auto* connection1 = ConnectClientAndSendConnectMessage(kClienT1Pid); + auto* connection2 = ConnectClientAndSendConnectMessage(kClienT2Pid); EXPECT_EQ(construct_count, 3); ExpectServerDestruction(); @@ -628,12 +615,13 @@ TEST_F(MessagePassingServerFixture, TestSamePidWhileQueued) StrictMock<::score::message_passing::ServerConnectionMock> connection; score::message_passing::ClientIdentity client_identity{kClienT2Pid, 0, 0}; EXPECT_CALL(connection, GetClientIdentity()).WillOnce(ReturnRef(client_identity)); - ExpectClientDestruction(client2); this->disconnect_callback(connection); std::this_thread::sleep_for(100ms); - auto* new_client = ExpectConnectCallBackCalledAndClientCreated(kClienT2Pid); - ExpectClientDestruction(new_client); + connections_.erase(kClienT2Pid); + + auto* new_connection = ConnectClientAndSendConnectMessage(kClienT2Pid); + std::ignore = new_connection; }); EXPECT_EQ(destruct_count, 0); // no destruction while we are still in the tick @@ -646,8 +634,9 @@ TEST_F(MessagePassingServerFixture, TestSamePidWhileQueued) EXPECT_EQ(destruct_count, 1); EXPECT_GE(tick_count, 2); - ExpectClientDestruction(client0); - ExpectClientDestruction(client1); + std::ignore = connection0; + std::ignore = connection1; + std::ignore = connection2; UninstantiateServer(); EXPECT_EQ(closed_by_peer_count, 1); @@ -681,25 +670,10 @@ TEST(MessagePassingServerTests, sessionWrapperCreateTest) TEST(MessagePassingServerTests, sessionHandleCreateTest) { const pid_t pid = 0; - - auto client = score::cpp::pmr::make_unique(score::cpp::pmr::get_default_resource()); - - auto* client_raw_ptr = client.get(); MessagePassingServer* msg_server = nullptr; - EXPECT_CALL(*client_raw_ptr, - Start(Matcher(_), - Matcher(_))); - - EXPECT_CALL(*client_raw_ptr, GetState()) - .WillRepeatedly(Return(score::message_passing::IClientConnection::State::kReady)); - - EXPECT_CALL(*client_raw_ptr, Send(An>())).Times(1); - - MessagePassingServer::SessionHandle session_handle(pid, msg_server, std::move(client)); - - EXPECT_NO_FATAL_FAILURE(session_handle.AcquireRequest()); - EXPECT_CALL(*client_raw_ptr, Destruct()).Times(AnyNumber()); + MessagePassingServer::SessionHandle session_handle(pid, msg_server); + EXPECT_FALSE(session_handle.AcquireRequest()); } struct TestParams From 83d9775dc01a2b83ad63c5a425c638ec915d371e Mon Sep 17 00:00:00 2001 From: Raghavendra Maddikery Date: Sun, 15 Feb 2026 19:17:48 +0100 Subject: [PATCH 09/22] mw/log: Handle acquire via NotifyCallback Register an IClientConnection NotifyCallback and process kAcquireRequest notifications by switching buffers and replying with kAcquireResponse. This is the client-side Notify-based acquire design. GIT_ORIGIN_SPP_REV_ID: 735123e22884507b8c9ab85f0833ae28a5b7865e --- .../data_router_message_client_impl.cpp | 20 ++- .../data_router_message_client_impl.h | 1 + .../data_router_message_client_test.cpp | 123 +++++++++++++++++- 3 files changed, 137 insertions(+), 7 deletions(-) diff --git a/score/mw/log/detail/data_router/data_router_message_client_impl.cpp b/score/mw/log/detail/data_router/data_router_message_client_impl.cpp index 7a6f27c6..cd1058d3 100644 --- a/score/mw/log/detail/data_router/data_router_message_client_impl.cpp +++ b/score/mw/log/detail/data_router/data_router_message_client_impl.cpp @@ -363,11 +363,29 @@ score::cpp::expected_blank DatarouterMessageClientImpl::Create state_condition_.notify_all(); }; + auto notify_callback = [this](score::cpp::span message) noexcept { + this->OnNotify(message); + }; // coverity[autosar_cpp14_a5_1_4_violation]: See justification above - sender_->Start(state_callback, score::message_passing::IClientConnection::NotifyCallback{}); + sender_->Start(state_callback, notify_callback); return {}; } +void DatarouterMessageClientImpl::OnNotify(const score::cpp::span message) noexcept +{ + if (message.size() != 1U) + { + return; + } + + if (message.front() != score::cpp::to_underlying(DatarouterMessageIdentifier::kAcquireRequest)) + { + return; + } + + OnAcquireRequest(); +} + void DatarouterMessageClientImpl::OnAcquireRequest() noexcept { // The acquire request shall be the first message Datarouter sends to the client. diff --git a/score/mw/log/detail/data_router/data_router_message_client_impl.h b/score/mw/log/detail/data_router/data_router_message_client_impl.h index 5109ed2b..5500a731 100644 --- a/score/mw/log/detail/data_router/data_router_message_client_impl.h +++ b/score/mw/log/detail/data_router/data_router_message_client_impl.h @@ -99,6 +99,7 @@ class DatarouterMessageClientImpl : public DatarouterMessageClient private: void RunConnectTask(); void OnAcquireRequest() noexcept; + void OnNotify(score::cpp::span message) noexcept; void UnlinkSharedMemoryFile() noexcept; void HandleFirstMessageReceived() noexcept; void RequestInternalShutdown() noexcept; diff --git a/score/mw/log/detail/data_router/data_router_message_client_test.cpp b/score/mw/log/detail/data_router/data_router_message_client_test.cpp index 38704437..47a81959 100644 --- a/score/mw/log/detail/data_router/data_router_message_client_test.cpp +++ b/score/mw/log/detail/data_router/data_router_message_client_test.cpp @@ -105,6 +105,7 @@ constexpr pthread_t kThreadId = 42; constexpr auto kLoggerThreadName = "logger"; constexpr uid_t kDatarouterDummyUid = 111; constexpr std::uint32_t kMaxSendBytes{17U}; +constexpr std::uint32_t kMaxNotifyBytes{1U}; constexpr std::uint32_t kMaxNumberMessagesInReceiverQueue{0UL}; class DatarouterMessageClientFixture : public ::testing::Test @@ -225,14 +226,15 @@ class DatarouterMessageClientFixture : public ::testing::Test score::message_passing::ClientConnectionMock* ExpectSenderCreation( score::message_passing::IClientConnection::StateCallback* state_callback = nullptr, - std::promise* callback_registered = nullptr) + std::promise* callback_registered = nullptr, + score::message_passing::IClientConnection::NotifyCallback* notify_callback = nullptr) { sender_mock_in_transit_ = score::cpp::pmr::make_unique>( score::cpp::pmr::get_default_resource()); auto* sender_mock = sender_mock_in_transit_.get(); const score::message_passing::ServiceProtocolConfig service_protocol_config{ - kDatarouterReceiverIdentifier, kMaxSendBytes, 0U, 0U}; + kDatarouterReceiverIdentifier, kMaxSendBytes, 0U, kMaxNotifyBytes}; const score::message_passing::IClientFactory::ClientConfig client_config{0, 10, false, true, false}; @@ -243,13 +245,17 @@ class DatarouterMessageClientFixture : public ::testing::Test EXPECT_CALL(*sender_mock, Start(Matcher(_), Matcher(_))) - .WillOnce([state_callback, callback_registered]( + .WillOnce([state_callback, callback_registered, notify_callback]( score::message_passing::IClientConnection::StateCallback st_callback, - score::message_passing::IClientConnection::NotifyCallback) { + score::message_passing::IClientConnection::NotifyCallback nt_callback) { if (state_callback != nullptr) { *state_callback = std::move(st_callback); } + if (notify_callback != nullptr) + { + *notify_callback = std::move(nt_callback); + } if (callback_registered != nullptr) { callback_registered->set_value(); @@ -340,6 +346,25 @@ class DatarouterMessageClientFixture : public ::testing::Test acquire_callback(connection, message); } + void SendAcquireNotifyAndExpectResponse(score::message_passing::IClientConnection::NotifyCallback& notify_callback, + score::message_passing::ClientConnectionMock** sender_ptr, + bool first_message, + bool unlink_successful = true) + { + ReadAcquireResult acquired_data{}; + acquired_data.acquired_buffer = shared_data_.control_block.switch_count_points_active_for_writing.load(); + + if (first_message) + { + ExpectUnlinkMwsrWriterFile(unlink_successful); + } + + ExpectSendAcquireResponse(*sender_ptr, acquired_data); + + const std::array message{score::cpp::to_underlying(DatarouterMessageIdentifier::kAcquireRequest)}; + notify_callback(message); + } + void ExpectSenderAndReceiverCreation( score::message_passing::ServerMock** receiver_ptr, score::message_passing::ClientConnectionMock** sender_ptr, @@ -352,7 +377,8 @@ class DatarouterMessageClientFixture : public ::testing::Test score::message_passing::MessageCallback* sent_callback = nullptr, score::message_passing::MessageCallback* sent_with_reply_callback = nullptr, bool block_termination_signal_pass = true, - bool receiver_start_listening = true) + bool receiver_start_listening = true, + score::message_passing::IClientConnection::NotifyCallback* notify_callback = nullptr) { std::ignore = block_termination_signal_pass; // TODO: remove this param @@ -373,7 +399,7 @@ class DatarouterMessageClientFixture : public ::testing::Test ExpectSetLoggerThreadName(); - auto* sender = ExpectSenderCreation(state_callback, callback_registered); + auto* sender = ExpectSenderCreation(state_callback, callback_registered, notify_callback); if (sender_ptr != nullptr) { *sender_ptr = sender; @@ -696,6 +722,91 @@ TEST_F(DatarouterMessageClientFixture, AcquireRequestShouldSendExpectedAcquireRe ExpectClientDestruction(sender_ptr); } +TEST_F(DatarouterMessageClientFixture, AcquireNotifyShouldSendExpectedAcquireResponse) +{ + RecordProperty("ASIL", "B"); + RecordProperty("Description", "Verifies that an acquire request delivered via NotifyCallback sends the expected acquire response."); + RecordProperty("TestType", "Interface test"); + RecordProperty("DerivationTechnique", "Generation and analysis of equivalence classes"); + + testing::InSequence order_matters; + + score::message_passing::ClientConnectionMock* sender_ptr{}; + score::message_passing::ServerMock* receiver_ptr{}; + score::message_passing::ConnectCallback connect_callback; + score::message_passing::DisconnectCallback disconnect_callback; + score::message_passing::MessageCallback sent_callback; + score::message_passing::MessageCallback sent_with_reply_callback; + score::message_passing::IClientConnection::StateCallback state_callback; + score::message_passing::IClientConnection::NotifyCallback notify_callback; + + ExpectSenderAndReceiverCreation(&receiver_ptr, + &sender_ptr, + &state_callback, + nullptr, + {}, + &connect_callback, + &disconnect_callback, + &sent_callback, + &sent_with_reply_callback, + true, + true, + ¬ify_callback); + + ExecuteCreateSenderAndReceiverSequence(true, &state_callback); + + bool first_message = true; + SendAcquireNotifyAndExpectResponse(notify_callback, &sender_ptr, first_message, false); + + ExpectServerDestruction(receiver_ptr); + ExpectClientDestruction(sender_ptr); +} + +TEST_F(DatarouterMessageClientFixture, InvalidNotifyShouldNotSendAcquireResponse) +{ + RecordProperty("ASIL", "B"); + RecordProperty("Description", "Verifies that invalid NotifyCallback payloads are ignored."); + RecordProperty("TestType", "Interface test"); + RecordProperty("DerivationTechnique", "Generation and analysis of equivalence classes"); + + testing::InSequence order_matters; + + score::message_passing::ClientConnectionMock* sender_ptr{}; + score::message_passing::ServerMock* receiver_ptr{}; + score::message_passing::IClientConnection::StateCallback state_callback; + score::message_passing::IClientConnection::NotifyCallback notify_callback; + + ExpectSenderAndReceiverCreation(&receiver_ptr, + &sender_ptr, + &state_callback, + nullptr, + {}, + nullptr, + nullptr, + nullptr, + nullptr, + true, + true, + ¬ify_callback); + + ExecuteCreateSenderAndReceiverSequence(true, &state_callback); + + EXPECT_CALL(*sender_ptr, Send(Matcher>(_))).Times(0); + + const score::cpp::span empty_message{}; + notify_callback(empty_message); + + const std::array oversized_message{score::cpp::to_underlying(DatarouterMessageIdentifier::kAcquireRequest), + std::uint8_t{0U}}; + notify_callback(oversized_message); + + const std::array wrong_id_message{score::cpp::to_underlying(DatarouterMessageIdentifier::kConnect)}; + notify_callback(wrong_id_message); + + ExpectServerDestruction(receiver_ptr); + ExpectClientDestruction(sender_ptr); +} + TEST_F(DatarouterMessageClientFixture, SecondAcquireRequestShouldNotSetMwsrReader) { RecordProperty("ASIL", "B"); From c12523914d9a97ec7d3e809cb340365416a8d45e Mon Sep 17 00:00:00 2001 From: Raghavendra Maddikery Date: Sun, 15 Feb 2026 19:23:07 +0100 Subject: [PATCH 10/22] pas/logging: Check ongoing acquire notifies Track per-session state and avoid sending additional kAcquireRequest notifications while a previous request is still outstanding. Clear stored server connection pointers on disconnect to avoid stale references. GIT_ORIGIN_SPP_REV_ID: 5cafcea87edf14043e3ebe1a85a11d90dde75e75 --- .../include/daemon/message_passing_server.h | 80 +++++------ .../src/daemon/message_passing_server.cpp | 129 ++++++++++-------- .../test_message_passing_server.cpp | 1 + 3 files changed, 111 insertions(+), 99 deletions(-) diff --git a/score/datarouter/include/daemon/message_passing_server.h b/score/datarouter/include/daemon/message_passing_server.h index 722c4a55..02f9b57e 100644 --- a/score/datarouter/include/daemon/message_passing_server.h +++ b/score/datarouter/include/daemon/message_passing_server.h @@ -86,10 +86,10 @@ class MessagePassingServer : public IMessagePassingServerSessionWrapper class ISession { public: - virtual bool Tick() = 0; - virtual void OnAcquireResponse(const score::mw::log::detail::ReadAcquireResult&) = 0; - virtual void OnClosedByPeer() = 0; - virtual bool IsSourceClosed() = 0; + virtual bool tick() = 0; + virtual void on_acquire_response(const score::mw::log::detail::ReadAcquireResult&) = 0; + virtual void on_closed_by_peer() = 0; + virtual bool is_source_closed() = 0; virtual ~ISession() = default; }; @@ -121,62 +121,62 @@ class MessagePassingServer : public IMessagePassingServerSessionWrapper const score::cpp::span message, pid_t pid); - using TimestampT = std::chrono::steady_clock::time_point; + using timestamp_t = std::chrono::steady_clock::time_point; struct SessionWrapper { - SessionWrapper(IMessagePassingServerSessionWrapper* server_instance, - pid_t process_id, - std::unique_ptr session_instance) - : server(server_instance), - pid(process_id), - session(std::move(session_instance)), - connection(nullptr), - enqueued(false), - running(false), - to_delete(false), - closed_by_peer(false), - to_force_finish(false) + SessionWrapper(IMessagePassingServerSessionWrapper* server, pid_t pid, std::unique_ptr session) + : server_(server), + pid_(pid), + session_(std::move(session)), + connection_(nullptr), + acquire_in_flight_(false), + enqueued_(false), + running_(false), + to_delete_(false), + closed_by_peer_(false), + to_force_finish_(false) { } - void EnqueueForDeleteWhileLocked(bool by_peer = false); - bool IsMarkedForDelete() const + void enqueue_for_delete_while_locked(bool by_peer = false); + bool is_marked_for_delete() { - return to_delete; + return to_delete_; } - bool GetResetClosedByPeer() + bool get_reset_closed_by_peer() { - bool by_peer = closed_by_peer; - closed_by_peer = false; + bool by_peer = closed_by_peer_; + closed_by_peer_ = false; return by_peer; } - bool TickAtWorkerThread() const; - void NotifyClosedByPeer() const; + bool tick_at_worker_thread(); + void notify_closed_by_peer(); - void SetRunningWhileLocked(); - bool ResetRunningWhileLocked(bool requeue); + void set_running_while_locked(); + bool reset_running_while_locked(bool requeue); - void EnqueueTickWhileLocked(); + void enqueue_tick_while_locked(); - inline bool GetIsSourceClosed() const + inline bool GetIsSourceClosed() { - return session->IsSourceClosed(); + return session_->is_source_closed(); } - IMessagePassingServerSessionWrapper* server; - pid_t pid; - std::unique_ptr session; + IMessagePassingServerSessionWrapper* server_; + pid_t pid_; + std::unique_ptr session_; - score::message_passing::IServerConnection* connection; + score::message_passing::IServerConnection* connection_; + bool acquire_in_flight_; - bool enqueued; - bool running; - bool to_delete; - bool closed_by_peer; - bool to_force_finish; + bool enqueued_; + bool running_; + bool to_delete_; + bool closed_by_peer_; + bool to_force_finish_; }; void FinishPreviousSessionWhileLocked(std::unordered_map::iterator it, @@ -190,7 +190,7 @@ class MessagePassingServer : public IMessagePassingServerSessionWrapper std::mutex mutex_; score::cpp::stop_source stop_source_; - TimestampT connection_timeout_; + timestamp_t connection_timeout_; score::cpp::jthread worker_thread_; std::condition_variable worker_cond_; // to wake up worker thread std::unordered_map pid_session_map_; diff --git a/score/datarouter/src/daemon/message_passing_server.cpp b/score/datarouter/src/daemon/message_passing_server.cpp index d7ffb6bb..4e1b6e85 100644 --- a/score/datarouter/src/daemon/message_passing_server.cpp +++ b/score/datarouter/src/daemon/message_passing_server.cpp @@ -19,7 +19,6 @@ #include "score/memory.hpp" #include -#include #include #include #include @@ -36,58 +35,58 @@ namespace internal using score::mw::log::detail::DatarouterMessageIdentifier; using score::mw::log::detail::MessagePassingConfig; -void MessagePassingServer::SessionWrapper::EnqueueForDeleteWhileLocked(bool by_peer) +void MessagePassingServer::SessionWrapper::enqueue_for_delete_while_locked(bool by_peer) { - to_delete = true; - closed_by_peer = by_peer; + to_delete_ = true; + closed_by_peer_ = by_peer; // in order not to mess with the logic of the queue, we don't enqueue currently running tick. Instead, we mark it // to be deleted (or re-enqueued for post-mortem processing, if closed by peer) at the end of the tick processing - if (!running && !enqueued) + if (!running_ && !enqueued_) { - server->EnqueueTickWhileLocked(pid); - enqueued = true; + server_->EnqueueTickWhileLocked(pid_); + enqueued_ = true; } } -bool MessagePassingServer::SessionWrapper::TickAtWorkerThread() const +bool MessagePassingServer::SessionWrapper::tick_at_worker_thread() { - bool requeue = session->Tick(); + bool requeue = session_->tick(); return requeue; } -void MessagePassingServer::SessionWrapper::NotifyClosedByPeer() const +void MessagePassingServer::SessionWrapper::notify_closed_by_peer() { - session->OnClosedByPeer(); + session_->on_closed_by_peer(); } -void MessagePassingServer::SessionWrapper::SetRunningWhileLocked() +void MessagePassingServer::SessionWrapper::set_running_while_locked() { - enqueued = false; - running = true; + enqueued_ = false; + running_ = true; } -bool MessagePassingServer::SessionWrapper::ResetRunningWhileLocked(bool requeue) +bool MessagePassingServer::SessionWrapper::reset_running_while_locked(bool requeue) { - running = false; + running_ = false; // check if we need to re-enqueue the tick after running again. It may happen because: // 1. not all the work in tick was done (returned early to avoid congestion); // 2. the tick was marked for delete as "closed by peer" when running, but we don't expedite its finishing. - if (requeue || closed_by_peer) + if (requeue || closed_by_peer_) { - enqueued = true; + enqueued_ = true; } - return enqueued; + return enqueued_; } -void MessagePassingServer::SessionWrapper::EnqueueTickWhileLocked() +void MessagePassingServer::SessionWrapper::enqueue_tick_while_locked() { - if (!enqueued && !to_delete) + if (!enqueued_ && !to_delete_) { - if (!running) + if (!running_) { - server->EnqueueTickWhileLocked(pid); + server_->EnqueueTickWhileLocked(pid_); } - enqueued = true; + enqueued_ = true; } } /* @@ -125,17 +124,17 @@ MessagePassingServer::MessagePassingServer(MessagePassingServer::SessionFactory std::cerr << "setname_np: " << ret_pthread.error() << std::endl; } - constexpr score::message_passing::ServiceProtocolConfig kServiceProtocolConfig{ + constexpr score::message_passing::ServiceProtocolConfig service_protocol_config{ MessagePassingConfig::kDatarouterReceiverIdentifier, MessagePassingConfig::kMaxMessageSize, MessagePassingConfig::kMaxReplySize, MessagePassingConfig::kMaxNotifySize}; - constexpr score::message_passing::IServerFactory::ServerConfig kServerConfig{ + constexpr score::message_passing::IServerFactory::ServerConfig server_config{ MessagePassingConfig::kMaxReceiverQueueSize, MessagePassingConfig::kPreAllocConnections, MessagePassingConfig::kMaxQueuedNotifies}; - receiver_ = server_factory_->Create(kServiceProtocolConfig, kServerConfig); + receiver_ = server_factory_->Create(service_protocol_config, server_config); auto connect_callback = [](score::message_passing::IServerConnection& connection) noexcept -> std::uintptr_t { const pid_t client_pid = connection.GetClientIdentity().pid; @@ -148,8 +147,9 @@ MessagePassingServer::MessagePassingServer(MessagePassingServer::SessionFactory if (found != pid_session_map_ptr->end()) { SessionWrapper& wrapper = found->second; - wrapper.to_force_finish = true; - found->second.EnqueueForDeleteWhileLocked(true); + wrapper.connection_ = nullptr; + wrapper.to_force_finish_ = true; + found->second.enqueue_for_delete_while_locked(true); } }; auto received_send_message_callback = [this_ptr = this]( @@ -213,8 +213,8 @@ MessagePassingServer::~MessagePassingServer() noexcept void MessagePassingServer::RunWorkerThread() { - constexpr std::int32_t kTimeoutInMs = 100; - TimestampT t1 = TimestampT::clock::now() + std::chrono::milliseconds(kTimeoutInMs); + constexpr std::int32_t TIMEOUT_IN_MS = 100; + timestamp_t t1 = timestamp_t::clock::now() + std::chrono::milliseconds(TIMEOUT_IN_MS); std::unique_lock lock(mutex_); while (!workers_exit_) @@ -224,15 +224,15 @@ void MessagePassingServer::RunWorkerThread() }); if (!workers_exit_) { - TimestampT now = TimestampT::clock::now(); - if (connection_timeout_ != TimestampT{} && now >= connection_timeout_) + timestamp_t now = timestamp_t::clock::now(); + if (connection_timeout_ != timestamp_t{} && now >= connection_timeout_) { - connection_timeout_ = TimestampT{}; + connection_timeout_ = timestamp_t{}; stop_source_.request_stop(); } if (now >= t1) { - t1 = now + std::chrono::milliseconds(kTimeoutInMs); + t1 = now + std::chrono::milliseconds(TIMEOUT_IN_MS); for (auto& ps : pid_session_map_) { if (ps.second.GetIsSourceClosed()) @@ -241,12 +241,12 @@ void MessagePassingServer::RunWorkerThread() this is private functions so it cannot be test. */ // LCOV_EXCL_START - ps.second.EnqueueForDeleteWhileLocked(true); + ps.second.enqueue_for_delete_while_locked(true); // LCOV_EXCL_STOP } else { - ps.second.EnqueueTickWhileLocked(); + ps.second.enqueue_tick_while_locked(); } } } @@ -257,16 +257,16 @@ void MessagePassingServer::RunWorkerThread() pid_t pid = work_queue_.front(); work_queue_.pop(); SessionWrapper& wrapper = pid_session_map_.at(pid); - wrapper.SetRunningWhileLocked(); - bool closed_by_peer = wrapper.GetResetClosedByPeer(); + wrapper.set_running_while_locked(); + bool closed_by_peer = wrapper.get_reset_closed_by_peer(); lock.unlock(); if (closed_by_peer) { - wrapper.NotifyClosedByPeer(); + wrapper.notify_closed_by_peer(); } - bool requeue = wrapper.TickAtWorkerThread(); + bool requeue = wrapper.tick_at_worker_thread(); lock.lock(); - if (wrapper.to_force_finish) + if (wrapper.to_force_finish_) { if (!closed_by_peer) { @@ -276,7 +276,7 @@ void MessagePassingServer::RunWorkerThread() this is private functions so it cannot be test. */ // LCOV_EXCL_START - wrapper.NotifyClosedByPeer(); + wrapper.notify_closed_by_peer(); requeue = true; // LCOV_EXCL_STOP } @@ -288,7 +288,7 @@ void MessagePassingServer::RunWorkerThread() lock.unlock(); do { - requeue = wrapper.TickAtWorkerThread(); + requeue = wrapper.tick_at_worker_thread(); } while (requeue); lock.lock(); // LCOV_EXCL_STOP @@ -303,13 +303,13 @@ void MessagePassingServer::RunWorkerThread() node = {}; lock.lock(); } - else if (wrapper.ResetRunningWhileLocked(requeue)) + else if (wrapper.reset_running_while_locked(requeue)) { // LCOV_EXCL_START: see above EnqueueTickWhileLocked(pid); // LCOV_EXCL_STOP } - else if (wrapper.IsMarkedForDelete()) + else if (wrapper.is_marked_for_delete()) { // Extract the session wrapper to destroy it outside the mutex lock auto node = pid_session_map_.extract(pid); @@ -338,10 +338,10 @@ void MessagePassingServer::FinishPreviousSessionWhileLocked( { const pid_t pid = it->first; SessionWrapper& wrapper = it->second; - wrapper.to_force_finish = true; - wrapper.EnqueueForDeleteWhileLocked(true); + wrapper.to_force_finish_ = true; + wrapper.enqueue_for_delete_while_locked(true); // if enqueued_ (i.e. not running) expedite the workload toward the front of the queue - if (wrapper.enqueued) + if (wrapper.enqueued_) { pid_t front_pid = work_queue_.front(); while (front_pid != pid) @@ -368,7 +368,7 @@ void MessagePassingServer::MessageCallback(score::message_passing::IServerConnec score::cpp::span message) { const pid_t pid = connection.GetClientIdentity().pid; - if (message.empty()) + if (message.size() < 1) { std::cerr << "MessagePassingServer: Empty message received from " << pid; return; @@ -419,6 +419,7 @@ void MessagePassingServer::OnConnectRequest(score::message_passing::IServerConne auto appid_sv = conn.GetAppId().GetStringView(); std::string appid{appid_sv.data(), appid_sv.size()}; + // check for timeout or exit request if (stop_source_.stop_requested()) { @@ -455,9 +456,9 @@ void MessagePassingServer::OnConnectRequest(score::message_passing::IServerConne { std::unique_lock lock(mutex_); auto emplace_result = pid_session_map_.emplace(pid, SessionWrapper{this, pid, std::move(session)}); - emplace_result.first->second.connection = &connection; + emplace_result.first->second.connection_ = &connection; // enqueue the tick to speed up processing connection - emplace_result.first->second.EnqueueTickWhileLocked(); + emplace_result.first->second.enqueue_tick_while_locked(); } } @@ -471,7 +472,7 @@ void MessagePassingServer::OnAcquireResponse(score::message_passing::IServerConn { auto& [key, session] = *found; std::ignore = key; - if (session.connection != &connection) + if (session.connection_ != &connection) { return; } @@ -487,9 +488,10 @@ void MessagePassingServer::OnAcquireResponse(score::message_passing::IServerConn // coverity[autosar_cpp14_m5_2_8_violation] score::cpp::span acq_span{static_cast(static_cast(&acq)), sizeof(acq)}; std::ignore = std::copy(message.begin(), message.end(), acq_span.begin()); - session.session->OnAcquireResponse(acq); + session.session_->on_acquire_response(acq); + session.acquire_in_flight_ = false; // enqueue the tick to speed up processing acquire response - session.EnqueueTickWhileLocked(); + session.enqueue_tick_while_locked(); } } @@ -502,16 +504,25 @@ bool MessagePassingServer::NotifyAcquireRequestWhileLocked(const pid_t pid) } auto& wrapper = found->second; - if (wrapper.connection == nullptr) + if (wrapper.connection_ == nullptr) { return false; } + if (wrapper.acquire_in_flight_) + { + return true; + } + constexpr std::array message{score::cpp::to_underlying(DatarouterMessageIdentifier::kAcquireRequest)}; - auto ret = wrapper.connection->Notify(message); + auto ret = wrapper.connection_->Notify(message); if (!ret) { - wrapper.EnqueueForDeleteWhileLocked(true); + wrapper.enqueue_for_delete_while_locked(true); + } + else + { + wrapper.acquire_in_flight_ = true; } return true; } @@ -536,7 +547,7 @@ void MessagePassingServer::NotifyAcquireRequestFailed(std::int32_t pid) return; // LCOV_EXCL_STOP } - found->second.EnqueueForDeleteWhileLocked(true); + found->second.enqueue_for_delete_while_locked(true); } bool MessagePassingServer::SessionHandle::AcquireRequest() const diff --git a/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp b/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp index 5e228581..89ee9fc5 100644 --- a/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp +++ b/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp @@ -428,6 +428,7 @@ TEST_F(MessagePassingServerFixture, TestOneConnectAcquireRelease) ::testing::Sequence seq; ExpectAcquireNotifyInSequence(DatarouterMessageIdentifier::kAcquireRequest, seq, connection_ptr); + session_map.at(kClienT0Pid).handle->AcquireRequest(); session_map.at(kClienT0Pid).handle->AcquireRequest(); EXPECT_EQ(acquire_response_count, 0); From dc033513b33d292c1fd4d7e8e7024ea550c2bc67 Mon Sep 17 00:00:00 2001 From: Raghavendra Maddikery Date: Sun, 15 Feb 2026 19:25:56 +0100 Subject: [PATCH 11/22] pas/logging: Add acquire watchdog teardown Track per-session acquire deadlines and miss counts and tear down sessions that do not reply with kAcquireResponse within the configured deadline for N consecutive misses. The watchdog runs off the existing mp_worker periodic tick to avoid per-session OS timers. GIT_ORIGIN_SPP_REV_ID: 84bb226ff8e017049e94740294234de06310a7f6 --- .../include/daemon/message_passing_server.h | 34 ++++++++---- .../src/daemon/message_passing_server.cpp | 22 +++++++- .../test_message_passing_server.cpp | 55 +++++++++++++++++-- .../data_router_message_client_test.cpp | 8 ++- 4 files changed, 100 insertions(+), 19 deletions(-) diff --git a/score/datarouter/include/daemon/message_passing_server.h b/score/datarouter/include/daemon/message_passing_server.h index 02f9b57e..b4ebacd5 100644 --- a/score/datarouter/include/daemon/message_passing_server.h +++ b/score/datarouter/include/daemon/message_passing_server.h @@ -28,6 +28,7 @@ #include "score/concurrency/interruptible_wait.h" #include +#include #include #include #include @@ -68,14 +69,22 @@ class IMessagePassingServerSessionWrapper class MessagePassingServer : public IMessagePassingServerSessionWrapper { public: - class SessionHandle : public daemon::ISessionHandle + struct AcquireWatchdogConfig { - public: - SessionHandle(pid_t pid, MessagePassingServer* server) - : daemon::ISessionHandle(), pid_(pid), server_(server) + AcquireWatchdogConfig(std::chrono::milliseconds deadline_in = std::chrono::milliseconds{1000}, + std::uint32_t max_misses_in = 3U) + : deadline(deadline_in), max_misses(max_misses_in) { } + std::chrono::milliseconds deadline; + std::uint32_t max_misses; + }; + class SessionHandle : public daemon::ISessionHandle + { + public: + SessionHandle(pid_t pid, MessagePassingServer* server) : daemon::ISessionHandle(), pid_(pid), server_(server) {} + bool AcquireRequest() const override; private: @@ -100,7 +109,8 @@ class MessagePassingServer : public IMessagePassingServerSessionWrapper explicit MessagePassingServer(SessionFactory factory, std::shared_ptr server_factory = nullptr, - std::shared_ptr client_factory = nullptr); + std::shared_ptr client_factory = nullptr, + AcquireWatchdogConfig watchdog_config = AcquireWatchdogConfig{}); ~MessagePassingServer() noexcept; // for unit test only. to keep rest of functions in private @@ -115,11 +125,11 @@ class MessagePassingServer : public IMessagePassingServerSessionWrapper void MessageCallback(score::message_passing::IServerConnection& connection, score::cpp::span message); void OnConnectRequest(score::message_passing::IServerConnection& connection, - const score::cpp::span message, - pid_t pid); + const score::cpp::span message, + pid_t pid); void OnAcquireResponse(score::message_passing::IServerConnection& connection, - const score::cpp::span message, - pid_t pid); + const score::cpp::span message, + pid_t pid); using timestamp_t = std::chrono::steady_clock::time_point; @@ -130,7 +140,7 @@ class MessagePassingServer : public IMessagePassingServerSessionWrapper pid_(pid), session_(std::move(session)), connection_(nullptr), - acquire_in_flight_(false), + acquire_in_flight_(false), enqueued_(false), running_(false), to_delete_(false), @@ -171,6 +181,8 @@ class MessagePassingServer : public IMessagePassingServerSessionWrapper score::message_passing::IServerConnection* connection_; bool acquire_in_flight_; + timestamp_t acquire_deadline_; + std::uint32_t acquire_miss_count_{0U}; bool enqueued_; bool running_; @@ -201,6 +213,8 @@ class MessagePassingServer : public IMessagePassingServerSessionWrapper std::shared_ptr server_factory_; std::shared_ptr client_factory_; + + AcquireWatchdogConfig watchdog_config_; }; } // namespace internal diff --git a/score/datarouter/src/daemon/message_passing_server.cpp b/score/datarouter/src/daemon/message_passing_server.cpp index 4e1b6e85..5f081ad7 100644 --- a/score/datarouter/src/daemon/message_passing_server.cpp +++ b/score/datarouter/src/daemon/message_passing_server.cpp @@ -99,7 +99,8 @@ void MessagePassingServer::SessionWrapper::enqueue_tick_while_locked() // coverity[autosar_cpp14_a3_1_1_violation] MessagePassingServer::MessagePassingServer(MessagePassingServer::SessionFactory factory, std::shared_ptr server_factory, - std::shared_ptr client_factory) + std::shared_ptr client_factory, + AcquireWatchdogConfig watchdog_config) : IMessagePassingServerSessionWrapper(), factory_{std::move(factory)}, mutex_{}, @@ -112,7 +113,8 @@ MessagePassingServer::MessagePassingServer(MessagePassingServer::SessionFactory server_cond_{}, session_finishing_{false}, server_factory_{server_factory}, - client_factory_{client_factory} + client_factory_{client_factory}, + watchdog_config_{watchdog_config} { worker_thread_ = score::cpp::jthread([this]() { RunWorkerThread(); @@ -246,6 +248,19 @@ void MessagePassingServer::RunWorkerThread() } else { + auto& wrapper = ps.second; + if (wrapper.acquire_in_flight_ && (wrapper.acquire_deadline_ != timestamp_t{}) && + (now >= wrapper.acquire_deadline_)) + { + wrapper.acquire_in_flight_ = false; + ++wrapper.acquire_miss_count_; + wrapper.acquire_deadline_ = timestamp_t{}; + if (wrapper.acquire_miss_count_ >= watchdog_config_.max_misses) + { + wrapper.enqueue_for_delete_while_locked(true); + continue; + } + } ps.second.enqueue_tick_while_locked(); } } @@ -490,6 +505,8 @@ void MessagePassingServer::OnAcquireResponse(score::message_passing::IServerConn std::ignore = std::copy(message.begin(), message.end(), acq_span.begin()); session.session_->on_acquire_response(acq); session.acquire_in_flight_ = false; + session.acquire_deadline_ = timestamp_t{}; + session.acquire_miss_count_ = 0U; // enqueue the tick to speed up processing acquire response session.enqueue_tick_while_locked(); } @@ -523,6 +540,7 @@ bool MessagePassingServer::NotifyAcquireRequestWhileLocked(const pid_t pid) else { wrapper.acquire_in_flight_ = true; + wrapper.acquire_deadline_ = timestamp_t::clock::now() + watchdog_config_.deadline; } return true; } diff --git a/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp b/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp index 89ee9fc5..741a5dfc 100644 --- a/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp +++ b/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp @@ -163,10 +163,10 @@ class MessagePassingServerFixture : public ::testing::Test score::cpp::pmr::get_default_resource()); server_mock = server_ptr.get(); - EXPECT_CALL( - *server_factory_mock, - Create(CompareServiceProtocol(ServiceProtocolConfig{"/logging.datarouter_recv", kMaxSendBytes, 0U, kMaxNotifyBytes}), - CompareServerConfig(server_config))) + EXPECT_CALL(*server_factory_mock, + Create(CompareServiceProtocol( + ServiceProtocolConfig{"/logging.datarouter_recv", kMaxSendBytes, 0U, kMaxNotifyBytes}), + CompareServerConfig(server_config))) .WillOnce(Return(ByMove(std::move(server_ptr)))); } @@ -254,6 +254,28 @@ class MessagePassingServerFixture : public ::testing::Test server.emplace(factory, server_factory_mock, client_factory_mock); } + void InstantiateServerWithWatchdog(MessagePassingServer::SessionFactory factory, + MessagePassingServer::AcquireWatchdogConfig watchdog_config) + { + EXPECT_CALL(*server_mock, + StartListening(Matcher(_), + Matcher(_), + Matcher(_), + Matcher(_))) + .WillOnce([this](score::message_passing::ConnectCallback con_callback, + score::message_passing::DisconnectCallback discon_callback, + score::message_passing::MessageCallback sn_callback, + score::message_passing::MessageCallback sn_rep_callback) { + this->connect_callback = std::move(con_callback); + this->disconnect_callback = std::move(discon_callback); + this->sent_callback = std::move(sn_callback); + this->sent_with_reply_callback = std::move(sn_rep_callback); + return score::cpp::expected_blank{}; + }); + + server.emplace(factory, server_factory_mock, client_factory_mock, watchdog_config); + } + auto CreateConnectMessageSample(const pid_t) { score::mw::log::detail::ConnectMessageFromClient msg; @@ -644,6 +666,31 @@ TEST_F(MessagePassingServerFixture, TestSamePidWhileQueued) EXPECT_EQ(destruct_count, 4); } +TEST_F(MessagePassingServerFixture, WatchdogShouldTearDownUnresponsiveClient) +{ + ExpectOurPidIsQueried(); + + MessagePassingServer::AcquireWatchdogConfig watchdog_config; + watchdog_config.deadline = std::chrono::milliseconds{0}; + watchdog_config.max_misses = 1U; + InstantiateServerWithWatchdog(GetCountingSessionFactory(), watchdog_config); + + auto* connection_ptr = ConnectClientAndSendConnectMessage(kClienT0Pid); + ::testing::Sequence seq; + ExpectAcquireNotifyInSequence(DatarouterMessageIdentifier::kAcquireRequest, seq, connection_ptr); + session_map.at(kClienT0Pid).handle->AcquireRequest(); + + { + std::unique_lock lock(map_mutex); + map_cond.wait(lock, [this]() { + return session_map.empty(); + }); + } + + ExpectServerDestruction(); + UninstantiateServer(); +} + TEST(MessagePassingServerTests, sessionWrapperCreateTest) { InSequence s; diff --git a/score/mw/log/detail/data_router/data_router_message_client_test.cpp b/score/mw/log/detail/data_router/data_router_message_client_test.cpp index 47a81959..81585940 100644 --- a/score/mw/log/detail/data_router/data_router_message_client_test.cpp +++ b/score/mw/log/detail/data_router/data_router_message_client_test.cpp @@ -725,7 +725,9 @@ TEST_F(DatarouterMessageClientFixture, AcquireRequestShouldSendExpectedAcquireRe TEST_F(DatarouterMessageClientFixture, AcquireNotifyShouldSendExpectedAcquireResponse) { RecordProperty("ASIL", "B"); - RecordProperty("Description", "Verifies that an acquire request delivered via NotifyCallback sends the expected acquire response."); + RecordProperty( + "Description", + "Verifies that an acquire request delivered via NotifyCallback sends the expected acquire response."); RecordProperty("TestType", "Interface test"); RecordProperty("DerivationTechnique", "Generation and analysis of equivalence classes"); @@ -796,8 +798,8 @@ TEST_F(DatarouterMessageClientFixture, InvalidNotifyShouldNotSendAcquireResponse const score::cpp::span empty_message{}; notify_callback(empty_message); - const std::array oversized_message{score::cpp::to_underlying(DatarouterMessageIdentifier::kAcquireRequest), - std::uint8_t{0U}}; + const std::array oversized_message{ + score::cpp::to_underlying(DatarouterMessageIdentifier::kAcquireRequest), std::uint8_t{0U}}; notify_callback(oversized_message); const std::array wrong_id_message{score::cpp::to_underlying(DatarouterMessageIdentifier::kConnect)}; From 95ef723b3b3494fda4caa43d9b825a95c232c458 Mon Sep 17 00:00:00 2001 From: Raghavendra Maddikery Date: Sun, 15 Feb 2026 19:28:11 +0100 Subject: [PATCH 12/22] pas/logging: Drop unused client factory - Remove the unused per-client IClientFactory dependency from the message passing server. - NotifyAcquireRequestFailed() is no longer used after the Notify based acquire refactoring. GIT_ORIGIN_SPP_REV_ID: 48aeeefb01510f1d9599c339796e11a13a422e30 --- .../include/daemon/message_passing_server.h | 10 +++------- .../src/daemon/message_passing_server.cpp | 19 ------------------- score/datarouter/src/daemon/socketserver.cpp | 4 +--- .../test_message_passing_server.cpp | 9 +++------ 4 files changed, 7 insertions(+), 35 deletions(-) diff --git a/score/datarouter/include/daemon/message_passing_server.h b/score/datarouter/include/daemon/message_passing_server.h index b4ebacd5..a012bdc3 100644 --- a/score/datarouter/include/daemon/message_passing_server.h +++ b/score/datarouter/include/daemon/message_passing_server.h @@ -18,7 +18,6 @@ #include "score/mw/log/detail/data_router/shared_memory/common.h" -#include "score/message_passing/i_client_factory.h" #include "score/message_passing/i_server_connection.h" #include "score/message_passing/i_server_factory.h" #include "score/mw/log/detail/logging_identifier.h" @@ -107,10 +106,9 @@ class MessagePassingServer : public IMessagePassingServerSessionWrapper const score::mw::log::detail::ConnectMessageFromClient&, score::cpp::pmr::unique_ptr)>; - explicit MessagePassingServer(SessionFactory factory, - std::shared_ptr server_factory = nullptr, - std::shared_ptr client_factory = nullptr, - AcquireWatchdogConfig watchdog_config = AcquireWatchdogConfig{}); + MessagePassingServer(SessionFactory factory, + std::shared_ptr server_factory = nullptr, + AcquireWatchdogConfig watchdog_config = AcquireWatchdogConfig{}); ~MessagePassingServer() noexcept; // for unit test only. to keep rest of functions in private @@ -121,7 +119,6 @@ class MessagePassingServer : public IMessagePassingServerSessionWrapper bool NotifyAcquireRequest(pid_t pid); bool NotifyAcquireRequestWhileLocked(pid_t pid); - void NotifyAcquireRequestFailed(std::int32_t pid); void MessageCallback(score::message_passing::IServerConnection& connection, score::cpp::span message); void OnConnectRequest(score::message_passing::IServerConnection& connection, @@ -212,7 +209,6 @@ class MessagePassingServer : public IMessagePassingServerSessionWrapper bool session_finishing_; std::shared_ptr server_factory_; - std::shared_ptr client_factory_; AcquireWatchdogConfig watchdog_config_; }; diff --git a/score/datarouter/src/daemon/message_passing_server.cpp b/score/datarouter/src/daemon/message_passing_server.cpp index 5f081ad7..8a12b0d3 100644 --- a/score/datarouter/src/daemon/message_passing_server.cpp +++ b/score/datarouter/src/daemon/message_passing_server.cpp @@ -99,7 +99,6 @@ void MessagePassingServer::SessionWrapper::enqueue_tick_while_locked() // coverity[autosar_cpp14_a3_1_1_violation] MessagePassingServer::MessagePassingServer(MessagePassingServer::SessionFactory factory, std::shared_ptr server_factory, - std::shared_ptr client_factory, AcquireWatchdogConfig watchdog_config) : IMessagePassingServerSessionWrapper(), factory_{std::move(factory)}, @@ -113,7 +112,6 @@ MessagePassingServer::MessagePassingServer(MessagePassingServer::SessionFactory server_cond_{}, session_finishing_{false}, server_factory_{server_factory}, - client_factory_{client_factory}, watchdog_config_{watchdog_config} { worker_thread_ = score::cpp::jthread([this]() { @@ -551,23 +549,6 @@ bool MessagePassingServer::NotifyAcquireRequest(const pid_t pid) return NotifyAcquireRequestWhileLocked(pid); } -void MessagePassingServer::NotifyAcquireRequestFailed(std::int32_t pid) -{ - std::lock_guard lock(mutex_); - const auto found = pid_session_map_.find(pid); - if (found == pid_session_map_.end()) - { - /* - Code will be hit only in case of pid changed, - but since this is private functions so it cannot be test. - */ - // LCOV_EXCL_START - return; - // LCOV_EXCL_STOP - } - found->second.enqueue_for_delete_while_locked(true); -} - bool MessagePassingServer::SessionHandle::AcquireRequest() const { if (server_ == nullptr) diff --git a/score/datarouter/src/daemon/socketserver.cpp b/score/datarouter/src/daemon/socketserver.cpp index 8d73e3c5..ddcba930 100644 --- a/score/datarouter/src/daemon/socketserver.cpp +++ b/score/datarouter/src/daemon/socketserver.cpp @@ -385,8 +385,6 @@ void SocketServer::DoWork(const std::atomic_bool& exit_requested, const bool no_ }; std::shared_ptr server_factory = std::make_shared(); - std::shared_ptr client_factory = - std::make_shared(/*server_factory_->GetEngine() Ticket-234313*/); /* Deviation from Rule A5-1-4: @@ -395,7 +393,7 @@ void SocketServer::DoWork(const std::atomic_bool& exit_requested, const bool no_ - mp_server does not exist inside any lambda. */ // coverity[autosar_cpp14_a5_1_4_violation: FALSE] - MessagePassingServer mp_server(mp_factory, std::move(server_factory), std::move(client_factory)); + MessagePassingServer mp_server(mp_factory, std::move(server_factory)); // Run main event loop RunEventLoop(exit_requested, router, *dlt_server, stats_logger); diff --git a/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp b/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp index 741a5dfc..b55166f4 100644 --- a/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp +++ b/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp @@ -14,7 +14,6 @@ #include "score/datarouter/include/daemon/message_passing_server.h" #include "score/message_passing/mock/client_connection_mock.h" -#include "score/message_passing/mock/client_factory_mock.h" #include "score/message_passing/mock/server_connection_mock.h" #include "score/message_passing/mock/server_factory_mock.h" #include "score/message_passing/mock/server_mock.h" @@ -155,7 +154,6 @@ class MessagePassingServerFixture : public ::testing::Test void SetUp() override { server_factory_mock = std::make_shared>(); - client_factory_mock = std::make_shared>(); const score::message_passing::IServerFactory::ServerConfig server_config{gKReceiverQueueMaxSize, 0U, 1U}; @@ -251,7 +249,7 @@ class MessagePassingServerFixture : public ::testing::Test }); // instantiate MessagePassingServer - server.emplace(factory, server_factory_mock, client_factory_mock); + server.emplace(factory, server_factory_mock); } void InstantiateServerWithWatchdog(MessagePassingServer::SessionFactory factory, @@ -273,7 +271,7 @@ class MessagePassingServerFixture : public ::testing::Test return score::cpp::expected_blank{}; }); - server.emplace(factory, server_factory_mock, client_factory_mock, watchdog_config); + server.emplace(factory, server_factory_mock, watchdog_config); } auto CreateConnectMessageSample(const pid_t) @@ -341,7 +339,6 @@ class MessagePassingServerFixture : public ::testing::Test } StrictMock<::score::message_passing::ServerMock>* server_mock{}; - std::shared_ptr> client_factory_mock; std::shared_ptr> server_factory_mock; ::score::os::MockGuard unistd_mock{}; @@ -412,7 +409,7 @@ TEST_F(MessagePassingServerFixture, TestFailedStartListening) return score::cpp::expected_blank{}; }); // instantiate MessagePassingServer - server.emplace(factory, server_factory_mock, client_factory_mock); + server.emplace(factory, server_factory_mock); ExpectServerDestruction(); UninstantiateServer(); From fa543a2fb37993dfdec2585731b6571bfaa83361 Mon Sep 17 00:00:00 2001 From: Raghavendra Maddikery Date: Sun, 15 Feb 2026 19:52:00 +0100 Subject: [PATCH 13/22] pas/logging: Close early-disconnect connect race - If a client disconnects while OnConnectRequest is creating a session outside mutex_, disconnect_callback previously could not clear connection_ because the PID was not yet in pid_session_map_. - This could lead to storing &connection after the framework already tore the connection down. - Track early disconnects per PID and refuse to store a connection pointer when a disconnect was observed. GIT_ORIGIN_SPP_REV_ID: 879672cc335db046848fe15571a0672c1616ee0a --- .../include/daemon/message_passing_server.h | 5 +++ .../src/daemon/message_passing_server.cpp | 34 +++++++++++++++---- 2 files changed, 33 insertions(+), 6 deletions(-) diff --git a/score/datarouter/include/daemon/message_passing_server.h b/score/datarouter/include/daemon/message_passing_server.h index a012bdc3..6c041c9c 100644 --- a/score/datarouter/include/daemon/message_passing_server.h +++ b/score/datarouter/include/daemon/message_passing_server.h @@ -35,6 +35,7 @@ #include #include #include +#include namespace score { @@ -203,6 +204,10 @@ class MessagePassingServer : public IMessagePassingServerSessionWrapper score::cpp::jthread worker_thread_; std::condition_variable worker_cond_; // to wake up worker thread std::unordered_map pid_session_map_; + // Tracks client PIDs that disconnected before a session could be created/emplaced. + // This closes a race where OnConnectRequest creates a session outside mutex_ while + // disconnect_callback may already have run and the connection object may be gone. + std::unordered_set disconnected_pids_; std::queue work_queue_; std::atomic workers_exit_; std::condition_variable server_cond_; // to wake up server thread diff --git a/score/datarouter/src/daemon/message_passing_server.cpp b/score/datarouter/src/daemon/message_passing_server.cpp index 8a12b0d3..f1b8089a 100644 --- a/score/datarouter/src/daemon/message_passing_server.cpp +++ b/score/datarouter/src/daemon/message_passing_server.cpp @@ -136,15 +136,22 @@ MessagePassingServer::MessagePassingServer(MessagePassingServer::SessionFactory MessagePassingConfig::kMaxQueuedNotifies}; receiver_ = server_factory_->Create(service_protocol_config, server_config); - auto connect_callback = [](score::message_passing::IServerConnection& connection) noexcept -> std::uintptr_t { + auto connect_callback = [this_ptr = this](score::message_passing::IServerConnection& connection) noexcept + -> std::uintptr_t { const pid_t client_pid = connection.GetClientIdentity().pid; + { + std::lock_guard lock(this_ptr->mutex_); + this_ptr->disconnected_pids_.erase(client_pid); + } return static_cast(client_pid); }; - auto disconnect_callback = [mutex_ptr = &mutex_, pid_session_map_ptr = &pid_session_map_]( - score::message_passing::IServerConnection& connection) noexcept { - std::unique_lock lock(*mutex_ptr); - const auto found = pid_session_map_ptr->find(connection.GetClientIdentity().pid); - if (found != pid_session_map_ptr->end()) + auto disconnect_callback = [this_ptr = this](score::message_passing::IServerConnection& connection) noexcept { + const pid_t client_pid = connection.GetClientIdentity().pid; + std::unique_lock lock(this_ptr->mutex_); + this_ptr->disconnected_pids_.insert(client_pid); + + const auto found = this_ptr->pid_session_map_.find(client_pid); + if (found != this_ptr->pid_session_map_.end()) { SessionWrapper& wrapper = found->second; wrapper.connection_ = nullptr; @@ -468,7 +475,22 @@ void MessagePassingServer::OnConnectRequest(score::message_passing::IServerConne if (session) { std::unique_lock lock(mutex_); + if (disconnected_pids_.find(pid) != disconnected_pids_.end()) + { + // Client disconnected before we could create/emplace the session. + // Do not store &connection (may be already destroyed by the framework). + return; + } + auto emplace_result = pid_session_map_.emplace(pid, SessionWrapper{this, pid, std::move(session)}); + if (!emplace_result.second) + { + // Existing session for this PID is still present; do not overwrite it. + // The reconnect path is handled by disconnect_callback + worker-thread teardown. + return; + } + + // connection_ points to framework-owned object. It is cleared in disconnect_callback. emplace_result.first->second.connection_ = &connection; // enqueue the tick to speed up processing connection emplace_result.first->second.enqueue_tick_while_locked(); From 46cee1ac097e2750abfbdca7ff0d9b3f29be1b23 Mon Sep 17 00:00:00 2001 From: Raghavendra Maddikery Date: Sun, 15 Feb 2026 19:53:18 +0100 Subject: [PATCH 14/22] logging: Use optional acquire watchdog deadline Avoid relying on implementation-defined steady_clock epochs and make the deadline not set state explicit. GIT_ORIGIN_SPP_REV_ID: 5f3da32556784c74bde3d52de069446907633c3d --- score/datarouter/include/daemon/message_passing_server.h | 2 +- score/datarouter/src/daemon/message_passing_server.cpp | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/score/datarouter/include/daemon/message_passing_server.h b/score/datarouter/include/daemon/message_passing_server.h index 6c041c9c..6f3a4b33 100644 --- a/score/datarouter/include/daemon/message_passing_server.h +++ b/score/datarouter/include/daemon/message_passing_server.h @@ -179,7 +179,7 @@ class MessagePassingServer : public IMessagePassingServerSessionWrapper score::message_passing::IServerConnection* connection_; bool acquire_in_flight_; - timestamp_t acquire_deadline_; + std::optional acquire_deadline_; std::uint32_t acquire_miss_count_{0U}; bool enqueued_; diff --git a/score/datarouter/src/daemon/message_passing_server.cpp b/score/datarouter/src/daemon/message_passing_server.cpp index f1b8089a..caabccab 100644 --- a/score/datarouter/src/daemon/message_passing_server.cpp +++ b/score/datarouter/src/daemon/message_passing_server.cpp @@ -254,12 +254,12 @@ void MessagePassingServer::RunWorkerThread() else { auto& wrapper = ps.second; - if (wrapper.acquire_in_flight_ && (wrapper.acquire_deadline_ != timestamp_t{}) && - (now >= wrapper.acquire_deadline_)) + if (wrapper.acquire_in_flight_ && wrapper.acquire_deadline_.has_value() && + (now >= *wrapper.acquire_deadline_)) { wrapper.acquire_in_flight_ = false; ++wrapper.acquire_miss_count_; - wrapper.acquire_deadline_ = timestamp_t{}; + wrapper.acquire_deadline_.reset(); if (wrapper.acquire_miss_count_ >= watchdog_config_.max_misses) { wrapper.enqueue_for_delete_while_locked(true); @@ -525,7 +525,7 @@ void MessagePassingServer::OnAcquireResponse(score::message_passing::IServerConn std::ignore = std::copy(message.begin(), message.end(), acq_span.begin()); session.session_->on_acquire_response(acq); session.acquire_in_flight_ = false; - session.acquire_deadline_ = timestamp_t{}; + session.acquire_deadline_.reset(); session.acquire_miss_count_ = 0U; // enqueue the tick to speed up processing acquire response session.enqueue_tick_while_locked(); From 761601dafb8b4a694bf1191f9a75a15d67fff7a7 Mon Sep 17 00:00:00 2001 From: Raghavendra Maddikery Date: Sun, 15 Feb 2026 19:54:12 +0100 Subject: [PATCH 15/22] pas/logging: Verify watchdog miss reset Add a unit test that verifies the acquire watchdog. After an initial missed deadline, a late but valid AcquireResponse must reset the miss counter so the client is only torn down after the configured number of subsequent misses. GIT_ORIGIN_SPP_REV_ID: 4667d0ea0811633d2fe10b57e1293aa0810be185 --- .../test_message_passing_server.cpp | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp b/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp index b55166f4..ed07c804 100644 --- a/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp +++ b/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp @@ -688,6 +688,52 @@ TEST_F(MessagePassingServerFixture, WatchdogShouldTearDownUnresponsiveClient) UninstantiateServer(); } +TEST_F(MessagePassingServerFixture, WatchdogMissCountShouldResetOnValidResponse) +{ + ExpectOurPidIsQueried(); + + MessagePassingServer::AcquireWatchdogConfig watchdog_config; + watchdog_config.deadline = std::chrono::milliseconds{0}; + watchdog_config.max_misses = 2U; + InstantiateServerWithWatchdog(GetCountingSessionFactory(), watchdog_config); + + auto* connection_ptr = ConnectClientAndSendConnectMessage(kClienT0Pid); + + ::testing::Sequence seq; + ExpectAcquireNotifyInSequence(DatarouterMessageIdentifier::kAcquireRequest, seq, connection_ptr); + ExpectAcquireNotifyInSequence(DatarouterMessageIdentifier::kAcquireRequest, seq, connection_ptr); + ExpectAcquireNotifyInSequence(DatarouterMessageIdentifier::kAcquireRequest, seq, connection_ptr); + + // 1) First acquire request is missed by the client -> miss_count becomes 1. + session_map.at(kClienT0Pid).handle->AcquireRequest(); + using namespace std::chrono_literals; + std::this_thread::sleep_for(150ms); + + // 2) Late valid response arrives -> miss_count should reset to 0. + score::mw::log::detail::ReadAcquireResult acquire_result{0U}; + std::array response{}; + response[0] = score::cpp::to_underlying(DatarouterMessageIdentifier::kAcquireResponse); + std::memcpy(&response[1], &acquire_result, sizeof(acquire_result)); + sent_callback(*connection_ptr, response); + + // 3) One more missed acquire should NOT tear down (max_misses = 2, miss_count should be 1). + session_map.at(kClienT0Pid).handle->AcquireRequest(); + std::this_thread::sleep_for(150ms); + EXPECT_FALSE(session_map.empty()); + + // 4) Second miss after the reset should now tear down. + session_map.at(kClienT0Pid).handle->AcquireRequest(); + { + std::unique_lock lock(map_mutex); + map_cond.wait(lock, [this]() { + return session_map.empty(); + }); + } + + ExpectServerDestruction(); + UninstantiateServer(); +} + TEST(MessagePassingServerTests, sessionWrapperCreateTest) { InSequence s; From 0d7a07a297f3285e29121c0056e5652d99a5da47 Mon Sep 17 00:00:00 2001 From: Raghavendra Maddikery Date: Sun, 15 Feb 2026 19:55:22 +0100 Subject: [PATCH 16/22] pas/logging: test stale connection - test ensuring that OnAcquireResponse ignores messages coming from a stale server connection after a client reconnects with the same PID. GIT_ORIGIN_SPP_REV_ID: 927c11ac7b4fd20cbd8e91930b25b4505f08788d --- .../test_message_passing_server.cpp | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp b/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp index ed07c804..8cf1936a 100644 --- a/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp +++ b/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp @@ -300,6 +300,9 @@ class MessagePassingServerFixture : public ::testing::Test .Times(AnyNumber()) .WillRepeatedly(ReturnRef(connection_identities_.at(pid))); + // Mirror production behavior: connect_callback runs before any messages. + std::ignore = connect_callback(*connection_ptr); + auto message = CreateConnectMessageSample(pid); sent_callback(*connection_ptr, message); @@ -550,6 +553,48 @@ TEST_F(MessagePassingServerFixture, TestTripleConnectSamePid) EXPECT_EQ(destruct_count, 3); } +TEST_F(MessagePassingServerFixture, StaleConnectionAcquireResponseShouldBeIgnored) +{ + ExpectOurPidIsQueried(); + InstantiateServer(GetCountingSessionFactory()); + + // Connect client (connection0) and create session. + auto* connection0_ptr = ConnectClientAndSendConnectMessage(kClienT0Pid); + + // Disconnect the client and wait until its session is torn down. + this->disconnect_callback(*connection0_ptr); + { + std::unique_lock lock(map_mutex); + map_cond.wait(lock, [this]() { + return session_map.empty(); + }); + } + + // Keep the old connection object alive, but free the PID slot for the new connection. + auto old_connection = std::move(connections_.at(kClienT0Pid)); + connections_.erase(kClienT0Pid); + auto* stale_connection_ptr = old_connection.get(); + + // Reconnect client with the same PID (connection1) and create a new session. + auto* connection1_ptr = ConnectClientAndSendConnectMessage(kClienT0Pid); + + score::mw::log::detail::ReadAcquireResult acquire_result{0U}; + std::array response{}; + response[0] = score::cpp::to_underlying(DatarouterMessageIdentifier::kAcquireResponse); + std::memcpy(&response[1], &acquire_result, sizeof(acquire_result)); + + // Response on stale connection must be ignored. + sent_callback(*stale_connection_ptr, response); + EXPECT_EQ(acquire_response_count, 0); + + // Response on the current connection must be accepted. + sent_callback(*connection1_ptr, response); + EXPECT_EQ(acquire_response_count, 1); + + ExpectServerDestruction(); + UninstantiateServer(); +} + TEST_F(MessagePassingServerFixture, TestSamePidWhileRunning) { From d61f1c2eb2198e94d6ec932c4ab7f6e5cbf2ee35 Mon Sep 17 00:00:00 2001 From: Raghavendra Maddikery Date: Fri, 20 Feb 2026 12:02:45 +0100 Subject: [PATCH 17/22] Fix client teardown after datarouter disconnect - Now that datarouter does not connect to client's IServer, the disconnect callback is never called. - Addds RequestInternatShutdown when the state callback kStopped is received. GIT_ORIGIN_SPP_REV_ID: fd475ea00de2bc097c8ae6f128344bf8641bddc1 --- .../include/daemon/message_passing_server.h | 24 +- .../src/daemon/message_passing_server.cpp | 6 +- .../test_message_passing_server.cpp | 2 +- .../data_router_message_client_impl.cpp | 5 + .../data_router_message_client_test.cpp | 279 ++++-------------- .../data_router/message_passing_config.h | 4 +- 6 files changed, 81 insertions(+), 239 deletions(-) diff --git a/score/datarouter/include/daemon/message_passing_server.h b/score/datarouter/include/daemon/message_passing_server.h index 6f3a4b33..22112452 100644 --- a/score/datarouter/include/daemon/message_passing_server.h +++ b/score/datarouter/include/daemon/message_passing_server.h @@ -133,17 +133,19 @@ class MessagePassingServer : public IMessagePassingServerSessionWrapper struct SessionWrapper { - SessionWrapper(IMessagePassingServerSessionWrapper* server, pid_t pid, std::unique_ptr session) - : server_(server), - pid_(pid), - session_(std::move(session)), - connection_(nullptr), - acquire_in_flight_(false), - enqueued_(false), - running_(false), - to_delete_(false), - closed_by_peer_(false), - to_force_finish_(false) + SessionWrapper(IMessagePassingServerSessionWrapper* message_passing_server, + pid_t client_pid, + std::unique_ptr message_passing_session) + : server(message_passing_server), + pid(client_pid), + session(std::move(message_passing_session)), + connection(nullptr), + acquire_in_flight(false), + enqueued(false), + running(false), + to_delete(false), + closed_by_peer(false), + to_force_finish(false) { } diff --git a/score/datarouter/src/daemon/message_passing_server.cpp b/score/datarouter/src/daemon/message_passing_server.cpp index caabccab..445daaa8 100644 --- a/score/datarouter/src/daemon/message_passing_server.cpp +++ b/score/datarouter/src/daemon/message_passing_server.cpp @@ -19,6 +19,8 @@ #include "score/memory.hpp" #include +#include +#include #include #include #include @@ -136,8 +138,8 @@ MessagePassingServer::MessagePassingServer(MessagePassingServer::SessionFactory MessagePassingConfig::kMaxQueuedNotifies}; receiver_ = server_factory_->Create(service_protocol_config, server_config); - auto connect_callback = [this_ptr = this](score::message_passing::IServerConnection& connection) noexcept - -> std::uintptr_t { + auto connect_callback = [this_ptr = + this](score::message_passing::IServerConnection& connection) noexcept -> std::uintptr_t { const pid_t client_pid = connection.GetClientIdentity().pid; { std::lock_guard lock(this_ptr->mutex_); diff --git a/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp b/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp index 8cf1936a..453484c5 100644 --- a/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp +++ b/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp @@ -81,7 +81,7 @@ constexpr pid_t kClienT2Pid = 1002; constexpr std::uint32_t kMaxSendBytes{17U}; constexpr std::uint32_t kMaxNotifyBytes{1U}; -std::uint32_t gKReceiverQueueMaxSize = 0; +std::uint32_t gKReceiverQueueMaxSize = 1; class MockSession : public MessagePassingServer::ISession { diff --git a/score/mw/log/detail/data_router/data_router_message_client_impl.cpp b/score/mw/log/detail/data_router/data_router_message_client_impl.cpp index cd1058d3..8e82e41f 100644 --- a/score/mw/log/detail/data_router/data_router_message_client_impl.cpp +++ b/score/mw/log/detail/data_router/data_router_message_client_impl.cpp @@ -361,6 +361,11 @@ score::cpp::expected_blank DatarouterMessageClientImpl::Create sender_state_ = new_state; } state_condition_.notify_all(); + + if (new_state == score::message_passing::IClientConnection::State::kStopped) + { + RequestInternalShutdown(); + } }; auto notify_callback = [this](score::cpp::span message) noexcept { diff --git a/score/mw/log/detail/data_router/data_router_message_client_test.cpp b/score/mw/log/detail/data_router/data_router_message_client_test.cpp index 81585940..dcc3ef6a 100644 --- a/score/mw/log/detail/data_router/data_router_message_client_test.cpp +++ b/score/mw/log/detail/data_router/data_router_message_client_test.cpp @@ -106,7 +106,7 @@ constexpr auto kLoggerThreadName = "logger"; constexpr uid_t kDatarouterDummyUid = 111; constexpr std::uint32_t kMaxSendBytes{17U}; constexpr std::uint32_t kMaxNotifyBytes{1U}; -constexpr std::uint32_t kMaxNumberMessagesInReceiverQueue{0UL}; +constexpr std::uint32_t kMaxNumberMessagesInReceiverQueue{1UL}; class DatarouterMessageClientFixture : public ::testing::Test { @@ -1077,17 +1077,19 @@ TEST_F(DatarouterMessageClientFixture, FailedToEmptySignalSet) TEST_F(DatarouterMessageClientFixture, ConnectToDatarouterShouldReportErrorAndShutdownWhenCreateSenderFails) { - + RecordProperty("ASIL", "B"); + RecordProperty("Description", + "Verifies ConnectToDatarouter reports an error and shuts down when CreateSender fails."); + RecordProperty("TestType", "Interface test"); + RecordProperty("DerivationTechnique", "Generation and analysis of equivalence classes"); testing::InSequence order_matters; - auto* receiver_ptr = ExpectReceiverCreated(); ExpectBlockTerminationSignalPass(); ExpectSetLoggerThreadName(); - // CreateClient returns nullptr, causing CreateSender to return nullopt - const score::message_passing::ServiceProtocolConfig service_protocol_config{ - kDatarouterReceiverIdentifier, kMaxSendBytes, 0U, 0U}; + const score::message_passing::ServiceProtocolConfig service_protocol_config{ + kDatarouterReceiverIdentifier, kMaxSendBytes, 0U, kMaxNotifyBytes}; const score::message_passing::IClientFactory::ClientConfig client_config{0, 10, false, true, false}; EXPECT_CALL(*message_passing_factory_, @@ -1095,275 +1097,106 @@ TEST_F(DatarouterMessageClientFixture, ConnectToDatarouterShouldReportErrorAndSh .WillOnce(Return(ByMove(nullptr))); ExpectUnlinkMwsrWriterFile(); - ExpectServerDestruction(receiver_ptr); - client_->SetupReceiver(); client_->ConnectToDatarouter(); -} - -TEST_F(DatarouterMessageClientFixture, ConnectCallbackBlocksSignalAndReturnsClientPid) -{ - constexpr pid_t kExpectedPid = 42; - - auto* receiver_ptr = ExpectReceiverCreated(); - EXPECT_CALL(*signal_mock_, SigEmptySet(_)).Times(2).WillRepeatedly(Return(0)); - EXPECT_CALL(*signal_mock_, SigAddSet(_, SIGTERM)).Times(2).WillRepeatedly(Return(0)); - EXPECT_CALL(*signal_mock_, PthreadSigMask(_, _)).Times(2).WillRepeatedly(Return(0)); - ExpectSetLoggerThreadName(); - - sender_mock_in_transit_ = score::cpp::pmr::make_unique>( - score::cpp::pmr::get_default_resource()); - auto* sender_ptr = sender_mock_in_transit_.get(); - - const score::message_passing::ServiceProtocolConfig service_protocol_config{ - kDatarouterReceiverIdentifier, kMaxSendBytes, 0U, 0U}; - const score::message_passing::IClientFactory::ClientConfig client_config{0, 10, false, true, false}; - - EXPECT_CALL(*message_passing_factory_, - CreateClient(CompareServiceProtocol(service_protocol_config), CompareClientConfig(client_config))) - .WillOnce(Return(ByMove(std::move(sender_mock_in_transit_)))); - - EXPECT_CALL(*sender_ptr, - Start(Matcher(_), - Matcher(_))) - .WillOnce([](score::message_passing::IClientConnection::StateCallback state_callback, - score::message_passing::IClientConnection::NotifyCallback) { - state_callback(score::message_passing::IClientConnection::State::kReady); - }); - - testing::StrictMock server_conn_mock; - - const score::message_passing::ClientIdentity expected_identity{kExpectedPid, 0, 0}; - EXPECT_CALL(server_conn_mock, GetClientIdentity()).WillOnce(ReturnRef(expected_identity)); - - // ExpectBlockTerminationSignalPass(); - - EXPECT_CALL(*receiver_ptr, - StartListening(Matcher(_), - Matcher(_), - Matcher(_), - Matcher(_))) - .WillOnce([&server_conn_mock](score::message_passing::ConnectCallback connect_cb, - score::message_passing::DisconnectCallback, - score::message_passing::MessageCallback, - score::message_passing::MessageCallback) { - auto result = connect_cb(server_conn_mock); - - EXPECT_TRUE(result.has_value()); - EXPECT_EQ(std::get(result.value()), static_cast(kExpectedPid)); - return score::cpp::expected_blank{}; - }); - - ExpectSendConnectMessage(sender_ptr); - - ExpectServerDestruction(receiver_ptr); - ExpectClientDestruction(sender_ptr); - - client_->SetupReceiver(); - client_->ConnectToDatarouter(); + EXPECT_TRUE(stop_source_.stop_requested()); } -TEST_F(DatarouterMessageClientFixture, DisconnectCallbackRequestsInternalShutdown) +TEST_F(DatarouterMessageClientFixture, CreateSenderFailsWhenFactoryReturnsNullptr) { - auto* receiver_ptr = ExpectReceiverCreated(); - ExpectBlockTerminationSignalPass(); - ExpectSetLoggerThreadName(); - - sender_mock_in_transit_ = score::cpp::pmr::make_unique>( - score::cpp::pmr::get_default_resource()); - auto* sender_ptr = sender_mock_in_transit_.get(); + RecordProperty("ASIL", "B"); + RecordProperty("Description", + "Verifies CreateSender reports an error and returns unexpected when the factory returns nullptr."); + RecordProperty("TestType", "Interface test"); + RecordProperty("DerivationTechnique", "Generation and analysis of equivalence classes"); const score::message_passing::ServiceProtocolConfig service_protocol_config{ - kDatarouterReceiverIdentifier, kMaxSendBytes, 0U, 0U}; + kDatarouterReceiverIdentifier, kMaxSendBytes, 0U, kMaxNotifyBytes}; const score::message_passing::IClientFactory::ClientConfig client_config{0, 10, false, true, false}; EXPECT_CALL(*message_passing_factory_, CreateClient(CompareServiceProtocol(service_protocol_config), CompareClientConfig(client_config))) - .WillOnce(Return(ByMove(std::move(sender_mock_in_transit_)))); - - EXPECT_CALL(*sender_ptr, - Start(Matcher(_), - Matcher(_))) - .WillOnce([](score::message_passing::IClientConnection::StateCallback state_callback, - score::message_passing::IClientConnection::NotifyCallback) { - state_callback(score::message_passing::IClientConnection::State::kReady); - }); - - testing::StrictMock server_conn_mock; - - EXPECT_CALL(*receiver_ptr, - StartListening(Matcher(_), - Matcher(_), - Matcher(_), - Matcher(_))) - .WillOnce([&server_conn_mock](score::message_passing::ConnectCallback, - score::message_passing::DisconnectCallback disconnect_cb, - score::message_passing::MessageCallback, - score::message_passing::MessageCallback) { - disconnect_cb(server_conn_mock); - return score::cpp::expected_blank{}; - }); - - ExpectUnlinkMwsrWriterFile(); - - ExpectServerDestruction(receiver_ptr); - ExpectClientDestruction(sender_ptr); + .WillOnce(Return(ByMove(nullptr))); + const auto result = client_->CreateSender(); + ASSERT_FALSE(result.has_value()); - client_->SetupReceiver(); - client_->ConnectToDatarouter(); - EXPECT_TRUE(stop_source_.stop_requested()); } -TEST_F(DatarouterMessageClientFixture, ReceivedSendMessageWithReplyCallbackDoesNothing) +TEST_F(DatarouterMessageClientFixture, ConnectToDatarouterShouldReportErrorAndShutdownWhenCreateSenderFails) { - auto* receiver_ptr = ExpectReceiverCreated(); + RecordProperty("ASIL", "B"); + RecordProperty("Description", + "Verifies ConnectToDatarouter reports an error and shuts down when CreateSender fails."); + RecordProperty("TestType", "Interface test"); + RecordProperty("DerivationTechnique", "Generation and analysis of equivalence classes"); + + testing::InSequence order_matters; ExpectBlockTerminationSignalPass(); ExpectSetLoggerThreadName(); - sender_mock_in_transit_ = score::cpp::pmr::make_unique>( - score::cpp::pmr::get_default_resource()); - auto* sender_ptr = sender_mock_in_transit_.get(); - const score::message_passing::ServiceProtocolConfig service_protocol_config{ - kDatarouterReceiverIdentifier, kMaxSendBytes, 0U, 0U}; + kDatarouterReceiverIdentifier, kMaxSendBytes, 0U, kMaxNotifyBytes}; const score::message_passing::IClientFactory::ClientConfig client_config{0, 10, false, true, false}; EXPECT_CALL(*message_passing_factory_, CreateClient(CompareServiceProtocol(service_protocol_config), CompareClientConfig(client_config))) - .WillOnce(Return(ByMove(std::move(sender_mock_in_transit_)))); - - EXPECT_CALL(*sender_ptr, - Start(Matcher(_), - Matcher(_))) - .WillOnce([](score::message_passing::IClientConnection::StateCallback state_callback, - score::message_passing::IClientConnection::NotifyCallback) { - state_callback(score::message_passing::IClientConnection::State::kReady); - }); - - EXPECT_CALL(*sender_ptr, Send(Matcher>(_))) - .WillOnce(Return(score::cpp::expected_blank{})); - - testing::StrictMock server_conn_mock; - - EXPECT_CALL(*receiver_ptr, - StartListening(Matcher(_), - Matcher(_), - Matcher(_), - Matcher(_))) - .WillOnce([&server_conn_mock](score::message_passing::ConnectCallback, - score::message_passing::DisconnectCallback, - score::message_passing::MessageCallback, - score::message_passing::MessageCallback send_with_reply_cb) { - const std::array message_data{0x01, 0x02, 0x03, 0x04}; - const score::cpp::span message_span{message_data}; - - const auto result = send_with_reply_cb(server_conn_mock, message_span); - static_cast(result); - - return score::cpp::expected_blank{}; - }); + .WillOnce(Return(ByMove(nullptr))); ExpectUnlinkMwsrWriterFile(); - ExpectServerDestruction(receiver_ptr); - ExpectClientDestruction(sender_ptr); - client_->SetupReceiver(); client_->ConnectToDatarouter(); + + EXPECT_TRUE(stop_source_.stop_requested()); } -TEST_F(DatarouterMessageClientFixture, StopRequestedDuringReceiverStartReportsShutdownDuringInitialization) +TEST_F(DatarouterMessageClientFixture, CreateSenderFailsWhenFactoryReturnsNullptr) { - auto* receiver_ptr = ExpectReceiverCreated(); - - ExpectBlockTerminationSignalPass(); - ExpectSetLoggerThreadName(); + RecordProperty("ASIL", "B"); + RecordProperty("Description", + "Verifies CreateSender reports an error and returns unexpected when the factory returns nullptr."); + RecordProperty("TestType", "Interface test"); + RecordProperty("DerivationTechnique", "Error guessing based on knowledge or experience"); - sender_mock_in_transit_ = score::cpp::pmr::make_unique>( - score::cpp::pmr::get_default_resource()); - auto* sender_ptr = sender_mock_in_transit_.get(); + testing::InSequence order_matters; const score::message_passing::ServiceProtocolConfig service_protocol_config{ - kDatarouterReceiverIdentifier, kMaxSendBytes, 0U, 0U}; + kDatarouterReceiverIdentifier, kMaxSendBytes, 0U, kMaxNotifyBytes}; const score::message_passing::IClientFactory::ClientConfig client_config{0, 10, false, true, false}; + // Factory returns nullptr — this is the key to hitting the branch. EXPECT_CALL(*message_passing_factory_, CreateClient(CompareServiceProtocol(service_protocol_config), CompareClientConfig(client_config))) - .WillOnce(Return(ByMove(std::move(sender_mock_in_transit_)))); - - EXPECT_CALL(*sender_ptr, - Start(Matcher(_), - Matcher(_))) - .WillOnce([](score::message_passing::IClientConnection::StateCallback state_callback, - score::message_passing::IClientConnection::NotifyCallback) { - state_callback(score::message_passing::IClientConnection::State::kReady); - }); - - // SendConnectMessage must never be reached - EXPECT_CALL(*sender_ptr, Send(Matcher>(_))).Times(0); - - testing::StrictMock server_conn_mock; - - EXPECT_CALL(*receiver_ptr, - StartListening(Matcher(_), - Matcher(_), - Matcher(_), - Matcher(_))) - .WillOnce([&server_conn_mock](score::message_passing::ConnectCallback, - score::message_passing::DisconnectCallback disconnect_cb, - score::message_passing::MessageCallback, - score::message_passing::MessageCallback) { - // Trigger disconnect during StartListening — this calls RequestInternalShutdown(), - // setting stop_source_ to stop_requested AFTER the earlier stop check in - // ConnectToDatarouter() has already passed. - disconnect_cb(server_conn_mock); - - return score::cpp::expected_blank{}; - }); - - ExpectUnlinkMwsrWriterFile(); - ExpectServerDestruction(receiver_ptr); - ExpectClientDestruction(sender_ptr); + .WillOnce(Return(ByMove(nullptr))); - client_->SetupReceiver(); - client_->ConnectToDatarouter(); + const auto result = client_->CreateSender(); - EXPECT_TRUE(stop_source_.stop_requested()); + ASSERT_FALSE(result.has_value()); } - -TEST_F(DatarouterMessageClientFixture, CreateSenderFailsWhenFactoryReturnsNullptr) +TEST_F(DatarouterMessageClientFixture, StateCallbackKStoppedShouldTriggerInternalShutdown) { - auto* receiver_ptr = ExpectReceiverCreated(); + RecordProperty("ASIL", "QM"); + RecordProperty("Description", "Verifies dataRouter termination triggers internal shutdown on client side"); + RecordProperty("TestType", "Verification of the control flow and data flow"); + RecordProperty("DerivationTechnique", "Error guessing based on knowledge or experience"); - ExpectBlockTerminationSignalPass(); - ExpectSetLoggerThreadName(); + testing::InSequence order_matters; - const score::message_passing::ServiceProtocolConfig service_protocol_config{ - kDatarouterReceiverIdentifier, kMaxSendBytes, 0U, 0U}; - const score::message_passing::IClientFactory::ClientConfig client_config{0, 10, false, true, false}; + score::message_passing::ClientConnectionMock* sender_ptr{}; + score::message_passing::IClientConnection::StateCallback state_callback; - // Factory returns nullptr — this is the key to hitting the branch - EXPECT_CALL(*message_passing_factory_, - CreateClient(CompareServiceProtocol(service_protocol_config), CompareClientConfig(client_config))) - .WillOnce(Return(ByMove(nullptr))); + ExpectSenderCreationSequence(&sender_ptr, &state_callback); - // Since CreateSender fails, Start should never be called - // Since CreateSender fails, StartListening should never be called - EXPECT_CALL(*receiver_ptr, - StartListening(Matcher(_), - Matcher(_), - Matcher(_), - Matcher(_))) - .Times(0); + ExecuteCreateSenderSequence(&state_callback); ExpectUnlinkMwsrWriterFile(); - ExpectServerDestruction(receiver_ptr); - client_->SetupReceiver(); - client_->ConnectToDatarouter(); + state_callback(score::message_passing::IClientConnection::State::kStopped); EXPECT_TRUE(stop_source_.stop_requested()); + + ExpectClientDestruction(sender_ptr); } } // namespace diff --git a/score/mw/log/detail/data_router/message_passing_config.h b/score/mw/log/detail/data_router/message_passing_config.h index 5ec6ddfb..17201377 100644 --- a/score/mw/log/detail/data_router/message_passing_config.h +++ b/score/mw/log/detail/data_router/message_passing_config.h @@ -32,8 +32,8 @@ struct MessagePassingConfig static constexpr std::uint32_t kMaxMessageSize{17U}; /// \brief Maximum number of messages in receiver queue. - /// \note Value not used at the moment of integration with message passing library. May change in the future. - static constexpr std::uint32_t kMaxReceiverQueueSize{0U}; + /// \note Value set based on the recommendation in message_passing/i_server_factory.h + static constexpr std::uint32_t kMaxReceiverQueueSize{1U}; /// \brief Number of pre-allocated connections. static constexpr std::uint32_t kPreAllocConnections{0U}; From 9dc3d5da947767617bff9dbc7bc4554ba24fb6fa Mon Sep 17 00:00:00 2001 From: Raghavendra Maddikery Date: Sun, 15 Feb 2026 19:56:47 +0100 Subject: [PATCH 18/22] pas/logging: Fix watchdog config formatting GIT_ORIGIN_SPP_REV_ID: 7d681ddd0dae1e8b63ace4e3fccab211a2773380 --- .../include/daemon/message_passing_server.h | 66 ++++---- .../src/daemon/message_passing_server.cpp | 150 +++++++++--------- .../src/daemon/persistentlogging_config.cpp | 15 +- .../test_message_passing_server.cpp | 41 +++-- 4 files changed, 146 insertions(+), 126 deletions(-) diff --git a/score/datarouter/include/daemon/message_passing_server.h b/score/datarouter/include/daemon/message_passing_server.h index 22112452..6cf11485 100644 --- a/score/datarouter/include/daemon/message_passing_server.h +++ b/score/datarouter/include/daemon/message_passing_server.h @@ -95,10 +95,10 @@ class MessagePassingServer : public IMessagePassingServerSessionWrapper class ISession { public: - virtual bool tick() = 0; - virtual void on_acquire_response(const score::mw::log::detail::ReadAcquireResult&) = 0; - virtual void on_closed_by_peer() = 0; - virtual bool is_source_closed() = 0; + virtual bool Tick() = 0; + virtual void OnAcquireResponse(const score::mw::log::detail::ReadAcquireResult&) = 0; + virtual void OnClosedByPeer() = 0; + virtual bool IsSourceClosed() = 0; virtual ~ISession() = default; }; @@ -129,7 +129,7 @@ class MessagePassingServer : public IMessagePassingServerSessionWrapper const score::cpp::span message, pid_t pid); - using timestamp_t = std::chrono::steady_clock::time_point; + using TimestampT = std::chrono::steady_clock::time_point; struct SessionWrapper { @@ -149,46 +149,46 @@ class MessagePassingServer : public IMessagePassingServerSessionWrapper { } - void enqueue_for_delete_while_locked(bool by_peer = false); - bool is_marked_for_delete() + void EnqueueForDeleteWhileLocked(bool by_peer = false); + bool IsMarkedForDelete() const { - return to_delete_; + return to_delete; } - bool get_reset_closed_by_peer() + bool GetResetClosedByPeer() { - bool by_peer = closed_by_peer_; - closed_by_peer_ = false; + bool by_peer = closed_by_peer; + closed_by_peer = false; return by_peer; } - bool tick_at_worker_thread(); - void notify_closed_by_peer(); + bool TickAtWorkerThread() const; + void NotifyClosedByPeer() const; - void set_running_while_locked(); - bool reset_running_while_locked(bool requeue); + void SetRunningWhileLocked(); + bool ResetRunningWhileLocked(bool requeue); - void enqueue_tick_while_locked(); + void EnqueueTickWhileLocked(); - inline bool GetIsSourceClosed() + inline bool GetIsSourceClosed() const { - return session_->is_source_closed(); + return session->IsSourceClosed(); } - IMessagePassingServerSessionWrapper* server_; - pid_t pid_; - std::unique_ptr session_; - - score::message_passing::IServerConnection* connection_; - bool acquire_in_flight_; - std::optional acquire_deadline_; - std::uint32_t acquire_miss_count_{0U}; - - bool enqueued_; - bool running_; - bool to_delete_; - bool closed_by_peer_; - bool to_force_finish_; + IMessagePassingServerSessionWrapper* server; + pid_t pid; + std::unique_ptr session; + + score::message_passing::IServerConnection* connection; + bool acquire_in_flight; + std::optional acquire_deadline; + std::uint32_t acquire_miss_count{0U}; + + bool enqueued; + bool running; + bool to_delete; + bool closed_by_peer; + bool to_force_finish; }; void FinishPreviousSessionWhileLocked(std::unordered_map::iterator it, @@ -202,7 +202,7 @@ class MessagePassingServer : public IMessagePassingServerSessionWrapper std::mutex mutex_; score::cpp::stop_source stop_source_; - timestamp_t connection_timeout_; + TimestampT connection_timeout_; score::cpp::jthread worker_thread_; std::condition_variable worker_cond_; // to wake up worker thread std::unordered_map pid_session_map_; diff --git a/score/datarouter/src/daemon/message_passing_server.cpp b/score/datarouter/src/daemon/message_passing_server.cpp index 445daaa8..751eb02d 100644 --- a/score/datarouter/src/daemon/message_passing_server.cpp +++ b/score/datarouter/src/daemon/message_passing_server.cpp @@ -37,58 +37,58 @@ namespace internal using score::mw::log::detail::DatarouterMessageIdentifier; using score::mw::log::detail::MessagePassingConfig; -void MessagePassingServer::SessionWrapper::enqueue_for_delete_while_locked(bool by_peer) +void MessagePassingServer::SessionWrapper::EnqueueForDeleteWhileLocked(bool by_peer) { - to_delete_ = true; - closed_by_peer_ = by_peer; + to_delete = true; + closed_by_peer = by_peer; // in order not to mess with the logic of the queue, we don't enqueue currently running tick. Instead, we mark it // to be deleted (or re-enqueued for post-mortem processing, if closed by peer) at the end of the tick processing - if (!running_ && !enqueued_) + if (!running && !enqueued) { - server_->EnqueueTickWhileLocked(pid_); - enqueued_ = true; + server->EnqueueTickWhileLocked(pid); + enqueued = true; } } -bool MessagePassingServer::SessionWrapper::tick_at_worker_thread() +bool MessagePassingServer::SessionWrapper::TickAtWorkerThread() const { - bool requeue = session_->tick(); + bool requeue = session->Tick(); return requeue; } -void MessagePassingServer::SessionWrapper::notify_closed_by_peer() +void MessagePassingServer::SessionWrapper::NotifyClosedByPeer() const { - session_->on_closed_by_peer(); + session->OnClosedByPeer(); } -void MessagePassingServer::SessionWrapper::set_running_while_locked() +void MessagePassingServer::SessionWrapper::SetRunningWhileLocked() { - enqueued_ = false; - running_ = true; + enqueued = false; + running = true; } -bool MessagePassingServer::SessionWrapper::reset_running_while_locked(bool requeue) +bool MessagePassingServer::SessionWrapper::ResetRunningWhileLocked(bool requeue) { - running_ = false; + running = false; // check if we need to re-enqueue the tick after running again. It may happen because: // 1. not all the work in tick was done (returned early to avoid congestion); // 2. the tick was marked for delete as "closed by peer" when running, but we don't expedite its finishing. - if (requeue || closed_by_peer_) + if (requeue || closed_by_peer) { - enqueued_ = true; + enqueued = true; } - return enqueued_; + return enqueued; } -void MessagePassingServer::SessionWrapper::enqueue_tick_while_locked() +void MessagePassingServer::SessionWrapper::EnqueueTickWhileLocked() { - if (!enqueued_ && !to_delete_) + if (!enqueued && !to_delete) { - if (!running_) + if (!running) { - server_->EnqueueTickWhileLocked(pid_); + server->EnqueueTickWhileLocked(pid); } - enqueued_ = true; + enqueued = true; } } /* @@ -126,17 +126,17 @@ MessagePassingServer::MessagePassingServer(MessagePassingServer::SessionFactory std::cerr << "setname_np: " << ret_pthread.error() << std::endl; } - constexpr score::message_passing::ServiceProtocolConfig service_protocol_config{ + constexpr score::message_passing::ServiceProtocolConfig kServiceProtocolConfig{ MessagePassingConfig::kDatarouterReceiverIdentifier, MessagePassingConfig::kMaxMessageSize, MessagePassingConfig::kMaxReplySize, MessagePassingConfig::kMaxNotifySize}; - constexpr score::message_passing::IServerFactory::ServerConfig server_config{ + constexpr score::message_passing::IServerFactory::ServerConfig kServerConfig{ MessagePassingConfig::kMaxReceiverQueueSize, MessagePassingConfig::kPreAllocConnections, MessagePassingConfig::kMaxQueuedNotifies}; - receiver_ = server_factory_->Create(service_protocol_config, server_config); + receiver_ = server_factory_->Create(kServiceProtocolConfig, kServerConfig); auto connect_callback = [this_ptr = this](score::message_passing::IServerConnection& connection) noexcept -> std::uintptr_t { @@ -156,9 +156,9 @@ MessagePassingServer::MessagePassingServer(MessagePassingServer::SessionFactory if (found != this_ptr->pid_session_map_.end()) { SessionWrapper& wrapper = found->second; - wrapper.connection_ = nullptr; - wrapper.to_force_finish_ = true; - found->second.enqueue_for_delete_while_locked(true); + wrapper.connection = nullptr; + wrapper.to_force_finish = true; + found->second.EnqueueForDeleteWhileLocked(true); } }; auto received_send_message_callback = [this_ptr = this]( @@ -222,8 +222,8 @@ MessagePassingServer::~MessagePassingServer() noexcept void MessagePassingServer::RunWorkerThread() { - constexpr std::int32_t TIMEOUT_IN_MS = 100; - timestamp_t t1 = timestamp_t::clock::now() + std::chrono::milliseconds(TIMEOUT_IN_MS); + constexpr std::int32_t kTimeoutInMs = 100; + TimestampT t1 = TimestampT::clock::now() + std::chrono::milliseconds(kTimeoutInMs); std::unique_lock lock(mutex_); while (!workers_exit_) @@ -233,15 +233,15 @@ void MessagePassingServer::RunWorkerThread() }); if (!workers_exit_) { - timestamp_t now = timestamp_t::clock::now(); - if (connection_timeout_ != timestamp_t{} && now >= connection_timeout_) + TimestampT now = TimestampT::clock::now(); + if (connection_timeout_ != TimestampT{} && now >= connection_timeout_) { - connection_timeout_ = timestamp_t{}; + connection_timeout_ = TimestampT{}; stop_source_.request_stop(); } if (now >= t1) { - t1 = now + std::chrono::milliseconds(TIMEOUT_IN_MS); + t1 = now + std::chrono::milliseconds(kTimeoutInMs); for (auto& ps : pid_session_map_) { if (ps.second.GetIsSourceClosed()) @@ -250,25 +250,25 @@ void MessagePassingServer::RunWorkerThread() this is private functions so it cannot be test. */ // LCOV_EXCL_START - ps.second.enqueue_for_delete_while_locked(true); + ps.second.EnqueueForDeleteWhileLocked(true); // LCOV_EXCL_STOP } else { auto& wrapper = ps.second; - if (wrapper.acquire_in_flight_ && wrapper.acquire_deadline_.has_value() && - (now >= *wrapper.acquire_deadline_)) + if (wrapper.acquire_in_flight && wrapper.acquire_deadline.has_value() && + (now >= *wrapper.acquire_deadline)) { - wrapper.acquire_in_flight_ = false; - ++wrapper.acquire_miss_count_; - wrapper.acquire_deadline_.reset(); - if (wrapper.acquire_miss_count_ >= watchdog_config_.max_misses) + wrapper.acquire_in_flight = false; + ++wrapper.acquire_miss_count; + wrapper.acquire_deadline.reset(); + if (wrapper.acquire_miss_count >= watchdog_config_.max_misses) { - wrapper.enqueue_for_delete_while_locked(true); + wrapper.EnqueueForDeleteWhileLocked(true); continue; } } - ps.second.enqueue_tick_while_locked(); + ps.second.EnqueueTickWhileLocked(); } } } @@ -279,16 +279,16 @@ void MessagePassingServer::RunWorkerThread() pid_t pid = work_queue_.front(); work_queue_.pop(); SessionWrapper& wrapper = pid_session_map_.at(pid); - wrapper.set_running_while_locked(); - bool closed_by_peer = wrapper.get_reset_closed_by_peer(); + wrapper.SetRunningWhileLocked(); + bool closed_by_peer = wrapper.GetResetClosedByPeer(); lock.unlock(); if (closed_by_peer) { - wrapper.notify_closed_by_peer(); + wrapper.NotifyClosedByPeer(); } - bool requeue = wrapper.tick_at_worker_thread(); + bool requeue = wrapper.TickAtWorkerThread(); lock.lock(); - if (wrapper.to_force_finish_) + if (wrapper.to_force_finish) { if (!closed_by_peer) { @@ -298,7 +298,7 @@ void MessagePassingServer::RunWorkerThread() this is private functions so it cannot be test. */ // LCOV_EXCL_START - wrapper.notify_closed_by_peer(); + wrapper.NotifyClosedByPeer(); requeue = true; // LCOV_EXCL_STOP } @@ -310,7 +310,7 @@ void MessagePassingServer::RunWorkerThread() lock.unlock(); do { - requeue = wrapper.tick_at_worker_thread(); + requeue = wrapper.TickAtWorkerThread(); } while (requeue); lock.lock(); // LCOV_EXCL_STOP @@ -325,13 +325,13 @@ void MessagePassingServer::RunWorkerThread() node = {}; lock.lock(); } - else if (wrapper.reset_running_while_locked(requeue)) + else if (wrapper.ResetRunningWhileLocked(requeue)) { // LCOV_EXCL_START: see above EnqueueTickWhileLocked(pid); // LCOV_EXCL_STOP } - else if (wrapper.is_marked_for_delete()) + else if (wrapper.IsMarkedForDelete()) { // Extract the session wrapper to destroy it outside the mutex lock auto node = pid_session_map_.extract(pid); @@ -360,10 +360,10 @@ void MessagePassingServer::FinishPreviousSessionWhileLocked( { const pid_t pid = it->first; SessionWrapper& wrapper = it->second; - wrapper.to_force_finish_ = true; - wrapper.enqueue_for_delete_while_locked(true); + wrapper.to_force_finish = true; + wrapper.EnqueueForDeleteWhileLocked(true); // if enqueued_ (i.e. not running) expedite the workload toward the front of the queue - if (wrapper.enqueued_) + if (wrapper.enqueued) { pid_t front_pid = work_queue_.front(); while (front_pid != pid) @@ -390,7 +390,7 @@ void MessagePassingServer::MessageCallback(score::message_passing::IServerConnec score::cpp::span message) { const pid_t pid = connection.GetClientIdentity().pid; - if (message.size() < 1) + if (message.empty()) { std::cerr << "MessagePassingServer: Empty message received from " << pid; return; @@ -437,7 +437,8 @@ void MessagePassingServer::OnConnectRequest(score::message_passing::IServerConne */ // coverity[autosar_cpp14_m5_2_8_violation] score::cpp::span conn_span{static_cast(static_cast(&conn)), sizeof(conn)}; - std::ignore = std::copy(message.begin(), message.end(), conn_span.begin()); + std::ignore = std::copy_n( + message.begin(), std::min(message.size(), static_cast(conn_span.size())), conn_span.begin()); auto appid_sv = conn.GetAppId().GetStringView(); std::string appid{appid_sv.data(), appid_sv.size()}; @@ -493,9 +494,9 @@ void MessagePassingServer::OnConnectRequest(score::message_passing::IServerConne } // connection_ points to framework-owned object. It is cleared in disconnect_callback. - emplace_result.first->second.connection_ = &connection; + emplace_result.first->second.connection = &connection; // enqueue the tick to speed up processing connection - emplace_result.first->second.enqueue_tick_while_locked(); + emplace_result.first->second.EnqueueTickWhileLocked(); } } @@ -509,7 +510,7 @@ void MessagePassingServer::OnAcquireResponse(score::message_passing::IServerConn { auto& [key, session] = *found; std::ignore = key; - if (session.connection_ != &connection) + if (session.connection != &connection) { return; } @@ -524,13 +525,14 @@ void MessagePassingServer::OnAcquireResponse(score::message_passing::IServerConn */ // coverity[autosar_cpp14_m5_2_8_violation] score::cpp::span acq_span{static_cast(static_cast(&acq)), sizeof(acq)}; - std::ignore = std::copy(message.begin(), message.end(), acq_span.begin()); - session.session_->on_acquire_response(acq); - session.acquire_in_flight_ = false; - session.acquire_deadline_.reset(); - session.acquire_miss_count_ = 0U; + std::ignore = std::copy_n( + message.begin(), std::min(message.size(), static_cast(acq_span.size())), acq_span.begin()); + session.session->OnAcquireResponse(acq); + session.acquire_in_flight = false; + session.acquire_deadline.reset(); + session.acquire_miss_count = 0U; // enqueue the tick to speed up processing acquire response - session.enqueue_tick_while_locked(); + session.EnqueueTickWhileLocked(); } } @@ -543,26 +545,26 @@ bool MessagePassingServer::NotifyAcquireRequestWhileLocked(const pid_t pid) } auto& wrapper = found->second; - if (wrapper.connection_ == nullptr) + if (wrapper.connection == nullptr) { return false; } - if (wrapper.acquire_in_flight_) + if (wrapper.acquire_in_flight) { return true; } - constexpr std::array message{score::cpp::to_underlying(DatarouterMessageIdentifier::kAcquireRequest)}; - auto ret = wrapper.connection_->Notify(message); + constexpr std::array kMessage{score::cpp::to_underlying(DatarouterMessageIdentifier::kAcquireRequest)}; + auto ret = wrapper.connection->Notify(kMessage); if (!ret) { - wrapper.enqueue_for_delete_while_locked(true); + wrapper.EnqueueForDeleteWhileLocked(true); } else { - wrapper.acquire_in_flight_ = true; - wrapper.acquire_deadline_ = timestamp_t::clock::now() + watchdog_config_.deadline; + wrapper.acquire_in_flight = true; + wrapper.acquire_deadline = TimestampT::clock::now() + watchdog_config_.deadline; } return true; } diff --git a/score/datarouter/src/daemon/persistentlogging_config.cpp b/score/datarouter/src/daemon/persistentlogging_config.cpp index 397656e3..616b3001 100644 --- a/score/datarouter/src/daemon/persistentlogging_config.cpp +++ b/score/datarouter/src/daemon/persistentlogging_config.cpp @@ -24,6 +24,7 @@ #include #include #include +#include #include #include @@ -41,8 +42,18 @@ PersistentLoggingConfig ReadPersistentLoggingConfig(const std::string& file_path using ReadResult = PersistentLoggingConfig::ReadResult; PersistentLoggingConfig config; - using UniqueFileT = std::unique_ptr; - UniqueFileT fp(std::fopen(file_path.c_str(), "r"), &fclose); + struct FileCloser + { + void operator()(std::FILE* file) const noexcept + { + if (file != nullptr) + { + std::fclose(file); + } + } + }; + using UniqueFileT = std::unique_ptr; + UniqueFileT fp(std::fopen(file_path.c_str(), "r"), FileCloser{}); if (nullptr == fp) { config.read_result = ReadResult::kErrorOpen; diff --git a/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp b/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp index 453484c5..0679c55b 100644 --- a/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp +++ b/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp @@ -292,13 +292,13 @@ class MessagePassingServerFixture : public ::testing::Test { auto connection = std::make_unique>(); auto* connection_ptr = connection.get(); - auto emplace_result = connections_.emplace(pid, std::move(connection)); + auto emplace_result = connections.emplace(pid, std::move(connection)); EXPECT_TRUE(emplace_result.second); - connection_identities_.insert_or_assign(pid, score::message_passing::ClientIdentity{pid, 0, 0}); + connection_identities.insert_or_assign(pid, score::message_passing::ClientIdentity{pid, 0, 0}); EXPECT_CALL(*connection_ptr, GetClientIdentity()) .Times(AnyNumber()) - .WillRepeatedly(ReturnRef(connection_identities_.at(pid))); + .WillRepeatedly(ReturnRef(connection_identities.at(pid))); // Mirror production behavior: connect_callback runs before any messages. std::ignore = connect_callback(*connection_ptr); @@ -351,8 +351,8 @@ class MessagePassingServerFixture : public ::testing::Test score::message_passing::MessageCallback sent_callback; score::message_passing::MessageCallback sent_with_reply_callback; - std::unordered_map>> connections_; - std::map connection_identities_; + std::unordered_map>> connections; + std::map connection_identities; std::mutex map_mutex; std::condition_variable map_cond; // currently only used for destruction @@ -463,6 +463,13 @@ TEST_F(MessagePassingServerFixture, TestOneConnectAcquireRelease) EXPECT_EQ(acquire_response_count, 1); + { + std::array bad_message{}; + bad_message[0] = score::cpp::to_underlying(DatarouterMessageIdentifier::kAcquireResponse); + sent_callback(*connection_ptr, bad_message); + EXPECT_EQ(acquire_response_count, 2); + } + EXPECT_EQ(closed_by_peer_count, 0); EXPECT_FALSE(session_map.empty()); @@ -527,13 +534,13 @@ TEST_F(MessagePassingServerFixture, TestTripleConnectSamePid) this->disconnect_callback(connection); using namespace std::chrono_literals; std::this_thread::sleep_for(100ms); - connections_.erase(kClienT0Pid); + connections.erase(kClienT0Pid); auto* connection1 = ConnectClientAndSendConnectMessage(kClienT0Pid); EXPECT_CALL(connection, GetClientIdentity()).WillOnce(ReturnRef(client_identity)); this->disconnect_callback(connection); std::this_thread::sleep_for(100ms); - connections_.erase(kClienT0Pid); + connections.erase(kClienT0Pid); auto* connection2 = ConnectClientAndSendConnectMessage(kClienT0Pid); EXPECT_EQ(construct_count, 3); @@ -571,8 +578,8 @@ TEST_F(MessagePassingServerFixture, StaleConnectionAcquireResponseShouldBeIgnore } // Keep the old connection object alive, but free the PID slot for the new connection. - auto old_connection = std::move(connections_.at(kClienT0Pid)); - connections_.erase(kClienT0Pid); + auto old_connection = std::move(connections.at(kClienT0Pid)); + connections.erase(kClienT0Pid); auto* stale_connection_ptr = old_connection.get(); // Reconnect client with the same PID (connection1) and create a new session. @@ -627,7 +634,7 @@ TEST_F(MessagePassingServerFixture, TestSamePidWhileRunning) this->disconnect_callback(connection); std::this_thread::sleep_for(100ms); - connections_.erase(kClienT0Pid); + connections.erase(kClienT0Pid); auto* new_connection = ConnectClientAndSendConnectMessage(kClienT0Pid); std::ignore = new_connection; @@ -683,7 +690,7 @@ TEST_F(MessagePassingServerFixture, TestSamePidWhileQueued) this->disconnect_callback(connection); std::this_thread::sleep_for(100ms); - connections_.erase(kClienT2Pid); + connections.erase(kClienT2Pid); auto* new_connection = ConnectClientAndSendConnectMessage(kClienT2Pid); std::ignore = new_connection; @@ -724,9 +731,9 @@ TEST_F(MessagePassingServerFixture, WatchdogShouldTearDownUnresponsiveClient) { std::unique_lock lock(map_mutex); - map_cond.wait(lock, [this]() { + ASSERT_TRUE(map_cond.wait_for(lock, std::chrono::seconds{3}, [this]() { return session_map.empty(); - }); + })) << "Timed out waiting for watchdog teardown"; } ExpectServerDestruction(); @@ -752,7 +759,7 @@ TEST_F(MessagePassingServerFixture, WatchdogMissCountShouldResetOnValidResponse) // 1) First acquire request is missed by the client -> miss_count becomes 1. session_map.at(kClienT0Pid).handle->AcquireRequest(); using namespace std::chrono_literals; - std::this_thread::sleep_for(150ms); + std::this_thread::sleep_for(1s); // 2) Late valid response arrives -> miss_count should reset to 0. score::mw::log::detail::ReadAcquireResult acquire_result{0U}; @@ -763,16 +770,16 @@ TEST_F(MessagePassingServerFixture, WatchdogMissCountShouldResetOnValidResponse) // 3) One more missed acquire should NOT tear down (max_misses = 2, miss_count should be 1). session_map.at(kClienT0Pid).handle->AcquireRequest(); - std::this_thread::sleep_for(150ms); + std::this_thread::sleep_for(1s); EXPECT_FALSE(session_map.empty()); // 4) Second miss after the reset should now tear down. session_map.at(kClienT0Pid).handle->AcquireRequest(); { std::unique_lock lock(map_mutex); - map_cond.wait(lock, [this]() { + ASSERT_TRUE(map_cond.wait_for(lock, std::chrono::seconds{3}, [this]() { return session_map.empty(); - }); + })) << "Timed out waiting for watchdog teardown after miss-count reset"; } ExpectServerDestruction(); From ca93a81e0f73fc049db9cc22c054637563b3b8e9 Mon Sep 17 00:00:00 2001 From: Raghavendra Maddikery Date: Wed, 11 Mar 2026 13:22:40 +0100 Subject: [PATCH 19/22] Removes stale receive in data_router_message_client - The server already uses IServerConnection::Notify() on the existing single channel to send acquire requests. The client's second channel is redundant. GIT_ORIGIN_SPP_REV_ID: e0802ae92ba9090de244ac7ef14872cb3432c213 --- ...ata_router_message_client_factory_test.cpp | 3 - .../data_router_message_client_impl.cpp | 84 +--- .../data_router_message_client_impl.h | 18 +- .../data_router_message_client_test.cpp | 449 ++---------------- 4 files changed, 36 insertions(+), 518 deletions(-) diff --git a/score/mw/log/detail/data_router/data_router_message_client_factory_test.cpp b/score/mw/log/detail/data_router/data_router_message_client_factory_test.cpp index fc7564e5..816c2798 100644 --- a/score/mw/log/detail/data_router/data_router_message_client_factory_test.cpp +++ b/score/mw/log/detail/data_router/data_router_message_client_factory_test.cpp @@ -41,7 +41,6 @@ using ::testing::StrEq; const auto kMwsrFileName = ""; constexpr pid_t kThisProcessPid = 1234; constexpr uid_t kUid = 1234; -const std::string kClientReceiverIdentifier = "/" + std::string(""); class DatarouterMessageClientFactoryFixture : public ::testing::Test { @@ -111,8 +110,6 @@ TEST_F(DatarouterMessageClientFactoryFixture, CreateOnceShouldReturnClientWithEx auto* client_impl = dynamic_cast(client.get()); // Using the getters check that the factory provided the expected values to the constructor. - - EXPECT_EQ(client_impl->GetReceiverIdentifier(), kClientReceiverIdentifier); EXPECT_EQ(client_impl->GetAppid(), LoggingIdentifier{config_.GetAppId()}); EXPECT_EQ(client_impl->GetThisProcessPid(), kThisProcessPid); EXPECT_EQ(client_impl->GetWriterFileName(), kMwsrFileName); diff --git a/score/mw/log/detail/data_router/data_router_message_client_impl.cpp b/score/mw/log/detail/data_router/data_router_message_client_impl.cpp index 8e82e41f..a29eca48 100644 --- a/score/mw/log/detail/data_router/data_router_message_client_impl.cpp +++ b/score/mw/log/detail/data_router/data_router_message_client_impl.cpp @@ -19,8 +19,6 @@ #include "score/mw/log/detail/error.h" #include "score/mw/log/detail/initialization_reporter.h" -#include "score/os/utils/signal_impl.h" -#include "score/mw/log/detail/utils/signal_handling/signal_handling.h" #include #include #include @@ -53,7 +51,6 @@ DatarouterMessageClientImpl::DatarouterMessageClientImpl(const MsgClientIdentifi state_condition_{}, sender_state_{}, sender_{nullptr}, - receiver_{nullptr}, connect_thread_{} { } @@ -69,7 +66,6 @@ void DatarouterMessageClientImpl::Run() // coverity[autosar_cpp14_a15_4_2_violation] SCORE_LANGUAGE_FUTURECPP_PRECONDITION_PRD_MESSAGE(run_started_ == false, "Run() must be called only once"); run_started_ = true; - SetupReceiver(); RunConnectTask(); } @@ -95,7 +91,7 @@ void DatarouterMessageClientImpl::ConnectToDatarouter() noexcept return; } - // Wait for the sender to be in Ready state before starting receiver + // Wait for the sender to be in Ready state before sending connect message { std::unique_lock lock(sender_state_change_mutex_); state_condition_.wait(lock, [&stop_source = stop_source_, &sender_state = sender_state_]() { @@ -124,12 +120,6 @@ void DatarouterMessageClientImpl::ConnectToDatarouter() noexcept } // LCOV_EXCL_STOP - if (StartReceiver() == false) - { - RequestInternalShutdown(); - return; - } - CheckExitRequestAndSendConnectMessage(); } @@ -182,72 +172,6 @@ void DatarouterMessageClientImpl::SetThreadName() noexcept } } -void DatarouterMessageClientImpl::SetupReceiver() noexcept -{ - const score::message_passing::ServiceProtocolConfig service_protocol_config{ - msg_client_ids_.GetReceiverID(), MessagePassingConfig::kMaxMessageSize, 0U, 0U}; - - constexpr score::message_passing::IServerFactory::ServerConfig kServerConfig{ - MessagePassingConfig::kMaxReceiverQueueSize, 0U, 0U}; - receiver_ = message_passing_factory_->CreateServer(service_protocol_config, kServerConfig); -} - -bool DatarouterMessageClientImpl::StartReceiver() -{ - // When the receiver starts listening, receive callbacks may be called that use the sender to reply. - // Thus we must create the sender before starting to listen to messages. - // Note that the receiver callback may only be called after the connect task finished. - SCORE_LANGUAGE_FUTURECPP_PRECONDITION_PRD_MESSAGE(sender_ != nullptr, "The sender must be created before the receiver."); - - auto* this_ptr = this; - auto connect_callback = [this_ptr](score::message_passing::IServerConnection& connection) noexcept -> std::uintptr_t { - std::ignore = SignalHandling::PThreadBlockSigTerm(this_ptr->utils_.GetSignal()); - const pid_t client_pid = connection.GetClientIdentity().pid; - return static_cast(client_pid); - }; - auto disconnect_callback = [this_ptr](score::message_passing::IServerConnection& /*connection*/) noexcept { - this_ptr->RequestInternalShutdown(); - }; - auto received_send_message_callback = [this_ptr]( - score::message_passing::IServerConnection& /*connection*/, - const score::cpp::span /*message*/) noexcept -> score::cpp::blank { - this_ptr->OnAcquireRequest(); - return {}; - }; - auto received_send_message_with_reply_callback = - [](score::message_passing::IServerConnection& /*connection*/, - score::cpp::span /*message*/) noexcept -> score::cpp::blank { - return {}; - }; - - const auto result = receiver_->StartListening(connect_callback, - disconnect_callback, - received_send_message_callback, - received_send_message_with_reply_callback); - - if (result.has_value() == false) - { - const std::string underlying_error = result.error().ToString(); - ReportInitializationError(mw::log::detail::Error::kReceiverInitializationError, - std::string_view{underlying_error}, - msg_client_ids_.GetAppID().GetStringView()); - - std::array app_zero_terminated{}; - std::ignore = std::copy_n(msg_client_ids_.GetAppID().GetStringView().begin(), - std::min(msg_client_ids_.GetAppID().GetStringView().size(), - app_zero_terminated.size() - static_cast(1)), - app_zero_terminated.begin()); - std::cerr - << "[[mw::log]] Application " << app_zero_terminated.data() << " (PID: " << msg_client_ids_.GetThisProcID() - << ") failed to start message passing receiver. Please add the 'PROCMGR_AID_PATHSPACE' ability to your" - "'app_config.json'." - << '\n'; - - return false; - } - return true; -} - void DatarouterMessageClientImpl::RequestInternalShutdown() noexcept { // Unlink the shared memory file as early as possible to prevent memory leaks. @@ -323,7 +247,6 @@ void DatarouterMessageClientImpl::Shutdown() noexcept connect_thread_.join(); } - receiver_.reset(); { std::unique_lock lock(sender_mutex_); sender_.reset(); @@ -456,11 +379,6 @@ void DatarouterMessageClientImpl::UnlinkSharedMemoryFile() noexcept } } -const std::string& DatarouterMessageClientImpl::GetReceiverIdentifier() const noexcept -{ - return msg_client_ids_.GetReceiverID(); -} - const pid_t& DatarouterMessageClientImpl::GetThisProcessPid() const noexcept { return msg_client_ids_.GetThisProcID(); diff --git a/score/mw/log/detail/data_router/data_router_message_client_impl.h b/score/mw/log/detail/data_router/data_router_message_client_impl.h index 5500a731..daf21a32 100644 --- a/score/mw/log/detail/data_router/data_router_message_client_impl.h +++ b/score/mw/log/detail/data_router/data_router_message_client_impl.h @@ -18,7 +18,6 @@ #include "score/jthread.hpp" #include "score/optional.hpp" #include "score/stop_token.hpp" -#include "score/message_passing/i_server_connection.h" #include "score/os/errno.h" #include "score/mw/log/detail/data_router/data_router_message_client.h" #include "score/mw/log/detail/data_router/data_router_message_client_backend.h" @@ -70,28 +69,21 @@ class DatarouterMessageClientImpl : public DatarouterMessageClient /// \pre Shall not be called concurrently to Run(). void Shutdown() noexcept override; - void SetupReceiver() noexcept; - /// \brief Creates the message passing client (sender) for communication with Datarouter. /// \returns An empty expected on success, or an error if the sender could not be created. score::cpp::expected_blank CreateSender() noexcept; - /// \pre SetupReceiver and CreateSender() called before. - /// \returns true if the receiver was started successfully. - bool StartReceiver(); - /// \pre CreateSender() called before. void SendConnectMessage() noexcept; /// \brief Sets the thread name of the logger thread. void SetThreadName() noexcept; - /// \pre SetupReceiver() called before. + /// \brief Connects to the Datarouter by creating the sender and sending the connect message. void ConnectToDatarouter() noexcept; void BlockTermSignal() const noexcept; - const std::string& GetReceiverIdentifier() const noexcept; const pid_t& GetThisProcessPid() const noexcept; const std::string& GetWriterFileName() const noexcept; const LoggingIdentifier& GetAppid() const noexcept; @@ -130,14 +122,12 @@ class DatarouterMessageClientImpl : public DatarouterMessageClient score::cpp::optional sender_state_; // The construction/destruction order is critical here! - // Sender and receiver both may contain running tasks. - // Receiver tasks (callbacks) may use the sender. - // Thus the receiver needs to destruct first, and then the sender. - // Finally it is safe to join the connect thread. + // The sender may contain running tasks. + // The connect thread may use the sender. + // Thus the sender needs to destruct before joining the connect thread. // Only then we can ensure that there are no concurrent tasks // accessing private data from another thread. score::cpp::pmr::unique_ptr sender_; - score::cpp::pmr::unique_ptr receiver_; score::cpp::jthread connect_thread_; }; diff --git a/score/mw/log/detail/data_router/data_router_message_client_test.cpp b/score/mw/log/detail/data_router/data_router_message_client_test.cpp index dcc3ef6a..4bee1149 100644 --- a/score/mw/log/detail/data_router/data_router_message_client_test.cpp +++ b/score/mw/log/detail/data_router/data_router_message_client_test.cpp @@ -13,9 +13,6 @@ #include "score/assert_support.hpp" #include "score/message_passing/mock/client_connection_mock.h" -#include "score/message_passing/mock/server_connection_mock.h" -#include "score/message_passing/mock/server_mock.h" -#include "score/message_passing/server_types.h" #include "score/os/mocklib/mock_pthread.h" #include "score/os/mocklib/unistdmock.h" #include "score/os/utils/mocklib/signalmock.h" @@ -43,7 +40,6 @@ using ::testing::_; using ::testing::ByMove; using ::testing::Matcher; using ::testing::Return; -using ::testing::ReturnRef; using ::testing::StrEq; class DatarouterMessageClientMockTest : public ::testing::Test @@ -73,17 +69,6 @@ MATCHER_P(CompareServiceProtocol, expected, "") return true; } -MATCHER_P(CompareServerConfig, expected, "") -{ - if (arg.max_queued_sends != expected.max_queued_sends || - arg.pre_alloc_connections != expected.pre_alloc_connections || - arg.max_queued_notifies != expected.max_queued_notifies) - { - return false; - } - return true; -} - MATCHER_P(CompareClientConfig, expected, "") { if (arg.max_async_replies != expected.max_async_replies || arg.max_queued_sends != expected.max_queued_sends || @@ -106,7 +91,6 @@ constexpr auto kLoggerThreadName = "logger"; constexpr uid_t kDatarouterDummyUid = 111; constexpr std::uint32_t kMaxSendBytes{17U}; constexpr std::uint32_t kMaxNotifyBytes{1U}; -constexpr std::uint32_t kMaxNumberMessagesInReceiverQueue{1UL}; class DatarouterMessageClientFixture : public ::testing::Test { @@ -170,60 +154,6 @@ class DatarouterMessageClientFixture : public ::testing::Test .WillOnce(Return(score::cpp::make_unexpected(score::os::Error::createUnspecifiedError()))); } - score::message_passing::ServerMock* ExpectReceiverCreated() - { - auto receiver = score::cpp::pmr::make_unique>( - score::cpp::pmr::get_default_resource()); - auto* receiver_ptr = receiver.get(); - const score::message_passing::ServiceProtocolConfig service_protocol_config{ - kClientReceiverIdentifier, kMaxSendBytes, 0U, 0U}; - - const score::message_passing::IServerFactory::ServerConfig server_config{ - kMaxNumberMessagesInReceiverQueue, 0U, 0U}; - EXPECT_CALL(*message_passing_factory_, - CreateServer(CompareServiceProtocol(service_protocol_config), CompareServerConfig(server_config))) - .WillOnce(Return(ByMove(std::move(receiver)))); - return receiver_ptr; - } - - void ExpectReceiverStartListening( - score::message_passing::ServerMock* receiver_ptr, - score::message_passing::ConnectCallback* connect_callback = nullptr, - score::message_passing::DisconnectCallback* disconnect_callback = nullptr, - score::message_passing::MessageCallback* sent_callback = nullptr, - score::message_passing::MessageCallback* sent_with_reply_callback = nullptr, - score::cpp::expected_blank result = score::cpp::expected_blank{}) - { - EXPECT_CALL(*receiver_ptr, - StartListening(Matcher(_), - Matcher(_), - Matcher(_), - Matcher(_))) - .WillOnce([connect_callback, disconnect_callback, sent_callback, sent_with_reply_callback, result]( - score::message_passing::ConnectCallback con_callback, - score::message_passing::DisconnectCallback discon_callback, - score::message_passing::MessageCallback sn_callback, - score::message_passing::MessageCallback sn_rep_callback) { - if (connect_callback != nullptr) - { - *connect_callback = std::move(con_callback); - } - if (disconnect_callback != nullptr) - { - *disconnect_callback = std::move(discon_callback); - } - if (sent_callback != nullptr) - { - *sent_callback = std::move(sn_callback); - } - if (sent_with_reply_callback != nullptr) - { - *sent_with_reply_callback = std::move(sn_rep_callback); - } - return result; - }); - } - score::message_passing::ClientConnectionMock* ExpectSenderCreation( score::message_passing::IClientConnection::StateCallback* state_callback = nullptr, std::promise* callback_registered = nullptr, @@ -270,10 +200,6 @@ class DatarouterMessageClientFixture : public ::testing::Test EXPECT_CALL(*sender_mock, Destruct()); } - void ExpectServerDestruction(score::message_passing::ServerMock* receiver_mock) - { - EXPECT_CALL(*receiver_mock, Destruct()); - } void ExpectSendAcquireResponse( score::message_passing::ClientConnectionMock* sender_ptr, ReadAcquireResult expected_content, @@ -325,27 +251,6 @@ class DatarouterMessageClientFixture : public ::testing::Test }); } - void SendAcquireRequestAndExpectResponse(score::message_passing::MessageCallback& acquire_callback, - score::message_passing::ClientConnectionMock** sender_ptr, - bool first_message, - bool unlink_successful = true) - { - ReadAcquireResult acquired_data{}; - acquired_data.acquired_buffer = shared_data_.control_block.switch_count_points_active_for_writing.load(); - - if (first_message) - { - // MwsrWriter file shall be unlinked on first acquire request. - ExpectUnlinkMwsrWriterFile(unlink_successful); - } - - ExpectSendAcquireResponse(*sender_ptr, acquired_data); - - score::message_passing::ServerConnectionMock connection; - const score::cpp::span message{}; - acquire_callback(connection, message); - } - void SendAcquireNotifyAndExpectResponse(score::message_passing::IClientConnection::NotifyCallback& notify_callback, score::message_passing::ClientConnectionMock** sender_ptr, bool first_message, @@ -365,29 +270,13 @@ class DatarouterMessageClientFixture : public ::testing::Test notify_callback(message); } - void ExpectSenderAndReceiverCreation( - score::message_passing::ServerMock** receiver_ptr, + void ExpectSenderCreationSequence( score::message_passing::ClientConnectionMock** sender_ptr, - score::message_passing::IClientConnection::StateCallback* state_callback = nullptr, std::promise* callback_registered = nullptr, - score::cpp::expected_blank listen_result = score::cpp::expected_blank{}, - score::message_passing::ConnectCallback* connect_callback = nullptr, - score::message_passing::DisconnectCallback* disconnect_callback = nullptr, - score::message_passing::MessageCallback* sent_callback = nullptr, - score::message_passing::MessageCallback* sent_with_reply_callback = nullptr, bool block_termination_signal_pass = true, - bool receiver_start_listening = true, score::message_passing::IClientConnection::NotifyCallback* notify_callback = nullptr) - { - std::ignore = block_termination_signal_pass; // TODO: remove this param - auto* receiver = ExpectReceiverCreated(); - if (receiver_ptr != nullptr) - { - *receiver_ptr = receiver; - } - if (block_termination_signal_pass) { ExpectBlockTerminationSignalPass(); @@ -404,15 +293,6 @@ class DatarouterMessageClientFixture : public ::testing::Test { *sender_ptr = sender; } - if (receiver_start_listening) - { - ExpectReceiverStartListening(receiver, - connect_callback, - disconnect_callback, - sent_callback, - sent_with_reply_callback, - listen_result); - } } void ExpectSendConnectMessage(score::message_passing::ClientConnectionMock* sender_ptr) @@ -450,16 +330,12 @@ class DatarouterMessageClientFixture : public ::testing::Test EXPECT_CALL(*pthread_mock_, setname_np(kThreadId, StrEq(kLoggerThreadName))).WillOnce(Return(setname_result)); } - void ExecuteCreateSenderAndReceiverSequence( - bool expect_receiver_success = true, - score::message_passing::IClientConnection::StateCallback* state_callback = nullptr) + void ExecuteCreateSenderSequence(score::message_passing::IClientConnection::StateCallback* state_callback = nullptr) { - client_->SetupReceiver(); client_->BlockTermSignal(); client_->SetThreadName(); client_->CreateSender(); (*state_callback)(score::message_passing::IClientConnection::State::kReady); - EXPECT_EQ(client_->StartReceiver(), expect_receiver_success); } bool unlink_done_{false}; @@ -510,58 +386,6 @@ TEST_F(DatarouterMessageClientFixture, CreateSenderShouldCreateSenderWithExpecte client_->CreateSender(); } -TEST_F(DatarouterMessageClientFixture, StartReceiverShouldStartListenSuccessfully) -{ - RecordProperty("ASIL", "B"); - RecordProperty("Description", "Verifies creating the receiver works properly."); - RecordProperty("TestType", "Interface test"); - RecordProperty("DerivationTechnique", "Generation and analysis of equivalence classes"); - - testing::InSequence order_matters; - - score::message_passing::ClientConnectionMock* sender_ptr{}; - score::message_passing::ServerMock* receiver_ptr{}; - score::message_passing::IClientConnection::StateCallback state_callback; - - ExpectSenderAndReceiverCreation(&receiver_ptr, &sender_ptr, &state_callback); - - ExpectServerDestruction(receiver_ptr); - ExpectClientDestruction(sender_ptr); - ExecuteCreateSenderAndReceiverSequence(true, &state_callback); -} - -TEST_F(DatarouterMessageClientFixture, StartReceiverWithoutSenderAndReceiverShouldFail) -{ - RecordProperty("ASIL", "B"); - RecordProperty("Description", "Verifies creating the receiver works properly."); - RecordProperty("TestType", "Interface test"); - RecordProperty("DerivationTechnique", "Generation and analysis of equivalence classes"); - - testing::InSequence order_matters; - - EXPECT_DEATH(client_->StartReceiver(), ""); -} -TEST_F(DatarouterMessageClientFixture, ReceiverStartListeningFailsShouldBeHandledGracefully) -{ - RecordProperty("ASIL", "B"); - RecordProperty("Description", "Verifies the ability of handling the receiver listen failure properly."); - RecordProperty("TestType", "Interface test"); - RecordProperty("DerivationTechnique", "Generation and analysis of equivalence classes"); - - testing::InSequence order_matters; - - score::message_passing::ClientConnectionMock* sender_ptr{}; - score::message_passing::ServerMock* receiver_ptr{}; - score::message_passing::IClientConnection::StateCallback state_callback; - - auto start_listening_error = score::cpp::make_unexpected(score::os::Error::createFromErrno()); - ExpectSenderAndReceiverCreation(&receiver_ptr, &sender_ptr, &state_callback, nullptr, start_listening_error); - - ExpectServerDestruction(receiver_ptr); - ExpectClientDestruction(sender_ptr); - ExecuteCreateSenderAndReceiverSequence(false, &state_callback); -} - TEST_F(DatarouterMessageClientFixture, SendConnectMessageShouldSendExpectedPayload) { RecordProperty("ASIL", "B"); @@ -632,14 +456,11 @@ TEST_F(DatarouterMessageClientFixture, ConnectToDatarouterShouldSendConnectMessa testing::InSequence order_matters; score::message_passing::ClientConnectionMock* sender_ptr{}; - score::message_passing::ServerMock* receiver_ptr{}; - ExpectSenderAndReceiverCreation(&receiver_ptr, &sender_ptr, &state_callback, &callback_registered); + ExpectSenderCreationSequence(&sender_ptr, &state_callback, &callback_registered); ExpectSendConnectMessage(sender_ptr); - ExpectServerDestruction(receiver_ptr); ExpectClientDestruction(sender_ptr); - client_->SetupReceiver(); // We need to unblock waiting for the connection so we change the state in a separate thread std::thread connect_thread([this]() noexcept { client_->ConnectToDatarouter(); @@ -651,77 +472,6 @@ TEST_F(DatarouterMessageClientFixture, ConnectToDatarouterShouldSendConnectMessa connect_thread.join(); } -TEST_F(DatarouterMessageClientFixture, ConnectToDatarouterGivenThatReceiverFailedShouldNotSendConnectMessage) -{ - RecordProperty("ASIL", "B"); - RecordProperty("Description", "Verifies the in-ability of sending connect message when receiver fails."); - RecordProperty("TestType", "Interface test"); - RecordProperty("DerivationTechnique", "Generation and analysis of equivalence classes"); - - testing::InSequence order_matters; - - score::message_passing::ClientConnectionMock* sender_ptr{}; - score::message_passing::ServerMock* receiver_ptr{}; - score::message_passing::IClientConnection::StateCallback state_callback; - std::promise callback_registered; - - ExpectSenderAndReceiverCreation(&receiver_ptr, - &sender_ptr, - &state_callback, - &callback_registered, - score::cpp::make_unexpected(score::os::Error::createFromErrno())); - - ExpectUnlinkMwsrWriterFile(); - - ExpectServerDestruction(receiver_ptr); - ExpectClientDestruction(sender_ptr); - client_->SetupReceiver(); - // We need to unblock waiting for the connection so we change the state in a separate thread - std::thread connect_thread([this]() noexcept { - client_->ConnectToDatarouter(); - }); - - callback_registered.get_future().wait(); - state_callback(score::message_passing::IClientConnection::State::kReady); - - connect_thread.join(); -} - -TEST_F(DatarouterMessageClientFixture, AcquireRequestShouldSendExpectedAcquireResponse) -{ - RecordProperty("ASIL", "B"); - RecordProperty("Description", "Verifies that acquired request shall send the expected acquired response."); - RecordProperty("TestType", "Interface test"); - RecordProperty("DerivationTechnique", "Generation and analysis of equivalence classes"); - - testing::InSequence order_matters; - - score::message_passing::ClientConnectionMock* sender_ptr{}; - score::message_passing::ServerMock* receiver_ptr{}; - score::message_passing::ConnectCallback connect_callback; - score::message_passing::DisconnectCallback disconnect_callback; - score::message_passing::MessageCallback sent_callback; - score::message_passing::MessageCallback sent_with_reply_callback; - score::message_passing::IClientConnection::StateCallback state_callback; - - ExpectSenderAndReceiverCreation(&receiver_ptr, - &sender_ptr, - &state_callback, - nullptr, - {}, - &connect_callback, - &disconnect_callback, - &sent_callback, - &sent_with_reply_callback); - - ExecuteCreateSenderAndReceiverSequence(true, &state_callback); - - bool first_message = true; - SendAcquireRequestAndExpectResponse(sent_callback, &sender_ptr, first_message, false); - ExpectServerDestruction(receiver_ptr); - ExpectClientDestruction(sender_ptr); -} - TEST_F(DatarouterMessageClientFixture, AcquireNotifyShouldSendExpectedAcquireResponse) { RecordProperty("ASIL", "B"); @@ -734,33 +484,16 @@ TEST_F(DatarouterMessageClientFixture, AcquireNotifyShouldSendExpectedAcquireRes testing::InSequence order_matters; score::message_passing::ClientConnectionMock* sender_ptr{}; - score::message_passing::ServerMock* receiver_ptr{}; - score::message_passing::ConnectCallback connect_callback; - score::message_passing::DisconnectCallback disconnect_callback; - score::message_passing::MessageCallback sent_callback; - score::message_passing::MessageCallback sent_with_reply_callback; score::message_passing::IClientConnection::StateCallback state_callback; score::message_passing::IClientConnection::NotifyCallback notify_callback; - ExpectSenderAndReceiverCreation(&receiver_ptr, - &sender_ptr, - &state_callback, - nullptr, - {}, - &connect_callback, - &disconnect_callback, - &sent_callback, - &sent_with_reply_callback, - true, - true, - ¬ify_callback); - - ExecuteCreateSenderAndReceiverSequence(true, &state_callback); + ExpectSenderCreationSequence(&sender_ptr, &state_callback, nullptr, true, ¬ify_callback); + + ExecuteCreateSenderSequence(&state_callback); bool first_message = true; SendAcquireNotifyAndExpectResponse(notify_callback, &sender_ptr, first_message, false); - ExpectServerDestruction(receiver_ptr); ExpectClientDestruction(sender_ptr); } @@ -774,24 +507,12 @@ TEST_F(DatarouterMessageClientFixture, InvalidNotifyShouldNotSendAcquireResponse testing::InSequence order_matters; score::message_passing::ClientConnectionMock* sender_ptr{}; - score::message_passing::ServerMock* receiver_ptr{}; score::message_passing::IClientConnection::StateCallback state_callback; score::message_passing::IClientConnection::NotifyCallback notify_callback; - ExpectSenderAndReceiverCreation(&receiver_ptr, - &sender_ptr, - &state_callback, - nullptr, - {}, - nullptr, - nullptr, - nullptr, - nullptr, - true, - true, - ¬ify_callback); - - ExecuteCreateSenderAndReceiverSequence(true, &state_callback); + ExpectSenderCreationSequence(&sender_ptr, &state_callback, nullptr, true, ¬ify_callback); + + ExecuteCreateSenderSequence(&state_callback); EXPECT_CALL(*sender_ptr, Send(Matcher>(_))).Times(0); @@ -805,49 +526,34 @@ TEST_F(DatarouterMessageClientFixture, InvalidNotifyShouldNotSendAcquireResponse const std::array wrong_id_message{score::cpp::to_underlying(DatarouterMessageIdentifier::kConnect)}; notify_callback(wrong_id_message); - ExpectServerDestruction(receiver_ptr); ExpectClientDestruction(sender_ptr); } -TEST_F(DatarouterMessageClientFixture, SecondAcquireRequestShouldNotSetMwsrReader) +TEST_F(DatarouterMessageClientFixture, SecondAcquireNotifyShouldNotUnlinkMwsrWriter) { RecordProperty("ASIL", "B"); - RecordProperty("Description", "Verifies that the second acquire request should not set mwsr reader."); + RecordProperty("Description", "Verifies that the second acquire notify should not unlink the mwsr writer file."); RecordProperty("TestType", "Interface test"); RecordProperty("DerivationTechnique", "Generation and analysis of equivalence classes"); testing::InSequence order_matters; score::message_passing::ClientConnectionMock* sender_ptr{}; - score::message_passing::ServerMock* receiver_ptr{}; - score::message_passing::ConnectCallback connect_callback; - score::message_passing::DisconnectCallback disconnect_callback; - score::message_passing::MessageCallback sent_callback; - score::message_passing::MessageCallback sent_with_reply_callback; score::message_passing::IClientConnection::StateCallback state_callback; + score::message_passing::IClientConnection::NotifyCallback notify_callback; - ExpectSenderAndReceiverCreation(&receiver_ptr, - &sender_ptr, - &state_callback, - nullptr, - {}, - &connect_callback, - &disconnect_callback, - &sent_callback, - &sent_with_reply_callback); + ExpectSenderCreationSequence(&sender_ptr, &state_callback, nullptr, true, ¬ify_callback); - ExecuteCreateSenderAndReceiverSequence(true, &state_callback); + ExecuteCreateSenderSequence(&state_callback); bool first_message = true; - SendAcquireRequestAndExpectResponse(sent_callback, &sender_ptr, first_message, false); + SendAcquireNotifyAndExpectResponse(notify_callback, &sender_ptr, first_message, false); first_message = false; - SendAcquireRequestAndExpectResponse(sent_callback, &sender_ptr, first_message); - ExpectServerDestruction(receiver_ptr); + SendAcquireNotifyAndExpectResponse(notify_callback, &sender_ptr, first_message); ExpectClientDestruction(sender_ptr); } -// Refactor to acquire request TEST_F(DatarouterMessageClientFixture, ClientShouldShutdownAfterFailingToSendMessage) { RecordProperty("ASIL", "B"); @@ -858,23 +564,12 @@ TEST_F(DatarouterMessageClientFixture, ClientShouldShutdownAfterFailingToSendMes testing::InSequence order_matters; score::message_passing::ClientConnectionMock* sender_ptr{}; - score::message_passing::ServerMock* receiver_ptr{}; - score::message_passing::ConnectCallback connect_callback; - score::message_passing::DisconnectCallback disconnect_callback; - score::message_passing::MessageCallback sent_callback; - score::message_passing::MessageCallback sent_with_reply_callback; score::message_passing::IClientConnection::StateCallback state_callback; - ExpectSenderAndReceiverCreation(&receiver_ptr, - &sender_ptr, - &state_callback, - nullptr, - {}, - &connect_callback, - &disconnect_callback, - &sent_callback, - &sent_with_reply_callback); - - ExecuteCreateSenderAndReceiverSequence(true, &state_callback); + score::message_passing::IClientConnection::NotifyCallback notify_callback; + + ExpectSenderCreationSequence(&sender_ptr, &state_callback, nullptr, true, ¬ify_callback); + + ExecuteCreateSenderSequence(&state_callback); auto send_error = score::cpp::make_unexpected(score::os::Error::createFromErrno()); @@ -884,10 +579,8 @@ TEST_F(DatarouterMessageClientFixture, ClientShouldShutdownAfterFailingToSendMes ExpectUnlinkMwsrWriterFile(); ExpectSendAcquireResponse(sender_ptr, result, send_error); - score::message_passing::ServerConnectionMock connection; - const score::cpp::span message{}; - sent_callback(connection, message); - ExpectServerDestruction(receiver_ptr); + const std::array message{score::cpp::to_underlying(DatarouterMessageIdentifier::kAcquireRequest)}; + notify_callback(message); ExpectClientDestruction(sender_ptr); } @@ -901,14 +594,12 @@ TEST_F(DatarouterMessageClientFixture, RunShouldSetupAndConnect) testing::InSequence order_matters; score::message_passing::ClientConnectionMock* sender_ptr{}; - score::message_passing::ServerMock* receiver_ptr{}; score::message_passing::IClientConnection::StateCallback state_callback; std::promise callback_registered; - ExpectSenderAndReceiverCreation(&receiver_ptr, &sender_ptr, &state_callback, &callback_registered); + ExpectSenderCreationSequence(&sender_ptr, &state_callback, &callback_registered); ExpectSendConnectMessage(sender_ptr); - ExpectServerDestruction(receiver_ptr); ExpectClientDestruction(sender_ptr); ExpectUnlinkMwsrWriterFile(); client_->Run(); @@ -930,14 +621,12 @@ TEST_F(DatarouterMessageClientFixture, RunShallNotBeCalledMoreThanOnce) testing::InSequence order_matters; score::message_passing::ClientConnectionMock* sender_ptr{}; - score::message_passing::ServerMock* receiver_ptr{}; score::message_passing::IClientConnection::StateCallback state_callback; std::promise callback_registered; - ExpectSenderAndReceiverCreation(&receiver_ptr, &sender_ptr, &state_callback, &callback_registered); + ExpectSenderCreationSequence(&sender_ptr, &state_callback, &callback_registered); ExpectSendConnectMessage(sender_ptr); - ExpectServerDestruction(receiver_ptr); ExpectClientDestruction(sender_ptr); ExpectUnlinkMwsrWriterFile(); @@ -986,13 +675,11 @@ TEST_F(DatarouterMessageClientFixture, FailedToChownOwnMsrWriterFileForDataRoute testing::InSequence order_matters; score::message_passing::ClientConnectionMock* sender_ptr{}; - score::message_passing::ServerMock* receiver_ptr{}; score::message_passing::IClientConnection::StateCallback state_callback; std::promise callback_registered; - ExpectSenderAndReceiverCreation(&receiver_ptr, &sender_ptr, &state_callback, &callback_registered); + ExpectSenderCreationSequence(&sender_ptr, &state_callback, &callback_registered); ExpectSendConnectMessage(sender_ptr); - ExpectServerDestruction(receiver_ptr); ExpectClientDestruction(sender_ptr); ExpectUnlinkMwsrWriterFile(false); client_->Run(); @@ -1013,24 +700,11 @@ TEST_F(DatarouterMessageClientFixture, GivenExitRequestDuringConnectionShouldNot testing::InSequence order_matters; score::message_passing::ClientConnectionMock* sender_ptr{}; - score::message_passing::ServerMock* receiver_ptr{}; - - ExpectSenderAndReceiverCreation(&receiver_ptr, - &sender_ptr, - nullptr, - nullptr, - score::cpp::make_unexpected(score::os::Error::createFromErrno()), - nullptr, - nullptr, - nullptr, - nullptr, - true, - false); + + ExpectSenderCreationSequence(&sender_ptr); ExpectUnlinkMwsrWriterFile(); - ExpectServerDestruction(receiver_ptr); ExpectClientDestruction(sender_ptr); - client_->SetupReceiver(); stop_source_.request_stop(); client_->ConnectToDatarouter(); } @@ -1045,26 +719,13 @@ TEST_F(DatarouterMessageClientFixture, FailedToEmptySignalSet) testing::InSequence order_matters; score::message_passing::ClientConnectionMock* sender_ptr{}; - score::message_passing::ServerMock* receiver_ptr{}; score::message_passing::IClientConnection::StateCallback state_callback; std::promise callback_registered; - ExpectSenderAndReceiverCreation(&receiver_ptr, - &sender_ptr, - &state_callback, - &callback_registered, - score::cpp::make_unexpected(score::os::Error::createFromErrno()), - nullptr, - nullptr, - nullptr, - nullptr, - false); - ExpectUnlinkMwsrWriterFile(); - - ExpectServerDestruction(receiver_ptr); + ExpectSenderCreationSequence(&sender_ptr, &state_callback, &callback_registered, false); + ExpectSendConnectMessage(sender_ptr); ExpectClientDestruction(sender_ptr); - client_->SetupReceiver(); // We need to unblock waiting for the connection so we change the state in a separate thread std::thread connect_thread([this]() noexcept { client_->ConnectToDatarouter(); @@ -1081,55 +742,7 @@ TEST_F(DatarouterMessageClientFixture, ConnectToDatarouterShouldReportErrorAndSh RecordProperty("Description", "Verifies ConnectToDatarouter reports an error and shuts down when CreateSender fails."); RecordProperty("TestType", "Interface test"); - RecordProperty("DerivationTechnique", "Generation and analysis of equivalence classes"); - testing::InSequence order_matters; - - - ExpectBlockTerminationSignalPass(); - ExpectSetLoggerThreadName(); - - const score::message_passing::ServiceProtocolConfig service_protocol_config{ - kDatarouterReceiverIdentifier, kMaxSendBytes, 0U, kMaxNotifyBytes}; - const score::message_passing::IClientFactory::ClientConfig client_config{0, 10, false, true, false}; - - EXPECT_CALL(*message_passing_factory_, - CreateClient(CompareServiceProtocol(service_protocol_config), CompareClientConfig(client_config))) - .WillOnce(Return(ByMove(nullptr))); - - ExpectUnlinkMwsrWriterFile(); - - client_->ConnectToDatarouter(); - - EXPECT_TRUE(stop_source_.stop_requested()); -} - -TEST_F(DatarouterMessageClientFixture, CreateSenderFailsWhenFactoryReturnsNullptr) -{ - RecordProperty("ASIL", "B"); - RecordProperty("Description", - "Verifies CreateSender reports an error and returns unexpected when the factory returns nullptr."); - RecordProperty("TestType", "Interface test"); - RecordProperty("DerivationTechnique", "Generation and analysis of equivalence classes"); - - const score::message_passing::ServiceProtocolConfig service_protocol_config{ - kDatarouterReceiverIdentifier, kMaxSendBytes, 0U, kMaxNotifyBytes}; - const score::message_passing::IClientFactory::ClientConfig client_config{0, 10, false, true, false}; - - EXPECT_CALL(*message_passing_factory_, - CreateClient(CompareServiceProtocol(service_protocol_config), CompareClientConfig(client_config))) - .WillOnce(Return(ByMove(nullptr))); - const auto result = client_->CreateSender(); - ASSERT_FALSE(result.has_value()); - -} - -TEST_F(DatarouterMessageClientFixture, ConnectToDatarouterShouldReportErrorAndShutdownWhenCreateSenderFails) -{ - RecordProperty("ASIL", "B"); - RecordProperty("Description", - "Verifies ConnectToDatarouter reports an error and shuts down when CreateSender fails."); - RecordProperty("TestType", "Interface test"); - RecordProperty("DerivationTechnique", "Generation and analysis of equivalence classes"); + RecordProperty("DerivationTechnique", "Error guessing based on knowledge or experience"); testing::InSequence order_matters; From 3775e219439a9877f3c53a0d1c200640f6fc1ebc Mon Sep 17 00:00:00 2001 From: Raghavendra Maddikery Date: Fri, 13 Mar 2026 18:41:30 +0100 Subject: [PATCH 20/22] Handle ENOBUFS for Notify - Treat ENOBUFS as a transient error and let the watchdog handle the unresponsive client - this mitigates session disconnection if the client is slow usually during startups GIT_ORIGIN_SPP_REV_ID: 35579555a589dfeab63587b6c3cac7640984963d --- .../src/daemon/message_passing_server.cpp | 9 +++ .../test_message_passing_server.cpp | 55 +++++++++++++++++++ 2 files changed, 64 insertions(+) diff --git a/score/datarouter/src/daemon/message_passing_server.cpp b/score/datarouter/src/daemon/message_passing_server.cpp index 751eb02d..92752e34 100644 --- a/score/datarouter/src/daemon/message_passing_server.cpp +++ b/score/datarouter/src/daemon/message_passing_server.cpp @@ -559,6 +559,15 @@ bool MessagePassingServer::NotifyAcquireRequestWhileLocked(const pid_t pid) auto ret = wrapper.connection->Notify(kMessage); if (!ret) { + // ENOBUFS indicates the notify pool is temporarily exhausted (a previous notification could still be in + // flight). This is a transient condition - Let the watchdog will handle it if the client never responds. + if (ret.error().GetOsDependentErrorCode() == ENOBUFS) + { + std::cerr << "MessagePassingServer: Notify pool exhausted for pid " << pid << ", skipping acquire request" + << std::endl; + return true; + } + std::cerr << "MessagePassingServer: Notify failed for pid " << pid << ": " << ret.error() << std::endl; wrapper.EnqueueForDeleteWhileLocked(true); } else diff --git a/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp b/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp index 0679c55b..d7ebb939 100644 --- a/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp +++ b/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp @@ -786,6 +786,61 @@ TEST_F(MessagePassingServerFixture, WatchdogMissCountShouldResetOnValidResponse) UninstantiateServer(); } +TEST_F(MessagePassingServerFixture, EnobufsFromNotifyShouldNotKillSession) +{ + ExpectOurPidIsQueried(); + + InstantiateServer(GetCountingSessionFactory()); + + auto* connection_ptr = ConnectClientAndSendConnectMessage(kClienT0Pid); + + // First Notify() fails with ENOBUFS (notify pool exhausted — transient). + EXPECT_CALL(*connection_ptr, Notify(Matcher>(_))) + .WillOnce(Return(score::cpp::make_unexpected(score::os::Error::createFromErrno(ENOBUFS)))); + + session_map.at(kClienT0Pid).handle->AcquireRequest(); + + // Session should still be alive — ENOBUFS is treated as a transient error. + using namespace std::chrono_literals; + std::this_thread::sleep_for(200ms); + EXPECT_FALSE(session_map.empty()); + + // A subsequent successful Notify() should work normally. + ::testing::Sequence seq; + ExpectAcquireNotifyInSequence(DatarouterMessageIdentifier::kAcquireRequest, seq, connection_ptr); + session_map.at(kClienT0Pid).handle->AcquireRequest(); + + // Verify session is still alive after successful notify. + std::this_thread::sleep_for(200ms); + EXPECT_FALSE(session_map.empty()); + + ExpectServerDestruction(); + UninstantiateServer(); +} + +TEST_F(MessagePassingServerFixture, NonEnobufsNotifyFailureShouldStillKillSession) +{ + ExpectOurPidIsQueried(); + + InstantiateServer(GetCountingSessionFactory()); + + auto* connection_ptr = ConnectClientAndSendConnectMessage(kClienT0Pid); + + // Notify() fails with EINVAL (not ENOBUFS) — should still tear down the session. + ExpectAndFailAcquireNotify(connection_ptr); + session_map.at(kClienT0Pid).handle->AcquireRequest(); + + { + std::unique_lock lock(map_mutex); + ASSERT_TRUE(map_cond.wait_for(lock, std::chrono::seconds{3}, [this]() { + return session_map.empty(); + })) << "Timed out waiting for session teardown after non-ENOBUFS failure"; + } + + ExpectServerDestruction(); + UninstantiateServer(); +} + TEST(MessagePassingServerTests, sessionWrapperCreateTest) { InSequence s; From ef1af61fa6fd58231dbb6ff6c704e0adf8b42993 Mon Sep 17 00:00:00 2001 From: Raghavendra Maddikery Date: Fri, 13 Mar 2026 18:53:06 +0100 Subject: [PATCH 21/22] Refactor NotifyAcquireRequest - release the global mutex before calling connection->Notify() - Removed NotifyAcquireRequestWhileLocked() as it is no longer needed. GIT_ORIGIN_SPP_REV_ID: 0546d81a5efcccaabeb0fc843acc102d57f7ea84 --- .../include/daemon/message_passing_server.h | 9 +- .../src/daemon/message_passing_server.cpp | 26 ++++-- .../test_message_passing_server.cpp | 92 +++++++++++++++---- 3 files changed, 97 insertions(+), 30 deletions(-) diff --git a/score/datarouter/include/daemon/message_passing_server.h b/score/datarouter/include/daemon/message_passing_server.h index 6cf11485..b95435fd 100644 --- a/score/datarouter/include/daemon/message_passing_server.h +++ b/score/datarouter/include/daemon/message_passing_server.h @@ -92,6 +92,14 @@ class MessagePassingServer : public IMessagePassingServerSessionWrapper MessagePassingServer* server_; }; + /// Thread-safety contract: + /// - Tick() is called from the worker thread without the server mutex held. + /// - OnAcquireResponse() is called from the dispatch thread with the server mutex held. + /// - OnClosedByPeer() is called from the worker thread without the server mutex held. + /// - IsSourceClosed() is called from the worker thread with the server mutex held. + /// Implementations must ensure that shared state accessed by Tick() and by any method + /// called under the mutex (OnAcquireResponse, IsSourceClosed) is properly synchronized + /// (e.g., via atomics or an internal lock). class ISession { public: @@ -119,7 +127,6 @@ class MessagePassingServer : public IMessagePassingServerSessionWrapper friend class SessionHandle; bool NotifyAcquireRequest(pid_t pid); - bool NotifyAcquireRequestWhileLocked(pid_t pid); void MessageCallback(score::message_passing::IServerConnection& connection, score::cpp::span message); void OnConnectRequest(score::message_passing::IServerConnection& connection, diff --git a/score/datarouter/src/daemon/message_passing_server.cpp b/score/datarouter/src/daemon/message_passing_server.cpp index 92752e34..87fc1603 100644 --- a/score/datarouter/src/daemon/message_passing_server.cpp +++ b/score/datarouter/src/daemon/message_passing_server.cpp @@ -490,6 +490,8 @@ void MessagePassingServer::OnConnectRequest(score::message_passing::IServerConne { // Existing session for this PID is still present; do not overwrite it. // The reconnect path is handled by disconnect_callback + worker-thread teardown. + std::cerr << "MessagePassingServer: Session for pid " << pid << " already exists, dropping new session" + << std::endl; return; } @@ -536,8 +538,16 @@ void MessagePassingServer::OnAcquireResponse(score::message_passing::IServerConn } } -bool MessagePassingServer::NotifyAcquireRequestWhileLocked(const pid_t pid) +bool MessagePassingServer::NotifyAcquireRequest(const pid_t pid) { + // Guard against calls during server destruction (e.g., from session destructors + // during pid_session_map_.clear()). workers_exit_ is set before the map is cleared. + if (workers_exit_.load()) + { + return false; + } + + std::lock_guard lock(mutex_); const auto found = pid_session_map_.find(pid); if (found == pid_session_map_.end()) { @@ -555,12 +565,16 @@ bool MessagePassingServer::NotifyAcquireRequestWhileLocked(const pid_t pid) return true; } + // Notify() is non-blocking (QNX pulse), so holding the mutex is safe and avoids + // a use-after-free race where disconnect_callback could destroy the connection + // object between releasing the lock and calling Notify(). constexpr std::array kMessage{score::cpp::to_underlying(DatarouterMessageIdentifier::kAcquireRequest)}; auto ret = wrapper.connection->Notify(kMessage); + if (!ret) { - // ENOBUFS indicates the notify pool is temporarily exhausted (a previous notification could still be in - // flight). This is a transient condition - Let the watchdog will handle it if the client never responds. + // ENOBUFS indicates the notify pool is temporarily exhausted (a previous notification is still in flight). + // This is a transient condition — the watchdog will handle it if the client never responds. if (ret.error().GetOsDependentErrorCode() == ENOBUFS) { std::cerr << "MessagePassingServer: Notify pool exhausted for pid " << pid << ", skipping acquire request" @@ -578,12 +592,6 @@ bool MessagePassingServer::NotifyAcquireRequestWhileLocked(const pid_t pid) return true; } -bool MessagePassingServer::NotifyAcquireRequest(const pid_t pid) -{ - std::lock_guard lock(mutex_); - return NotifyAcquireRequestWhileLocked(pid); -} - bool MessagePassingServer::SessionHandle::AcquireRequest() const { if (server_ == nullptr) diff --git a/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp b/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp index d7ebb939..d163fa5d 100644 --- a/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp +++ b/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp @@ -532,13 +532,22 @@ TEST_F(MessagePassingServerFixture, TestTripleConnectSamePid) auto* connection0 = ConnectClientAndSendConnectMessage(kClienT0Pid); EXPECT_CALL(connection, GetClientIdentity()).WillOnce(ReturnRef(client_identity)); this->disconnect_callback(connection); - using namespace std::chrono_literals; - std::this_thread::sleep_for(100ms); + { + std::unique_lock lock(map_mutex); + ASSERT_TRUE(map_cond.wait_for(lock, std::chrono::seconds{1}, [this]() { + return session_map.find(kClienT0Pid) == session_map.end(); + })) << "Timed out waiting for session cleanup after first disconnect"; + } connections.erase(kClienT0Pid); auto* connection1 = ConnectClientAndSendConnectMessage(kClienT0Pid); EXPECT_CALL(connection, GetClientIdentity()).WillOnce(ReturnRef(client_identity)); this->disconnect_callback(connection); - std::this_thread::sleep_for(100ms); + { + std::unique_lock lock(map_mutex); + ASSERT_TRUE(map_cond.wait_for(lock, std::chrono::seconds{1}, [this]() { + return session_map.find(kClienT0Pid) == session_map.end(); + })) << "Timed out waiting for session cleanup after second disconnect"; + } connections.erase(kClienT0Pid); auto* connection2 = ConnectClientAndSendConnectMessage(kClienT0Pid); @@ -622,17 +631,20 @@ TEST_F(MessagePassingServerFixture, TestSamePidWhileRunning) // wait until CLIENT0 is blocked inside the first tick session_map.at(kClienT0Pid).WaitStartOfFirstTick(); - // accumulate other ticks in the queue - using namespace std::chrono_literals; - std::this_thread::sleep_for(100ms); - // we will need to unblock the tick before the callback returns, so start it on a separate thread std::thread connect_thread([&]() { StrictMock<::score::message_passing::ServerConnectionMock> connection; score::message_passing::ClientIdentity client_identity{kClienT0Pid, 0, 0}; EXPECT_CALL(connection, GetClientIdentity()).WillOnce(ReturnRef(client_identity)); this->disconnect_callback(connection); - std::this_thread::sleep_for(100ms); + + // Wait for the old session to be fully destroyed before reconnecting + { + std::unique_lock lock(map_mutex); + ASSERT_TRUE(map_cond.wait_for(lock, std::chrono::seconds{1}, [this]() { + return session_map.find(kClienT0Pid) == session_map.end(); + })) << "Timed out waiting for old CLIENT0 session destruction"; + } connections.erase(kClienT0Pid); @@ -678,17 +690,20 @@ TEST_F(MessagePassingServerFixture, TestSamePidWhileQueued) // wait until CLIENT0 is blocked inside the first tick session_map.at(kClienT0Pid).WaitStartOfFirstTick(); - // accumulate other ticks (CLIENT2 in particular) in the queue - using namespace std::chrono_literals; - std::this_thread::sleep_for(250ms); - // we will need to unblock the tick before the callback returns, so start it on a separate thread std::thread connect_thread([&]() { StrictMock<::score::message_passing::ServerConnectionMock> connection; score::message_passing::ClientIdentity client_identity{kClienT2Pid, 0, 0}; EXPECT_CALL(connection, GetClientIdentity()).WillOnce(ReturnRef(client_identity)); this->disconnect_callback(connection); - std::this_thread::sleep_for(100ms); + + // Wait for the old session to be fully destroyed before reconnecting + { + std::unique_lock lock(map_mutex); + ASSERT_TRUE(map_cond.wait_for(lock, std::chrono::seconds{1}, [this]() { + return session_map.find(kClienT2Pid) == session_map.end(); + })) << "Timed out waiting for old CLIENT2 session destruction"; + } connections.erase(kClienT2Pid); @@ -731,7 +746,7 @@ TEST_F(MessagePassingServerFixture, WatchdogShouldTearDownUnresponsiveClient) { std::unique_lock lock(map_mutex); - ASSERT_TRUE(map_cond.wait_for(lock, std::chrono::seconds{3}, [this]() { + ASSERT_TRUE(map_cond.wait_for(lock, std::chrono::seconds{1}, [this]() { return session_map.empty(); })) << "Timed out waiting for watchdog teardown"; } @@ -759,7 +774,7 @@ TEST_F(MessagePassingServerFixture, WatchdogMissCountShouldResetOnValidResponse) // 1) First acquire request is missed by the client -> miss_count becomes 1. session_map.at(kClienT0Pid).handle->AcquireRequest(); using namespace std::chrono_literals; - std::this_thread::sleep_for(1s); + std::this_thread::sleep_for(100ms); // 2) Late valid response arrives -> miss_count should reset to 0. score::mw::log::detail::ReadAcquireResult acquire_result{0U}; @@ -770,14 +785,14 @@ TEST_F(MessagePassingServerFixture, WatchdogMissCountShouldResetOnValidResponse) // 3) One more missed acquire should NOT tear down (max_misses = 2, miss_count should be 1). session_map.at(kClienT0Pid).handle->AcquireRequest(); - std::this_thread::sleep_for(1s); + std::this_thread::sleep_for(100ms); EXPECT_FALSE(session_map.empty()); // 4) Second miss after the reset should now tear down. session_map.at(kClienT0Pid).handle->AcquireRequest(); { std::unique_lock lock(map_mutex); - ASSERT_TRUE(map_cond.wait_for(lock, std::chrono::seconds{3}, [this]() { + ASSERT_TRUE(map_cond.wait_for(lock, std::chrono::seconds{1}, [this]() { return session_map.empty(); })) << "Timed out waiting for watchdog teardown after miss-count reset"; } @@ -802,7 +817,7 @@ TEST_F(MessagePassingServerFixture, EnobufsFromNotifyShouldNotKillSession) // Session should still be alive — ENOBUFS is treated as a transient error. using namespace std::chrono_literals; - std::this_thread::sleep_for(200ms); + std::this_thread::sleep_for(50ms); EXPECT_FALSE(session_map.empty()); // A subsequent successful Notify() should work normally. @@ -811,7 +826,7 @@ TEST_F(MessagePassingServerFixture, EnobufsFromNotifyShouldNotKillSession) session_map.at(kClienT0Pid).handle->AcquireRequest(); // Verify session is still alive after successful notify. - std::this_thread::sleep_for(200ms); + std::this_thread::sleep_for(50ms); EXPECT_FALSE(session_map.empty()); ExpectServerDestruction(); @@ -832,7 +847,7 @@ TEST_F(MessagePassingServerFixture, NonEnobufsNotifyFailureShouldStillKillSessio { std::unique_lock lock(map_mutex); - ASSERT_TRUE(map_cond.wait_for(lock, std::chrono::seconds{3}, [this]() { + ASSERT_TRUE(map_cond.wait_for(lock, std::chrono::seconds{1}, [this]() { return session_map.empty(); })) << "Timed out waiting for session teardown after non-ENOBUFS failure"; } @@ -841,6 +856,43 @@ TEST_F(MessagePassingServerFixture, NonEnobufsNotifyFailureShouldStillKillSessio UninstantiateServer(); } +TEST_F(MessagePassingServerFixture, DisconnectDuringNotifyShouldNotCrash) +{ + ExpectOurPidIsQueried(); + + InstantiateServer(GetCountingSessionFactory()); + + auto* connection_ptr = ConnectClientAndSendConnectMessage(kClienT0Pid); + + // Simulate a disconnect arriving right after the Notify() kernel call. + // In production the disconnect callback fires on the dispatch thread and can + // race with NotifyAcquireRequest. NotifyAcquireRequest holds the server mutex + // for the duration of Notify(), so we spawn the disconnect thread inside the + // mock (it blocks on the mutex) and join it after AcquireRequest returns. + std::thread disconnect_thread; + EXPECT_CALL(*connection_ptr, Notify(Matcher>(_))) + .WillOnce([this, connection_ptr, &disconnect_thread](const auto /*m*/) { + disconnect_thread = std::thread([this, connection_ptr]() { + this->disconnect_callback(*connection_ptr); + }); + return score::cpp::expected_blank{}; + }); + + session_map.at(kClienT0Pid).handle->AcquireRequest(); + disconnect_thread.join(); + + // Wait for teardown to complete. + { + std::unique_lock lock(map_mutex); + ASSERT_TRUE(map_cond.wait_for(lock, std::chrono::seconds{1}, [this]() { + return session_map.empty(); + })) << "Timed out waiting for session teardown after disconnect-during-notify"; + } + + ExpectServerDestruction(); + UninstantiateServer(); +} + TEST(MessagePassingServerTests, sessionWrapperCreateTest) { InSequence s; From 9ed60eecec39a5bc01403fe56bef51a57f7e4f6b Mon Sep 17 00:00:00 2001 From: Jianjun Wen Date: Fri, 3 Jul 2026 04:28:19 +0200 Subject: [PATCH 22/22] fix unit tests GIT_ORIGIN_SPP_REV_ID: 35a2250fa80b6e15f22e51288ba7a9d16c883ebc --- .../test_message_passing_server.cpp | 29 ++----------------- .../data_router_message_client_impl.cpp | 6 ++++ 2 files changed, 8 insertions(+), 27 deletions(-) diff --git a/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp b/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp index d163fa5d..10b66d11 100644 --- a/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp +++ b/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp @@ -430,7 +430,7 @@ TEST_F(MessagePassingServerFixture, TestStartListeningFailure) Matcher(_))) .WillOnce(Return(score::cpp::make_unexpected(score::os::Error::createFromErrno(EINVAL)))); - server.emplace(factory, server_factory_mock, client_factory_mock); + server.emplace(factory, server_factory_mock); ExpectServerDestruction(); UninstantiateServer(); } @@ -892,7 +892,6 @@ TEST_F(MessagePassingServerFixture, DisconnectDuringNotifyShouldNotCrash) ExpectServerDestruction(); UninstantiateServer(); } - TEST(MessagePassingServerTests, sessionWrapperCreateTest) { InSequence s; @@ -1090,25 +1089,12 @@ TEST_F(MessagePassingServerFixture, MessageCallbackUnknownMessageType) TEST(MessagePassingServerTests, SessionHandleAcquireRequestNotReady) { const pid_t pid = 0; - - auto client = score::cpp::pmr::make_unique(score::cpp::pmr::get_default_resource()); - auto* client_raw_ptr = client.get(); MessagePassingServer* msg_server = nullptr; - EXPECT_CALL(*client_raw_ptr, - Start(Matcher(_), - Matcher(_))); - - // Return a non-Ready state to trigger "return false" - EXPECT_CALL(*client_raw_ptr, GetState()) - .WillRepeatedly(Return(score::message_passing::IClientConnection::State::kStarting)); - - MessagePassingServer::SessionHandle session_handle(pid, msg_server, std::move(client)); + MessagePassingServer::SessionHandle session_handle(pid, msg_server); const bool result = session_handle.AcquireRequest(); EXPECT_FALSE(result); - - EXPECT_CALL(*client_raw_ptr, Destruct()).Times(AnyNumber()); } // Covers OnConnectRequest "ConnectMessageFromClient too small" branch: @@ -1203,17 +1189,6 @@ TEST_F(MessagePassingServerFixture, OnConnectRequestStopRequestedCoversEarlyExit auto* server_for_test = &(*server); server_for_test->stop_source_.request_stop(); - // client_factory_->Create() is called before the stop check — set up the mock - auto client = score::cpp::pmr::make_unique>( - score::cpp::pmr::get_default_resource()); - auto* client_mock = client.get(); - EXPECT_CALL(*client_factory_mock, - Create(Matcher(_), - Matcher(_))) - .WillOnce(Return(ByMove(std::move(client)))); - // sender is destroyed at the early return inside OnConnectRequest - EXPECT_CALL(*client_mock, Destruct()).Times(1); - StrictMock<::score::message_passing::ServerConnectionMock> connection; score::message_passing::ClientIdentity client_identity{kClienT0Pid, 0, 0}; EXPECT_CALL(connection, GetClientIdentity()).Times(AnyNumber()).WillRepeatedly(ReturnRef(client_identity)); diff --git a/score/mw/log/detail/data_router/data_router_message_client_impl.cpp b/score/mw/log/detail/data_router/data_router_message_client_impl.cpp index a29eca48..b16454c6 100644 --- a/score/mw/log/detail/data_router/data_router_message_client_impl.cpp +++ b/score/mw/log/detail/data_router/data_router_message_client_impl.cpp @@ -182,6 +182,11 @@ void DatarouterMessageClientImpl::RequestInternalShutdown() noexcept void DatarouterMessageClientImpl::CheckExitRequestAndSendConnectMessage() noexcept { + // LCOV_EXCL_START : Defensive check for the rare race between the stop_requested() guard in + // ConnectToDatarouter() and this call site. ConnectToDatarouter() already returns early when + // stop is requested, so reaching this function with stop_requested() == true requires stop to + // be requested in the narrow window between the two checks. That window is not deterministically + // coverable in a unit test. if (stop_source_.stop_requested()) { ReportInitializationError(score::mw::log::detail::Error::kShutdownDuringInitialization, @@ -189,6 +194,7 @@ void DatarouterMessageClientImpl::CheckExitRequestAndSendConnectMessage() noexce msg_client_ids_.GetAppID().GetStringView()); return; } + // LCOV_EXCL_STOP SendConnectMessage(); }