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/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 diff --git a/score/datarouter/include/daemon/dlt_log_server.h b/score/datarouter/include/daemon/dlt_log_server.h index 598121ab..3c86fe1f 100644 --- a/score/datarouter/include/daemon/dlt_log_server.h +++ b/score/datarouter/include/daemon/dlt_log_server.h @@ -187,6 +187,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 +315,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/daemon/message_passing_server.h b/score/datarouter/include/daemon/message_passing_server.h index 6f897541..b95435fd 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" @@ -28,6 +27,7 @@ #include "score/concurrency/interruptible_wait.h" #include +#include #include #include #include @@ -35,6 +35,7 @@ #include #include #include +#include namespace score { @@ -68,25 +69,37 @@ class IMessagePassingServerSessionWrapper class MessagePassingServer : public IMessagePassingServerSessionWrapper { public: - class SessionHandle : public daemon::ISessionHandle + struct AcquireWatchdogConfig { - 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_{} + 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: - score::cpp::pmr::unique_ptr sender_; pid_t pid_; MessagePassingServer* server_; - mutable std::optional sender_state_; }; + /// 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: @@ -102,31 +115,39 @@ 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); + 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 class MessagePassingServerForTest; private: - void NotifyAcquireRequestFailed(std::int32_t pid); + friend class SessionHandle; + + bool NotifyAcquireRequest(pid_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)), + 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), @@ -165,6 +186,11 @@ class MessagePassingServer : public IMessagePassingServerSessionWrapper 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; @@ -187,13 +213,18 @@ 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 bool session_finishing_; std::shared_ptr server_factory_; - std::shared_ptr client_factory_; + + AcquireWatchdogConfig watchdog_config_; }; } // namespace internal 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..8d3145da 100644 --- a/score/datarouter/src/daemon/dlt_log_server.cpp +++ b/score/datarouter/src/daemon/dlt_log_server.cpp @@ -213,11 +213,13 @@ 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); diff --git a/score/datarouter/src/daemon/message_passing_server.cpp b/score/datarouter/src/daemon/message_passing_server.cpp index 4539bfdc..87fc1603 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 @@ -99,7 +101,7 @@ void MessagePassingServer::SessionWrapper::EnqueueTickWhileLocked() // 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)}, mutex_{}, @@ -112,7 +114,7 @@ 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]() { RunWorkerThread(); @@ -136,17 +138,25 @@ MessagePassingServer::MessagePassingServer(MessagePassingServer::SessionFactory MessagePassingConfig::kMaxQueuedNotifies}; receiver_ = server_factory_->Create(kServiceProtocolConfig, kServerConfig); - 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; wrapper.to_force_finish = true; found->second.EnqueueForDeleteWhileLocked(true); } @@ -154,8 +164,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 = @@ -246,6 +255,19 @@ void MessagePassingServer::RunWorkerThread() } else { + auto& wrapper = ps.second; + 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.EnqueueForDeleteWhileLocked(true); + continue; + } + } ps.second.EnqueueTickWhileLocked(); } } @@ -364,8 +386,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 +401,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; @@ -411,42 +437,12 @@ void MessagePassingServer::OnConnectRequest(const 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()}; - // LCOV_EXCL_START: false positive since it is tested. - std::string client_receiver_name; - // LCOV_EXCL_STOP - if (true == conn.GetUseDynamicIdentifier()) - { - /* - this is private functions so it cannot be test. - */ - // LCOV_EXCL_START - std::string random_part; - for (const auto& s : conn.GetRandomPart()) - { - random_part += s; - } - client_receiver_name = std::string("/logging-") + random_part; - // LCOV_EXCL_STOP - } - else - { - client_receiver_name = std::string("/logging.") + appid + "." + std::to_string(conn.GetUid()); - } - - score::cpp::pmr::memory_resource* memory_resource = score::cpp::pmr::get_default_resource(); - - const score::message_passing::ServiceProtocolConfig protocol_config{ - client_receiver_name, MessagePassingConfig::kMaxMessageSize, 0U, 0U}; - constexpr bool kTrulyAsyncSetToTrue = true; - const score::message_passing::IClientFactory::ClientConfig client_config{0, 10, false, kTrulyAsyncSetToTrue, false}; - - auto sender = client_factory_->Create(protocol_config, client_config); - // check for timeout or exit request if (stop_source_.stop_requested()) { @@ -462,8 +458,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) { @@ -481,13 +478,33 @@ void MessagePassingServer::OnConnectRequest(const score::cpp::span 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. + std::cerr << "MessagePassingServer: Session for pid " << pid << " already exists, dropping new session" + << std::endl; + 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.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 +512,10 @@ void MessagePassingServer::OnAcquireResponse(const score::cpp::span acq_span{static_cast(static_cast(&acq)), sizeof(acq)}; - std::ignore = std::copy(message.begin(), message.end(), acq_span.begin()); + 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.EnqueueTickWhileLocked(); } } -void MessagePassingServer::NotifyAcquireRequestFailed(std::int32_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()) { - /* - 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 + return false; } - found->second.EnqueueForDeleteWhileLocked(true); -} -bool MessagePassingServer::SessionHandle::AcquireRequest() const -{ - if (!sender_state_.has_value()) + auto& wrapper = found->second; + if (wrapper.connection == nullptr) { - sender_->Start(score::message_passing::IClientConnection::StateCallback{}, - score::message_passing::IClientConnection::NotifyCallback{}); + return false; } - sender_state_ = sender_->GetState(); - if (sender_state_ != score::message_passing::IClientConnection::State::kReady) + if (wrapper.acquire_in_flight) { - return false; + 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 = sender_->Send(kMessage); + auto ret = wrapper.connection->Notify(kMessage); + if (!ret) { - if (server_ != nullptr) + // 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) { - server_->NotifyAcquireRequestFailed(pid_); + 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 + { + wrapper.acquire_in_flight = true; + wrapper.acquire_deadline = TimestampT::clock::now() + watchdog_config_.deadline; } return true; } +bool MessagePassingServer::SessionHandle::AcquireRequest() const +{ + if (server_ == nullptr) + { + return false; + } + return server_->NotifyAcquireRequest(pid_); +} + } // namespace internal } // namespace platform } // namespace score diff --git a/score/datarouter/src/daemon/persistentlogging_config.cpp b/score/datarouter/src/daemon/persistentlogging_config.cpp index 5a585c1f..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 @@ -39,11 +40,20 @@ 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; - 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/src/daemon/socketserver.cpp b/score/datarouter/src/daemon/socketserver.cpp index b936338d..ddcba930 100644 --- a/score/datarouter/src/daemon/socketserver.cpp +++ b/score/datarouter/src/daemon/socketserver.cpp @@ -171,6 +171,7 @@ std::unique_ptr SocketServer::Crea { auto global_handlers = dlt_server_.GetGlobalHandlers(); score::platform::internal::LogParser::HandleRequestMap handle_request_map; + for (auto& binding : dlt_server_.GetTypeHandlerBindings()) { handle_request_map.emplace(std::move(binding.type_name), binding.handler); @@ -226,7 +227,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 +295,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 @@ -384,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: @@ -394,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_dltserver.cpp b/score/datarouter/test/ut/ut_logging/test_dltserver.cpp index 7eab7882..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" @@ -151,7 +155,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 +1010,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 +1018,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 +1154,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 +1183,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); @@ -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_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/ut/ut_logging/test_message_passing_server.cpp b/score/datarouter/test/ut/ut_logging/test_message_passing_server.cpp index da8447dc..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 @@ -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" @@ -28,6 +27,7 @@ #include #include +#include #include #include @@ -79,8 +79,9 @@ 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; +std::uint32_t gKReceiverQueueMaxSize = 1; class MockSession : public MessagePassingServer::ISession { @@ -153,18 +154,17 @@ 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, 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()); server_mock = server_ptr.get(); - EXPECT_CALL( - *server_factory_mock, - Create(CompareServiceProtocol(ServiceProtocolConfig{"/logging.datarouter_recv", kMaxSendBytes, 0U, 0U}), - 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)))); } @@ -212,11 +212,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()); @@ -254,7 +249,29 @@ 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, + 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, watchdog_config); } auto CreateConnectMessageSample(const pid_t) @@ -271,27 +288,25 @@ 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)))); + 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))); - 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)); + // Mirror production behavior: connect_callback runs before any messages. + std::ignore = connect_callback(*connection_ptr); auto message = CreateConnectMessageSample(pid); - sent_callback(connection, message); + sent_callback(*connection_ptr, message); - return client_mock; + return connection_ptr; } void UninstantiateServer() @@ -304,11 +319,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{}; @@ -320,14 +335,13 @@ 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)))); } StrictMock<::score::message_passing::ServerMock>* server_mock{}; - std::shared_ptr> client_factory_mock; std::shared_ptr> server_factory_mock; ::score::os::MockGuard unistd_mock{}; @@ -337,6 +351,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; @@ -395,7 +412,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(); @@ -413,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(); } @@ -427,40 +444,37 @@ 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(); 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); + { + 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()); - 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 @@ -484,15 +498,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); @@ -515,22 +529,34 @@ 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); + { + 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)); - ExpectClientDestruction(client1); 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"; + } - 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); @@ -543,6 +569,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) { @@ -553,32 +621,35 @@ 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(); - // 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)); - ExpectClientDestruction(client0); this->disconnect_callback(connection); - std::this_thread::sleep_for(100ms); - auto* new_client = ExpectConnectCallBackCalledAndClientCreated(kClienT0Pid); - ExpectClientDestruction(new_client); + // 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); + + auto* new_connection = ConnectClientAndSendConnectMessage(kClienT0Pid); + std::ignore = new_connection; }); EXPECT_EQ(destruct_count, 0); // no destruction while we are still in the tick @@ -591,8 +662,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); @@ -608,9 +680,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(); @@ -618,21 +690,25 @@ 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)); - ExpectClientDestruction(client2); this->disconnect_callback(connection); - std::this_thread::sleep_for(100ms); - auto* new_client = ExpectConnectCallBackCalledAndClientCreated(kClienT2Pid); - ExpectClientDestruction(new_client); + // 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); + + auto* new_connection = ConnectClientAndSendConnectMessage(kClienT2Pid); + std::ignore = new_connection; }); EXPECT_EQ(destruct_count, 0); // no destruction while we are still in the tick @@ -645,14 +721,177 @@ 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); 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); + ASSERT_TRUE(map_cond.wait_for(lock, std::chrono::seconds{1}, [this]() { + return session_map.empty(); + })) << "Timed out waiting for watchdog teardown"; + } + + ExpectServerDestruction(); + 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(100ms); + + // 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(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{1}, [this]() { + return session_map.empty(); + })) << "Timed out waiting for watchdog teardown after miss-count reset"; + } + + ExpectServerDestruction(); + 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(50ms); + 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(50ms); + 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{1}, [this]() { + return session_map.empty(); + })) << "Timed out waiting for session teardown after non-ENOBUFS failure"; + } + + ExpectServerDestruction(); + 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; @@ -680,25 +919,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 @@ -865,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: @@ -978,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/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/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 26fa28f9..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 @@ -19,10 +19,9 @@ #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 namespace score { @@ -52,7 +51,6 @@ DatarouterMessageClientImpl::DatarouterMessageClientImpl(const MsgClientIdentifi state_condition_{}, sender_state_{}, sender_{nullptr}, - receiver_{nullptr}, connect_thread_{} { } @@ -68,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(); } @@ -94,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_]() { @@ -123,12 +120,6 @@ void DatarouterMessageClientImpl::ConnectToDatarouter() noexcept } // LCOV_EXCL_STOP - if (StartReceiver() == false) - { - RequestInternalShutdown(); - return; - } - CheckExitRequestAndSendConnectMessage(); } @@ -181,73 +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 { - 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); - }; - 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. @@ -258,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, @@ -265,6 +194,7 @@ void DatarouterMessageClientImpl::CheckExitRequestAndSendConnectMessage() noexce msg_client_ids_.GetAppID().GetStringView()); return; } + // LCOV_EXCL_STOP SendConnectMessage(); } @@ -323,7 +253,6 @@ void DatarouterMessageClientImpl::Shutdown() noexcept connect_thread_.join(); } - receiver_.reset(); { std::unique_lock lock(sender_mutex_); sender_.reset(); @@ -361,13 +290,36 @@ 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 { + 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. @@ -433,11 +385,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 5109ed2b..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; @@ -99,6 +91,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; @@ -129,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 38704437..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 || @@ -105,7 +90,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 kMaxNumberMessagesInReceiverQueue{0UL}; +constexpr std::uint32_t kMaxNotifyBytes{1U}; class DatarouterMessageClientFixture : public ::testing::Test { @@ -169,70 +154,17 @@ 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) + 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 +175,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(); @@ -264,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, @@ -319,49 +251,32 @@ 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) + 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) { - // 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); + const std::array message{score::cpp::to_underlying(DatarouterMessageIdentifier::kAcquireRequest)}; + 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(); @@ -373,20 +288,11 @@ 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; } - 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) @@ -424,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}; @@ -484,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"); @@ -606,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(); @@ -625,116 +472,88 @@ TEST_F(DatarouterMessageClientFixture, ConnectToDatarouterShouldSendConnectMessa connect_thread.join(); } -TEST_F(DatarouterMessageClientFixture, ConnectToDatarouterGivenThatReceiverFailedShouldNotSendConnectMessage) +TEST_F(DatarouterMessageClientFixture, AcquireNotifyShouldSendExpectedAcquireResponse) { RecordProperty("ASIL", "B"); - RecordProperty("Description", "Verifies the in-ability of sending connect message when receiver fails."); + 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::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())); + score::message_passing::IClientConnection::NotifyCallback notify_callback; - ExpectUnlinkMwsrWriterFile(); + ExpectSenderCreationSequence(&sender_ptr, &state_callback, nullptr, true, ¬ify_callback); - 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(); - }); + ExecuteCreateSenderSequence(&state_callback); - callback_registered.get_future().wait(); - state_callback(score::message_passing::IClientConnection::State::kReady); + bool first_message = true; + SendAcquireNotifyAndExpectResponse(notify_callback, &sender_ptr, first_message, false); - connect_thread.join(); + ExpectClientDestruction(sender_ptr); } -TEST_F(DatarouterMessageClientFixture, AcquireRequestShouldSendExpectedAcquireResponse) +TEST_F(DatarouterMessageClientFixture, InvalidNotifyShouldNotSendAcquireResponse) { RecordProperty("ASIL", "B"); - RecordProperty("Description", "Verifies that acquired request shall send the expected acquired response."); + 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::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); + + 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); - bool first_message = true; - SendAcquireRequestAndExpectResponse(sent_callback, &sender_ptr, first_message, false); - 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"); @@ -745,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()); @@ -771,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); } @@ -788,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(); @@ -817,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(); @@ -873,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(); @@ -900,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(); } @@ -932,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(); @@ -964,17 +738,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", "Error guessing based on knowledge or experience"); 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}; + kDatarouterReceiverIdentifier, kMaxSendBytes, 0U, kMaxNotifyBytes}; const score::message_passing::IClientFactory::ClientConfig client_config{0, 10, false, true, false}; EXPECT_CALL(*message_passing_factory_, @@ -982,275 +758,58 @@ 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(); -} - -TEST_F(DatarouterMessageClientFixture, DisconnectCallbackRequestsInternalShutdown) -{ - 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(); - - 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; - - 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); - - client_->SetupReceiver(); - client_->ConnectToDatarouter(); EXPECT_TRUE(stop_source_.stop_requested()); } -TEST_F(DatarouterMessageClientFixture, ReceivedSendMessageWithReplyCallbackDoesNothing) +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); - }); - - 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); + const auto result = client_->CreateSender(); - client_->SetupReceiver(); - client_->ConnectToDatarouter(); + ASSERT_FALSE(result.has_value()); } - -TEST_F(DatarouterMessageClientFixture, StopRequestedDuringReceiverStartReportsShutdownDuringInitialization) +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(); - - 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_)))); + testing::InSequence order_matters; - 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); - }); + score::message_passing::ClientConnectionMock* sender_ptr{}; + score::message_passing::IClientConnection::StateCallback state_callback; - // SendConnectMessage must never be reached - EXPECT_CALL(*sender_ptr, Send(Matcher>(_))).Times(0); + ExpectSenderCreationSequence(&sender_ptr, &state_callback); - 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{}; - }); + ExecuteCreateSenderSequence(&state_callback); ExpectUnlinkMwsrWriterFile(); - ExpectServerDestruction(receiver_ptr); - ExpectClientDestruction(sender_ptr); - client_->SetupReceiver(); - client_->ConnectToDatarouter(); + state_callback(score::message_passing::IClientConnection::State::kStopped); EXPECT_TRUE(stop_source_.stop_requested()); -} - -TEST_F(DatarouterMessageClientFixture, CreateSenderFailsWhenFactoryReturnsNullptr) -{ - auto* receiver_ptr = ExpectReceiverCreated(); - - ExpectBlockTerminationSignalPass(); - ExpectSetLoggerThreadName(); - - 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}; - - // 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))); - - // 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); - ExpectUnlinkMwsrWriterFile(); - ExpectServerDestruction(receiver_ptr); - - client_->SetupReceiver(); - client_->ConnectToDatarouter(); - - 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 7242bd99..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,20 +32,20 @@ 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}; /// \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"};