From 63f72b56724669e681538b36b76cfb08f5d74000 Mon Sep 17 00:00:00 2001 From: tejveerpratap Date: Thu, 30 Apr 2026 08:00:57 +0200 Subject: [PATCH 1/2] Send notification to Skeleton on Proxy destruction to "Unsubscribe" method - Add OnProxyMethodUnsubscribeFinished to notify SkeletonMethod when Proxy is destroyed - Add MethodUnsubscriptionRegistrationGuard for cleanup management - Add MethodResourceMap for tracking method resource ownership per proxy instance - Refactor: use ProxyMethodInstanceIdentifier as key in registration_guards_ (resolves intermediate solution from issue-250236) --- .../mw/com/impl/bindings/lola/messaging/BUILD | 18 ++ .../messaging/i_message_passing_service.h | 61 ++++ .../i_message_passing_service_instance.h | 11 + .../messaging/message_passing_service.cpp | 38 +++ .../lola/messaging/message_passing_service.h | 20 ++ .../message_passing_service_instance.cpp | 148 ++++++++- .../message_passing_service_instance.h | 33 +- ..._passing_service_instance_methods_test.cpp | 296 ++++++++++++++++++ .../message_passing_service_instance_mock.h | 12 + .../messaging/message_passing_service_mock.h | 13 + .../message_passing_service_test.cpp | 137 ++++++++ ...thod_unsubscription_registration_guard.cpp | 35 +++ ...method_unsubscription_registration_guard.h | 45 +++ ...unsubscription_registration_guard_test.cpp | 116 +++++++ .../lola/methods/method_resource_map.cpp | 16 + .../lola/methods/method_resource_map.h | 7 + .../lola/methods/method_resource_map_test.cpp | 65 ++++ score/mw/com/impl/bindings/lola/proxy.cpp | 47 ++- score/mw/com/impl/bindings/lola/proxy.h | 6 + .../lola/proxy_method_handling_test.cpp | 165 +++++++++- score/mw/com/impl/bindings/lola/skeleton.cpp | 60 ++++ score/mw/com/impl/bindings/lola/skeleton.h | 10 + .../impl/bindings/lola/skeleton_method.cpp | 52 +-- .../com/impl/bindings/lola/skeleton_method.h | 17 +- .../lola/skeleton_method_handling_test.cpp | 235 ++++++++++++++ .../bindings/lola/skeleton_method_test.cpp | 94 ++++++ 26 files changed, 1716 insertions(+), 41 deletions(-) create mode 100644 score/mw/com/impl/bindings/lola/messaging/method_unsubscription_registration_guard.cpp create mode 100644 score/mw/com/impl/bindings/lola/messaging/method_unsubscription_registration_guard.h create mode 100644 score/mw/com/impl/bindings/lola/messaging/method_unsubscription_registration_guard_test.cpp diff --git a/score/mw/com/impl/bindings/lola/messaging/BUILD b/score/mw/com/impl/bindings/lola/messaging/BUILD index 28f6e8c86..0611230ab 100644 --- a/score/mw/com/impl/bindings/lola/messaging/BUILD +++ b/score/mw/com/impl/bindings/lola/messaging/BUILD @@ -37,11 +37,13 @@ cc_library( "i_message_passing_service.cpp", "method_call_registration_guard.cpp", "method_subscription_registration_guard.cpp", + "method_unsubscription_registration_guard.cpp", ], hdrs = [ "i_message_passing_service.h", "method_call_registration_guard.h", "method_subscription_registration_guard.h", + "method_unsubscription_registration_guard.h", ], features = COMPILER_WARNING_FEATURES, implementation_deps = [ @@ -370,6 +372,21 @@ cc_gtest_unit_test( ], ) +cc_gtest_unit_test( + name = "method_unsubscription_registration_guard_test", + srcs = ["method_unsubscription_registration_guard_test.cpp"], + features = COMPILER_WARNING_FEATURES, + visibility = [ + "//score/mw/com/impl:__subpackages__", + ], + deps = [ + ":i_message_passing_service", + ":message_passing_service_mock", + "@score_baselibs//score/mw/log:recorder_mock", + "@score_baselibs//score/scope_exit", + ], +) + cc_gtest_unit_test( name = "method_call_registration_guard_test", srcs = ["method_call_registration_guard_test.cpp"], @@ -412,6 +429,7 @@ cc_unit_test_suites_for_host_and_qnx( ":message_passing_service_test", ":mw_log_logger_test", ":method_subscription_registration_guard_test", + ":method_unsubscription_registration_guard_test", ":method_call_registration_guard_test", ], visibility = ["//score/mw/com/impl/bindings/lola:__pkg__"], diff --git a/score/mw/com/impl/bindings/lola/messaging/i_message_passing_service.h b/score/mw/com/impl/bindings/lola/messaging/i_message_passing_service.h index 962d49d91..1a5feab82 100644 --- a/score/mw/com/impl/bindings/lola/messaging/i_message_passing_service.h +++ b/score/mw/com/impl/bindings/lola/messaging/i_message_passing_service.h @@ -16,6 +16,7 @@ #include "score/mw/com/impl/bindings/lola/element_fq_id.h" #include "score/mw/com/impl/bindings/lola/messaging/method_call_registration_guard.h" #include "score/mw/com/impl/bindings/lola/messaging/method_subscription_registration_guard.h" +#include "score/mw/com/impl/bindings/lola/messaging/method_unsubscription_registration_guard.h" #include "score/mw/com/impl/bindings/lola/methods/proxy_method_instance_identifier.h" #include "score/mw/com/impl/bindings/lola/proxy_instance_identifier.h" #include "score/mw/com/impl/bindings/lola/skeleton_instance_identifier.h" @@ -47,6 +48,7 @@ class IMessagePassingService { friend class MethodSubscriptionRegistrationGuardFactory; friend class MethodCallRegistrationGuardFactory; + friend class MethodUnsubscriptionRegistrationGuardFactory; public: using HandlerRegistrationNoType = std::uint32_t; @@ -86,6 +88,19 @@ class IMessagePassingService using ServiceMethodSubscribedHandler = safecpp::CopyableScopedFunction< score::Result(ProxyInstanceIdentifier proxy_instance_identifier, uid_t proxy_uid, pid_t proxy_pid)>; + /// \brief Handler which will be called when the proxy process sends a message that it has unsubscribed from a + /// service method. + /// + /// When a Proxy is destroyed, it will call UnsubscribeServiceMethod which will send a message to the Skeleton + /// process. The Skeleton process will then call the ServiceMethodUnsubscribedHandler registered in + /// RegisterOnServiceMethodUnsubscribedHandler. The handler should close the proxy's shared memory region and + /// unregister the method call handler corresponding to the proxy's ProxyMethodInstanceIdentifier from each + /// SkeletonMethod. + /// + /// \param proxy_instance_identifier unique identifier of the Proxy instance which unsubscribed from the method. + using ServiceMethodUnsubscribedHandler = + safecpp::CopyableScopedFunction; + /// \brief Handler which will be called when the proxy process sends a message that it has called a method. /// /// This will be triggered when the Proxy process calls CallMethod. @@ -197,6 +212,24 @@ class IMessagePassingService ServiceMethodSubscribedHandler subscribed_callback, AllowedConsumerUids allowed_proxy_uids) = 0; + /// \brief Register a handler on Skeleton side which will be called when UnsubscribeServiceMethod is called by a + /// Proxy. + /// + /// When a Proxy is destroyed, it will call UnsubscribeServiceMethod. When this message is received in the + /// Skeleton process, the handler registered in this function will be called. + /// + /// The handler is scoped to on_service_method_subscribed_handler_scope_ on the Skeleton side, so that it is + /// not called after the Skeleton has started its StopOffer sequence. This is our synchronisation point. + /// + /// \param asil_level ASIL level of method. + /// \param skeleton_instance_identifier to identify which ServiceMethodUnsubscribedHandler to call when + /// UnsubscribeServiceMethod is called on the Proxy side + /// \param unsubscribed_callback callback that will be called when UnsubscribeServiceMethod is called + virtual Result RegisterOnServiceMethodUnsubscribedHandler( + const QualityType asil_level, + const SkeletonInstanceIdentifier skeleton_instance_identifier, + ServiceMethodUnsubscribedHandler unsubscribed_callback) = 0; + /// \brief Register a handler on Skeleton side which will be called when CallMethod is called by a ProxyMethod. /// /// When a user calls a method on a ProxyMethod, it will put the InArgs in shared memory (if there are any) and then @@ -283,6 +316,21 @@ class IMessagePassingService const ProxyInstanceIdentifier& proxy_instance_identifier, const pid_t target_node_id) = 0; + /// \brief Best-effort call made by a Proxy on destruction to notify the Skeleton that the proxy is being + /// destroyed so that the Skeleton can clean up the proxy's shared memory region. + /// + /// This mirrors SubscribeServiceMethod. If this call fails, some resources will be leaked on the Skeleton side + /// but will be cleaned up when the Proxy process restarts and the Skeleton detects the PID change. + /// + /// \param asil_level ASIL level of method. + /// \param skeleton_instance_identifier identification of the Skeleton corresponding to the Proxy. + /// \param proxy_instance_identifier identification of the Proxy which is being destroyed. + /// \param target_node_id PID of the Skeleton process which the unsubscribe call is sent to. + virtual ResultBlank UnsubscribeServiceMethod(const QualityType asil_level, + const SkeletonInstanceIdentifier& skeleton_instance_identifier, + const ProxyInstanceIdentifier& proxy_instance_identifier, + const pid_t target_node_id) = 0; + /// \brief Blocking call which is called on Proxy side to trigger the Skeleton to process a method call. The /// callback registered with RegisterOnServiceMethodSubscribed will be called on the Skeleton side and a response /// will be returned @@ -330,6 +378,19 @@ class IMessagePassingService /// \param proxy_method_instance_identifier to identify which registered MethodCallHandler to unregister virtual void UnregisterMethodCallHandler(const QualityType asil_level, ProxyMethodInstanceIdentifier proxy_method_instance_identifier) = 0; + + /// \brief Unregister handler that was registered with RegisterOnServiceMethodUnsubscribedHandler + /// + /// This function is private and will only be called by MethodUnsubscriptionRegistrationGuardFactory on + /// destruction. + /// + /// \pre Shall only be called after RegisterOnServiceMethodUnsubscribedHandler was successfully called. + /// + /// \param asil_level ASIL level of method. + /// \param skeleton_instance_identifier to identify which registered ServiceMethodUnsubscribedHandler to unregister + virtual void UnregisterOnServiceMethodUnsubscribedHandler( + const QualityType asil_level, + SkeletonInstanceIdentifier skeleton_instance_identifier) = 0; }; } // namespace score::mw::com::impl::lola diff --git a/score/mw/com/impl/bindings/lola/messaging/i_message_passing_service_instance.h b/score/mw/com/impl/bindings/lola/messaging/i_message_passing_service_instance.h index bd7ca92bc..1996e2233 100644 --- a/score/mw/com/impl/bindings/lola/messaging/i_message_passing_service_instance.h +++ b/score/mw/com/impl/bindings/lola/messaging/i_message_passing_service_instance.h @@ -54,6 +54,10 @@ class IMessagePassingServiceInstance IMessagePassingService::ServiceMethodSubscribedHandler subscribed_callback, IMessagePassingService::AllowedConsumerUids allowed_proxy_uids) = 0; + virtual ResultBlank RegisterOnServiceMethodUnsubscribedHandler( + const SkeletonInstanceIdentifier skeleton_instance_identifier, + IMessagePassingService::ServiceMethodUnsubscribedHandler unsubscribed_callback) = 0; + virtual Result RegisterMethodCallHandler(const ProxyMethodInstanceIdentifier proxy_method_instance_identifier, IMessagePassingService::MethodCallHandler method_call_callback, const uid_t allowed_proxy_uid) = 0; @@ -61,6 +65,9 @@ class IMessagePassingServiceInstance virtual void UnregisterOnServiceMethodSubscribedHandler( SkeletonInstanceIdentifier skeleton_instance_identifier) = 0; + virtual void UnregisterOnServiceMethodUnsubscribedHandler( + SkeletonInstanceIdentifier skeleton_instance_identifier) = 0; + virtual void UnregisterMethodCallHandler(ProxyMethodInstanceIdentifier proxy_method_instance_identifier) = 0; virtual void NotifyOutdatedNodeId(const pid_t outdated_node_id, const pid_t target_node_id) noexcept = 0; @@ -75,6 +82,10 @@ class IMessagePassingServiceInstance const ProxyInstanceIdentifier& proxy_instance_identifier, const pid_t target_node_id) = 0; + virtual ResultBlank UnsubscribeServiceMethod(const SkeletonInstanceIdentifier& skeleton_instance_identifier, + const ProxyInstanceIdentifier& proxy_instance_identifier, + const pid_t target_node_id) = 0; + virtual Result CallMethod(const ProxyMethodInstanceIdentifier& proxy_method_instance_identifier, const std::size_t queue_position, const pid_t target_node_id) = 0; diff --git a/score/mw/com/impl/bindings/lola/messaging/message_passing_service.cpp b/score/mw/com/impl/bindings/lola/messaging/message_passing_service.cpp index b7c37b3e3..be871ee0c 100644 --- a/score/mw/com/impl/bindings/lola/messaging/message_passing_service.cpp +++ b/score/mw/com/impl/bindings/lola/messaging/message_passing_service.cpp @@ -15,6 +15,7 @@ #include "score/mw/com/impl/bindings/lola/messaging/message_passing_service_instance_factory.h" #include "score/mw/com/impl/bindings/lola/messaging/method_call_registration_guard.h" #include "score/mw/com/impl/bindings/lola/messaging/method_subscription_registration_guard.h" +#include "score/mw/com/impl/bindings/lola/messaging/method_unsubscription_registration_guard.h" #include "score/mw/com/impl/bindings/lola/messaging/mw_log_logger.h" #include "score/mw/com/impl/bindings/lola/messaging/thread_abstraction.h" @@ -164,6 +165,23 @@ Result MessagePassingService::RegisterOnSer *this, asil_level, skeleton_instance_identifier, registration_guards_scope_); } +Result MessagePassingService::RegisterOnServiceMethodUnsubscribedHandler( + const QualityType asil_level, + SkeletonInstanceIdentifier skeleton_instance_identifier, + ServiceMethodUnsubscribedHandler unsubscribed_callback) +{ + auto& instance = GetMessagePassingServiceInstance(asil_level); + + const auto result = instance.RegisterOnServiceMethodUnsubscribedHandler(skeleton_instance_identifier, + std::move(unsubscribed_callback)); + if (!(result.has_value())) + { + return MakeUnexpected(result.error()); + } + return MethodUnsubscriptionRegistrationGuardFactory::Create( + *this, asil_level, skeleton_instance_identifier, registration_guards_scope_); +} + Result MessagePassingService::RegisterMethodCallHandler( const QualityType asil_level, ProxyMethodInstanceIdentifier proxy_method_instance_identifier, @@ -211,6 +229,17 @@ Result MessagePassingService::SubscribeServiceMethod( return instance.SubscribeServiceMethod(skeleton_instance_identifier, proxy_instance_identifier, target_node_id); } +ResultBlank MessagePassingService::UnsubscribeServiceMethod( + const QualityType asil_level, + const SkeletonInstanceIdentifier& skeleton_instance_identifier, + const ProxyInstanceIdentifier& proxy_instance_identifier, + const pid_t target_node_id) +{ + auto& instance = GetMessagePassingServiceInstance(asil_level); + + return instance.UnsubscribeServiceMethod(skeleton_instance_identifier, proxy_instance_identifier, target_node_id); +} + Result MessagePassingService::CallMethod(const QualityType asil_level, const ProxyMethodInstanceIdentifier& proxy_method_instance_identifier, std::size_t queue_position, @@ -230,6 +259,15 @@ void MessagePassingService::UnregisterOnServiceMethodSubscribedHandler( instance.UnregisterOnServiceMethodSubscribedHandler(skeleton_instance_identifier); } +void MessagePassingService::UnregisterOnServiceMethodUnsubscribedHandler( + const QualityType asil_level, + SkeletonInstanceIdentifier skeleton_instance_identifier) +{ + auto& instance = GetMessagePassingServiceInstance(asil_level); + + instance.UnregisterOnServiceMethodUnsubscribedHandler(skeleton_instance_identifier); +} + void MessagePassingService::UnregisterMethodCallHandler(const QualityType asil_level, ProxyMethodInstanceIdentifier proxy_method_instance_identifier) { diff --git a/score/mw/com/impl/bindings/lola/messaging/message_passing_service.h b/score/mw/com/impl/bindings/lola/messaging/message_passing_service.h index 4b33f4d48..4fa7658fd 100644 --- a/score/mw/com/impl/bindings/lola/messaging/message_passing_service.h +++ b/score/mw/com/impl/bindings/lola/messaging/message_passing_service.h @@ -19,6 +19,7 @@ #include "score/mw/com/impl/bindings/lola/messaging/i_message_passing_service_instance.h" #include "score/mw/com/impl/bindings/lola/messaging/i_message_passing_service_instance_factory.h" #include "score/mw/com/impl/bindings/lola/messaging/message_passing_service_instance.h" +#include "score/mw/com/impl/bindings/lola/messaging/method_unsubscription_registration_guard.h" #include "score/mw/com/impl/bindings/lola/proxy_instance_identifier.h" #include "score/mw/com/impl/bindings/lola/skeleton_instance_identifier.h" #include "score/mw/com/impl/configuration/global_configuration.h" @@ -108,6 +109,14 @@ class MessagePassingService final : public IMessagePassingService ServiceMethodSubscribedHandler subscribed_callback, AllowedConsumerUids allowed_proxy_uids) override; + /// \brief Register a handler on Skeleton side which will be called when UnsubscribeServiceMethod is called by a + /// Proxy. + /// \details see IMessagePassingService::RegisterOnServiceMethodUnsubscribedHandler + Result RegisterOnServiceMethodUnsubscribedHandler( + const QualityType asil_level, + SkeletonInstanceIdentifier skeleton_instance_identifier, + ServiceMethodUnsubscribedHandler unsubscribed_callback) override; + /// \brief Register a handler on Skeleton side which will be called when CallMethod is called by a Proxy. /// \details see IMessagePassingService::RegisterMethodCallHandler Result RegisterMethodCallHandler( @@ -144,6 +153,14 @@ class MessagePassingService final : public IMessagePassingService const ProxyInstanceIdentifier& proxy_instance_identifier, const pid_t target_node_id) override; + /// \brief Best-effort call made by a Proxy on destruction to notify the Skeleton that the proxy is being + /// destroyed. + /// \details see IMessagePassingService::UnsubscribeServiceMethod + ResultBlank UnsubscribeServiceMethod(const QualityType asil_level, + const SkeletonInstanceIdentifier& skeleton_instance_identifier, + const ProxyInstanceIdentifier& proxy_instance_identifier, + const pid_t target_node_id) override; + /// \brief Blocking call which is called on Proxy side to trigger the Skeleton to process a method call. The /// callback registered with RegisterOnServiceMethodSubscribed will be called on the Skeleton side and a response /// will be returned. @@ -161,6 +178,9 @@ class MessagePassingService final : public IMessagePassingService void UnregisterOnServiceMethodSubscribedHandler(const QualityType asil_level, SkeletonInstanceIdentifier skeleton_instance_identifier) override; + void UnregisterOnServiceMethodUnsubscribedHandler(const QualityType asil_level, + SkeletonInstanceIdentifier skeleton_instance_identifier) override; + void UnregisterMethodCallHandler(const QualityType asil_level, ProxyMethodInstanceIdentifier proxy_method_instance_identifier) override; diff --git a/score/mw/com/impl/bindings/lola/messaging/message_passing_service_instance.cpp b/score/mw/com/impl/bindings/lola/messaging/message_passing_service_instance.cpp index b3b82ff76..adeabe796 100644 --- a/score/mw/com/impl/bindings/lola/messaging/message_passing_service_instance.cpp +++ b/score/mw/com/impl/bindings/lola/messaging/message_passing_service_instance.cpp @@ -66,6 +66,12 @@ struct SubscribeServiceMethodUnserializedPayload ProxyInstanceIdentifier proxy_instance_identifier; }; +struct UnsubscribeServiceMethodUnserializedPayload +{ + SkeletonInstanceIdentifier skeleton_instance_identifier; + ProxyInstanceIdentifier proxy_instance_identifier; +}; + struct MethodCallUnserializedPayload { ProxyMethodInstanceIdentifier proxy_method_instance_identifier; @@ -411,6 +417,10 @@ score::Result MessagePassingServiceInstance::MessageCallbackWithReply( { return HandleSubscribeServiceMethodMsg(payload, sender_uid, sender_pid); } + case score::cpp::to_underlying(MessageWithReplyType::kUnsubscribeServiceMethod): + { + return HandleUnsubscribeServiceMethodMsg(payload, sender_pid); + } case score::cpp::to_underlying(MessageWithReplyType::kCallMethod): { return HandleCallMethodMsg(payload, sender_uid); @@ -605,6 +615,20 @@ score::Result MessagePassingServiceInstance::HandleSubscribeServiceMethodM sender_node_id); } +score::ResultBlank MessagePassingServiceInstance::HandleUnsubscribeServiceMethodMsg( + const score::cpp::span payload, + const pid_t /*sender_node_id*/) +{ + UnsubscribeServiceMethodUnserializedPayload unserialized_payload{}; + if (!DeserializeFromPayload(payload, unserialized_payload)) + { + return MakeUnexpected(MethodErrc::kUnexpectedMessageSize); + } + + return CallUnsubscribeServiceMethodLocally(unserialized_payload.skeleton_instance_identifier, + unserialized_payload.proxy_instance_identifier); +} + score::Result MessagePassingServiceInstance::HandleCallMethodMsg( const score::cpp::span payload, const uid_t sender_uid) @@ -661,6 +685,36 @@ score::Result MessagePassingServiceInstance::CallSubscribeServiceMethodLoc return invocation_result.value(); } +score::ResultBlank MessagePassingServiceInstance::CallUnsubscribeServiceMethodLocally( + const SkeletonInstanceIdentifier& skeleton_instance_identifier, + const ProxyInstanceIdentifier& proxy_instance_identifier) +{ + // A copy of the handler is made under lock and called outside the lock. See CallSubscribeServiceMethodLocally for + // a detailed explanation of the pattern. + std::shared_lock read_lock{unsubscribe_service_method_handlers_mutex_}; + auto handler_it = unsubscribe_service_method_handlers_.find(skeleton_instance_identifier); + if (handler_it == unsubscribe_service_method_handlers_.cend()) + { + // The skeleton may have already called StopOffer, which expired the scope used for the handler, or may not + // have registered a handler at all. This is a best-effort call so we treat this as success. + mw::log::LogInfo("lola") << "Unsubscribe method handler has not been registered for this SkeletonMethod. " + "Skeleton may have already stopped offering."; + return {}; + } + + auto handler_copy = handler_it->second; + read_lock.unlock(); + + const auto invocation_result = std::invoke(handler_copy, proxy_instance_identifier); + if (!(invocation_result.has_value())) + { + mw::log::LogInfo("lola") << "Invocation of unsubscribe service method handler was a no-op: scope has been " + "destroyed (skeleton already stopped offering)."; + return {}; + } + return invocation_result.value(); +} + score::Result MessagePassingServiceInstance::CallServiceMethodLocally( const ProxyMethodInstanceIdentifier& proxy_method_instance_identifier, const std::size_t queue_position, @@ -734,6 +788,39 @@ Result MessagePassingServiceInstance::CallSubscribeServiceMethodRemotely( return deserialization_result.value(); } +ResultBlank MessagePassingServiceInstance::CallUnsubscribeServiceMethodRemotely( + const SkeletonInstanceIdentifier& skeleton_instance_identifier, + const ProxyInstanceIdentifier& proxy_instance_identifier, + const pid_t target_node_id) +{ + UnsubscribeServiceMethodUnserializedPayload unserialized_payload{skeleton_instance_identifier, + proxy_instance_identifier}; + const auto message = SerializeToMessage(score::cpp::to_underlying(MessageWithReplyType::kUnsubscribeServiceMethod), + unserialized_payload); + auto sender = client_cache_.GetMessagePassingClient(target_node_id); + + std::array reply{}; + score::cpp::span reply_buffer{reply.data(), reply.size()}; + const auto method_reply_result = sender->SendWaitReply(message, reply_buffer); + if (!method_reply_result.has_value()) + { + score::mw::log::LogError("lola") + << "MessagePassingServiceInstance: Sending UnsubscribeServiceMethodMessage to node_id " << target_node_id + << " failed with error: " << method_reply_result.error(); + return MakeUnexpected(MethodErrc::kMessagePassingError); + } + + const auto deserialization_result = DeserializeFromMethodReplyPayload(method_reply_result.value()); + if (!(deserialization_result.has_value())) + { + score::mw::log::LogError("lola") + << "MessagePassingService: Parsing UnsubscribeServiceMethodMessage reply from node_id " << target_node_id + << "failed during deserialization"; + return MakeUnexpected(deserialization_result.error()); + } + return deserialization_result.value(); +} + Result MessagePassingServiceInstance::CallServiceMethodRemotely( const ProxyMethodInstanceIdentifier& proxy_method_instance_identifier, const std::size_t queue_position, @@ -1141,6 +1228,25 @@ Result MessagePassingServiceInstance::RegisterOnServiceMethodSubscribedHan return {}; } +ResultBlank MessagePassingServiceInstance::RegisterOnServiceMethodUnsubscribedHandler( + const SkeletonInstanceIdentifier skeleton_instance_identifier, + IMessagePassingService::ServiceMethodUnsubscribedHandler unsubscribed_callback) +{ + std::unique_lock write_lock(unsubscribe_service_method_handlers_mutex_); + const auto [existing_element, was_inserted] = + unsubscribe_service_method_handlers_.insert({skeleton_instance_identifier, std::move(unsubscribed_callback)}); + + if (!was_inserted) + { + score::mw::log::LogError("lola") + << "MessagePassingService: Failed to register OnServiceMethodUnsubscribedHandler " + "since it could not be inserted into map."; + return MakeUnexpected(ComErrc::kBindingFailure); + } + + return {}; +} + Result MessagePassingServiceInstance::RegisterMethodCallHandler( const ProxyMethodInstanceIdentifier proxy_method_instance_identifier, IMessagePassingService::MethodCallHandler method_call_callback, @@ -1168,6 +1274,16 @@ void MessagePassingServiceInstance::UnregisterOnServiceMethodSubscribedHandler( "Function must only be called when a subscribe service method handler was successfully registered!"); } +void MessagePassingServiceInstance::UnregisterOnServiceMethodUnsubscribedHandler( + const SkeletonInstanceIdentifier skeleton_instance_identifier) +{ + std::unique_lock write_lock(unsubscribe_service_method_handlers_mutex_); + const auto num_elements_erased = unsubscribe_service_method_handlers_.erase(skeleton_instance_identifier); + SCORE_LANGUAGE_FUTURECPP_PRECONDITION_PRD_MESSAGE( + num_elements_erased != 0U, + "Function must only be called when an unsubscribe service method handler was successfully registered!"); +} + void MessagePassingServiceInstance::UnregisterMethodCallHandler( const ProxyMethodInstanceIdentifier proxy_method_instance_identifier) { @@ -1210,8 +1326,8 @@ void MessagePassingServiceInstance::RegisterEventNotificationRemote(const Elemen { score::mw::log::LogError("lola") << "MessagePassingService: RegisterEventNotificationRemote called for event" << event_id.ToString() - << "and node_id" << target_node_id << "although event is " - << " currently located at node" << registration_count_inserted.first->second.node_id; + << "and node_id" << target_node_id << "although event is " << " currently located at node" + << registration_count_inserted.first->second.node_id; registration_count_inserted.first->second.node_id = target_node_id; registration_count_inserted.first->second.counter = 1U; } @@ -1416,6 +1532,34 @@ Result MessagePassingServiceInstance::SubscribeServiceMethod( } } +ResultBlank MessagePassingServiceInstance::UnsubscribeServiceMethod( + const SkeletonInstanceIdentifier& skeleton_instance_identifier, + const ProxyInstanceIdentifier& proxy_instance_identifier, + const pid_t target_node_id) +{ + const auto are_skeleton_and_proxy_in_same_process = (target_node_id == self_pid_); + if (are_skeleton_and_proxy_in_same_process) + { + const auto result = + CallUnsubscribeServiceMethodLocally(skeleton_instance_identifier, proxy_instance_identifier); + if (!(result.has_value())) + { + return MakeUnexpected(ComErrc::kBindingFailure); + } + return {}; + } + else + { + const auto result = CallUnsubscribeServiceMethodRemotely( + skeleton_instance_identifier, proxy_instance_identifier, target_node_id); + if (!(result.has_value())) + { + return MakeUnexpected(ComErrc::kBindingFailure); + } + return {}; + } +} + Result MessagePassingServiceInstance::CallMethod( const ProxyMethodInstanceIdentifier& proxy_method_instance_identifier, std::size_t queue_position, diff --git a/score/mw/com/impl/bindings/lola/messaging/message_passing_service_instance.h b/score/mw/com/impl/bindings/lola/messaging/message_passing_service_instance.h index 209d67bb5..b9f7811cf 100644 --- a/score/mw/com/impl/bindings/lola/messaging/message_passing_service_instance.h +++ b/score/mw/com/impl/bindings/lola/messaging/message_passing_service_instance.h @@ -85,6 +85,10 @@ class MessagePassingServiceInstance : public IMessagePassingServiceInstance IMessagePassingService::ServiceMethodSubscribedHandler subscribed_callback, IMessagePassingService::AllowedConsumerUids allowed_proxy_uids) override; + ResultBlank RegisterOnServiceMethodUnsubscribedHandler( + const SkeletonInstanceIdentifier skeleton_instance_identifier, + IMessagePassingService::ServiceMethodUnsubscribedHandler unsubscribed_callback) override; + Result RegisterMethodCallHandler(const ProxyMethodInstanceIdentifier proxy_method_instance_identifier, IMessagePassingService::MethodCallHandler method_call_callback, const uid_t allowed_proxy_uid) override; @@ -92,6 +96,9 @@ class MessagePassingServiceInstance : public IMessagePassingServiceInstance void UnregisterOnServiceMethodSubscribedHandler( const SkeletonInstanceIdentifier skeleton_instance_identifier) override; + void UnregisterOnServiceMethodUnsubscribedHandler( + const SkeletonInstanceIdentifier skeleton_instance_identifier) override; + void UnregisterMethodCallHandler(const ProxyMethodInstanceIdentifier proxy_method_instance_identifier) override; void NotifyOutdatedNodeId(const pid_t outdated_node_id, const pid_t target_node_id) noexcept override; @@ -119,6 +126,10 @@ class MessagePassingServiceInstance : public IMessagePassingServiceInstance const ProxyInstanceIdentifier& proxy_instance_identifier, const pid_t target_node_id) override; + ResultBlank UnsubscribeServiceMethod(const SkeletonInstanceIdentifier& skeleton_instance_identifier, + const ProxyInstanceIdentifier& proxy_instance_identifier, + const pid_t target_node_id) override; + Result CallMethod(const ProxyMethodInstanceIdentifier& proxy_method_instance_identifier, const std::size_t queue_position, const pid_t target_node_id) override; @@ -136,7 +147,8 @@ class MessagePassingServiceInstance : public IMessagePassingServiceInstance enum class MessageWithReplyType : std::uint8_t { kSubscribeServiceMethod = 1U, - kCallMethod, + kCallMethod = 2U, + kUnsubscribeServiceMethod = 3U, }; struct RegisteredNotificationHandler @@ -197,6 +209,8 @@ class MessagePassingServiceInstance : public IMessagePassingServiceInstance score::Result HandleSubscribeServiceMethodMsg(const score::cpp::span payload, const uid_t sender_uid, const pid_t sender_node_id); + score::ResultBlank HandleUnsubscribeServiceMethodMsg(const score::cpp::span payload, + const pid_t sender_node_id); score::Result HandleCallMethodMsg(const score::cpp::span payload, const uid_t sender_uid); std::uint32_t NotifyEventLocally(const ElementFqId event_id) noexcept; @@ -212,6 +226,11 @@ class MessagePassingServiceInstance : public IMessagePassingServiceInstance const ProxyInstanceIdentifier& proxy_instance_identifier, const uid_t proxy_uid, const pid_t proxy_pid); + + score::ResultBlank CallUnsubscribeServiceMethodLocally( + const SkeletonInstanceIdentifier& skeleton_instance_identifier, + const ProxyInstanceIdentifier& proxy_instance_identifier); + score::Result CallServiceMethodLocally(const ProxyMethodInstanceIdentifier& proxy_method_instance_identifier, const std::size_t queue_position, const uid_t proxy_uid); @@ -219,6 +238,11 @@ class MessagePassingServiceInstance : public IMessagePassingServiceInstance Result CallSubscribeServiceMethodRemotely(const SkeletonInstanceIdentifier& skeleton_instance_identifier, const ProxyInstanceIdentifier& proxy_instance_identifier, const pid_t target_node_id); + + ResultBlank CallUnsubscribeServiceMethodRemotely(const SkeletonInstanceIdentifier& skeleton_instance_identifier, + const ProxyInstanceIdentifier& proxy_instance_identifier, + const pid_t target_node_id); + Result CallServiceMethodRemotely(const ProxyMethodInstanceIdentifier& proxy_method_instance_identifier, const std::size_t queue_position, const pid_t target_node_id); @@ -344,6 +368,13 @@ class MessagePassingServiceInstance : public IMessagePassingServiceInstance std::shared_mutex subscribe_service_method_handlers_mutex_; + using UnsubscribeServiceMethodMapType = + std::unordered_map; + + UnsubscribeServiceMethodMapType unsubscribe_service_method_handlers_; + + std::shared_mutex unsubscribe_service_method_handlers_mutex_; + CallMethodMapType call_method_handlers_; std::shared_mutex call_method_handlers_mutex_; diff --git a/score/mw/com/impl/bindings/lola/messaging/message_passing_service_instance_methods_test.cpp b/score/mw/com/impl/bindings/lola/messaging/message_passing_service_instance_methods_test.cpp index 700524604..008b59578 100644 --- a/score/mw/com/impl/bindings/lola/messaging/message_passing_service_instance_methods_test.cpp +++ b/score/mw/com/impl/bindings/lola/messaging/message_passing_service_instance_methods_test.cpp @@ -37,6 +37,7 @@ enum class MessageWithReplyType : std::uint8_t { kSubscribeServiceMethod = 1, kCallMethod, + kUnsubscribeServiceMethod, }; struct SubscribeServiceMethodUnserializedPayload @@ -45,6 +46,12 @@ struct SubscribeServiceMethodUnserializedPayload ProxyInstanceIdentifier proxy_instance_identifier; }; +struct UnsubscribeServiceMethodUnserializedPayload +{ + SkeletonInstanceIdentifier skeleton_instance_identifier; + ProxyInstanceIdentifier proxy_instance_identifier; +}; + struct MethodCallUnserializedPayload { ProxyMethodInstanceIdentifier proxy_method_instance_identifier; @@ -111,6 +118,7 @@ class MessagePassingServiceInstanceMethodsFixture : public ::testing::Test }); ON_CALL(mock_subscribe_method_handler_, Call(_, _, _)).WillByDefault(Return(score::Result{})); + ON_CALL(mock_unsubscribe_method_handler_, Call(_)).WillByDefault(Return(score::ResultBlank{})); } MessagePassingServiceInstanceMethodsFixture& GivenAMessagePassingServiceInstance( @@ -179,6 +187,18 @@ class MessagePassingServiceInstanceMethodsFixture : public ::testing::Test return *this; } + MessagePassingServiceInstanceMethodsFixture& WithARegisteredUnsubscribeMethodHandler( + const SkeletonInstanceIdentifier skeleton_instance_identifier) + { + SCORE_LANGUAGE_FUTURECPP_ASSERT_PRD(unit_ != nullptr); + IMessagePassingService::ServiceMethodUnsubscribedHandler scoped_unsubscribe_method_handler{ + unsubscribe_method_handler_scope_, mock_unsubscribe_method_handler_.AsStdFunction()}; + auto result = unit_->RegisterOnServiceMethodUnsubscribedHandler(skeleton_instance_identifier, + scoped_unsubscribe_method_handler); + EXPECT_TRUE(result.has_value()); + return *this; + } + template UnserializedPayload DeserializeMethodMessage(score::cpp::span message, MessageWithReplyType message_type) @@ -235,6 +255,13 @@ class MessagePassingServiceInstanceMethodsFixture : public ::testing::Test return CreateSerializedMethodMessage(payload, MessageWithReplyType::kSubscribeServiceMethod); } + score::cpp::span CreateValidUnsubscribeMethodMessage() + { + const UnsubscribeServiceMethodUnserializedPayload payload{kSkeletonInstanceIdentifier, + kProxyInstanceIdentifier}; + return CreateSerializedMethodMessage(payload, MessageWithReplyType::kUnsubscribeServiceMethod); + } + NiceMock client_factory_mock_{}; NiceMock server_factory_mock_{}; @@ -257,10 +284,12 @@ class MessagePassingServiceInstanceMethodsFixture : public ::testing::Test safecpp::Scope<> method_call_handler_scope_{}; safecpp::Scope<> subscribe_method_handler_scope_{}; + safecpp::Scope<> unsubscribe_method_handler_scope_{}; ::testing::MockFunction mock_method_call_handler_{}; ::testing::MockFunction(ProxyInstanceIdentifier, uid_t, pid_t)> mock_subscribe_method_handler_{}; + ::testing::MockFunction mock_unsubscribe_method_handler_{}; // Since an SendWaitReply returns an score::cpp::span to a message (which is essentially a pointer to a message), we // need a buffer to store the message. @@ -1330,5 +1359,272 @@ TEST_F(MessagePassingServiceInstanceUnregisterSubscribeMethodHandlerTest, unit_->UnregisterOnServiceMethodSubscribedHandler(kSkeletonInstanceIdentifier)); } +using MessagePassingServiceInstanceRegisterUnsubscribeHandlerTest = MessagePassingServiceInstanceMethodsFixture; +TEST_F(MessagePassingServiceInstanceRegisterUnsubscribeHandlerTest, ReregisteringHandlerReturnsError) +{ + ::testing::MockFunction mock_unsubscribe_method_handler_2{}; + safecpp::Scope<> unsubscribe_method_handler_scope_2{}; + IMessagePassingService::ServiceMethodUnsubscribedHandler scoped_unsubscribe_method_handler_2{ + unsubscribe_method_handler_scope_2, mock_unsubscribe_method_handler_2.AsStdFunction()}; + + GivenAMessagePassingServiceInstance().WithAClientInTheSameProcess().WithARegisteredUnsubscribeMethodHandler( + kSkeletonInstanceIdentifier); + + // When registering a new on method unsubscribed handler for the same skeleton instance identifier + auto result = unit_->RegisterOnServiceMethodUnsubscribedHandler(kSkeletonInstanceIdentifier, + scoped_unsubscribe_method_handler_2); + + // Then an error is returned + ASSERT_FALSE(result.has_value()); + EXPECT_EQ(result.error(), ComErrc::kBindingFailure); +} + +using MessagePassingServiceInstanceLocalUnsubscribeMethodTest = MessagePassingServiceInstanceMethodsFixture; +TEST_F(MessagePassingServiceInstanceLocalUnsubscribeMethodTest, CallingWithSelfPidCallsUnsubscribeHandlerLocally) +{ + GivenAMessagePassingServiceInstance().WithAClientInTheSameProcess().WithARegisteredUnsubscribeMethodHandler( + kSkeletonInstanceIdentifier); + + // Expecting that the registered unsubscribe handler will be called with the provided ProxyInstanceIdentifier + EXPECT_CALL(mock_unsubscribe_method_handler_, Call(kProxyInstanceIdentifier)); + + // and expecting that an UnsubscribeServiceMethod message will NOT be sent + EXPECT_CALL(client_connection_mock_, SendWaitReply(_, _)).Times(0); + + // When calling UnsubscribeServiceMethod with target_node_id equal to the PID of the current process + const auto call_result = + unit_->UnsubscribeServiceMethod(kSkeletonInstanceIdentifier, kProxyInstanceIdentifier, kLocalPid); + + // Then the result is valid + ASSERT_TRUE(call_result.has_value()); +} + +TEST_F(MessagePassingServiceInstanceLocalUnsubscribeMethodTest, + CallingWithSkeletonIdentifierThatWasNeverRegisteredReturnsSuccess) +{ + GivenAMessagePassingServiceInstance().WithAClientInTheSameProcess().WithARegisteredUnsubscribeMethodHandler( + kSkeletonInstanceIdentifier2); + + // When calling UnsubscribeServiceMethod with a SkeletonInstanceIdentifier for which no unsubscribe handler has been + // registered (best-effort: returns success instead of error) + const auto call_result = + unit_->UnsubscribeServiceMethod(kSkeletonInstanceIdentifier, kProxyInstanceIdentifier, kLocalPid); + + // Then the result is valid (best-effort: not-found is treated as success) + ASSERT_TRUE(call_result.has_value()); +} + +TEST_F(MessagePassingServiceInstanceLocalUnsubscribeMethodTest, + CallingAfterUnsubscribeHandlerScopeHasExpiredReturnsSuccess) +{ + GivenAMessagePassingServiceInstance().WithAClientInTheSameProcess().WithARegisteredUnsubscribeMethodHandler( + kSkeletonInstanceIdentifier); + + // and given that the unsubscribe method handler scope has expired + unsubscribe_method_handler_scope_.Expire(); + + // When calling UnsubscribeServiceMethod with target_node_id equal to the PID of the current process + const auto call_result = + unit_->UnsubscribeServiceMethod(kSkeletonInstanceIdentifier, kProxyInstanceIdentifier, kLocalPid); + + // Then the result is valid (best-effort: scope-expired is treated as success) + ASSERT_TRUE(call_result.has_value()); +} + +TEST_F(MessagePassingServiceInstanceLocalUnsubscribeMethodTest, + CallingAfterUnsubscribeHandlerScopeHasExpiredDoesNotCallHandler) +{ + GivenAMessagePassingServiceInstance().WithAClientInTheSameProcess().WithARegisteredUnsubscribeMethodHandler( + kSkeletonInstanceIdentifier); + + // and given that the unsubscribe method handler scope has expired + unsubscribe_method_handler_scope_.Expire(); + + // Expecting that the registered unsubscribe method handler will not be called + EXPECT_CALL(mock_unsubscribe_method_handler_, Call(_)).Times(0); + + // When calling UnsubscribeServiceMethod + score::cpp::ignore = + unit_->UnsubscribeServiceMethod(kSkeletonInstanceIdentifier, kProxyInstanceIdentifier, kLocalPid); +} + +using MessagePassingServiceInstanceRemoteUnsubscribeMethodTest = MessagePassingServiceInstanceMethodsFixture; +TEST_F(MessagePassingServiceInstanceRemoteUnsubscribeMethodTest, CallingWithOtherProcessPidSendsUnsubscribeMessage) +{ + GivenAMessagePassingServiceInstance().WithAClientInDifferentProcess().WithARegisteredUnsubscribeMethodHandler( + kSkeletonInstanceIdentifier); + + // Expecting that an UnsubscribeServiceMethod message will be sent containing the provided ProxyInstanceIdentifier + // and SkeletonInstanceIdentifier + EXPECT_CALL(client_connection_mock_, SendWaitReply(_, _)).WillOnce(WithArg<0>(Invoke([this](auto message) { + const auto actual_payload = DeserializeMethodMessage( + message, MessageWithReplyType::kUnsubscribeServiceMethod); + EXPECT_EQ(actual_payload.skeleton_instance_identifier, kSkeletonInstanceIdentifier); + EXPECT_EQ(actual_payload.proxy_instance_identifier, kProxyInstanceIdentifier); + + return CreateSerializedMethodReply(score::ResultBlank{}, method_reply_buffer_); + }))); + + // and expecting that the registered unsubscribe method handler will NOT be called (which would happen when the + // client is in the same process) + EXPECT_CALL(mock_unsubscribe_method_handler_, Call(_)).Times(0); + + // When calling UnsubscribeServiceMethod with target_node_id equal to the PID of a different process + const auto call_result = + unit_->UnsubscribeServiceMethod(kSkeletonInstanceIdentifier, kProxyInstanceIdentifier, kRemotePid); + + // Then the result is valid + ASSERT_TRUE(call_result.has_value()); +} + +TEST_F(MessagePassingServiceInstanceRemoteUnsubscribeMethodTest, ReturnsErrorWhenSendWaitReplyReturnsError) +{ + GivenAMessagePassingServiceInstance().WithAClientInDifferentProcess().WithARegisteredUnsubscribeMethodHandler( + kSkeletonInstanceIdentifier); + + // Expecting that SendWaitReply will be called which returns an error + EXPECT_CALL(client_connection_mock_, SendWaitReply(_, _)) + .WillOnce(Return(score::cpp::make_unexpected(score::os::Error::createFromErrno()))); + + // When calling UnsubscribeServiceMethod with target_node_id equal to the PID of a different process + const auto call_result = + unit_->UnsubscribeServiceMethod(kSkeletonInstanceIdentifier, kProxyInstanceIdentifier, kRemotePid); + + // Then an error is returned + ASSERT_FALSE(call_result.has_value()); + EXPECT_EQ(call_result.error(), ComErrc::kBindingFailure); +} + +TEST_F(MessagePassingServiceInstanceRemoteUnsubscribeMethodTest, ReturnsErrorWhenReplyPayloadHasUnexpectedSize) +{ + GivenAMessagePassingServiceInstance().WithAClientInDifferentProcess().WithARegisteredUnsubscribeMethodHandler( + kSkeletonInstanceIdentifier); + + // Expecting that SendWaitReply will be called which returns a payload with an unexpected size + std::vector payload_with_unexpected_size(sizeof(MethodReplyPayload) + 2U); + EXPECT_CALL(client_connection_mock_, SendWaitReply(_, _)) + .WillOnce(Return( + score::cpp::span{payload_with_unexpected_size.data(), payload_with_unexpected_size.size()})); + + // When calling UnsubscribeServiceMethod with target_node_id equal to the PID of a different process + const auto call_result = + unit_->UnsubscribeServiceMethod(kSkeletonInstanceIdentifier, kProxyInstanceIdentifier, kRemotePid); + + // Then an error is returned + ASSERT_FALSE(call_result.has_value()); + EXPECT_EQ(call_result.error(), ComErrc::kBindingFailure); +} + +using MessagePassingServiceInstanceHandleUnsubscribeMethodMessageTest = MessagePassingServiceInstanceMethodsFixture; +TEST_F(MessagePassingServiceInstanceHandleUnsubscribeMethodMessageTest, ReturnsErrorWhenPayloadHasUnexpectedSize) +{ + GivenAMessagePassingServiceInstance().WithAClientInDifferentProcess().WithARegisteredUnsubscribeMethodHandler( + kSkeletonInstanceIdentifier); + + // When a MessageWithReply message is received of type kUnsubscribeServiceMethod with the wrong payload size + std::vector payload_with_unexpected_size(sizeof(UnsubscribeServiceMethodUnserializedPayload) + 2U); + payload_with_unexpected_size[0] = static_cast(MessageWithReplyType::kUnsubscribeServiceMethod); + const auto result = received_send_message_with_reply_callback_( + server_connection_mock_, + score::cpp::span{payload_with_unexpected_size.data(), payload_with_unexpected_size.size()}); + + // Then an error is returned since the error is unrecoverable + ASSERT_FALSE(result.has_value()); + EXPECT_EQ(result.error(), os::Error::Code::kUnexpected); +} + +TEST_F(MessagePassingServiceInstanceHandleUnsubscribeMethodMessageTest, RepliesWithErrorWhenPayloadHasUnexpectedSize) +{ + GivenAMessagePassingServiceInstance().WithAClientInDifferentProcess().WithARegisteredUnsubscribeMethodHandler( + kSkeletonInstanceIdentifier); + + // Expecting that a reply will be sent containing an unexpected message size error + EXPECT_CALL(server_connection_mock_, Reply(_)) + .WillOnce(Invoke([this](auto reply_buffer) -> score::cpp::expected_blank { + const auto reply_result = DeserializeMethodReplyMessage(reply_buffer); + EXPECT_THAT(reply_result, ContainsError(MethodErrc::kUnexpectedMessageSize)); + return {}; + })); + + // When a MessageWithReply message is received of type kUnsubscribeServiceMethod with the wrong payload size + std::vector payload_with_unexpected_size(sizeof(UnsubscribeServiceMethodUnserializedPayload) + 2U); + payload_with_unexpected_size[0] = static_cast(MessageWithReplyType::kUnsubscribeServiceMethod); + score::cpp::ignore = received_send_message_with_reply_callback_( + server_connection_mock_, + score::cpp::span{payload_with_unexpected_size.data(), payload_with_unexpected_size.size()}); +} + +TEST_F(MessagePassingServiceInstanceHandleUnsubscribeMethodMessageTest, + ReturnsSuccessWhenUnsubscribeHandlerNotRegistered) +{ + // Given that no unsubscribe handler has been registered + GivenAMessagePassingServiceInstance().WithAClientInDifferentProcess(); + + // Expecting that no handlers are called + EXPECT_CALL(mock_unsubscribe_method_handler_, Call(_)).Times(0); + + // When a valid MessageWithReply message is received of type kUnsubscribeServiceMethod + const auto result = + received_send_message_with_reply_callback_(server_connection_mock_, CreateValidUnsubscribeMethodMessage()); + + // Then a valid result is returned (best-effort: not-found returns success) + ASSERT_TRUE(result.has_value()); +} + +TEST_F(MessagePassingServiceInstanceHandleUnsubscribeMethodMessageTest, + RepliesWithSuccessWhenUnsubscribeHandlerNotRegistered) +{ + // Given that no unsubscribe handler has been registered + GivenAMessagePassingServiceInstance().WithAClientInDifferentProcess(); + + // Expecting that a reply will be sent containing success (best-effort: not-found returns success) + EXPECT_CALL(server_connection_mock_, Reply(_)) + .WillOnce(Invoke([this](auto reply_buffer) -> score::cpp::expected_blank { + const auto reply_result = DeserializeMethodReplyMessage(reply_buffer); + EXPECT_TRUE(reply_result.has_value()); + return {}; + })); + + // When a valid MessageWithReply message is received of type kUnsubscribeServiceMethod + score::cpp::ignore = + received_send_message_with_reply_callback_(server_connection_mock_, CreateValidUnsubscribeMethodMessage()); +} + +TEST_F(MessagePassingServiceInstanceHandleUnsubscribeMethodMessageTest, + CallsUnsubscribeMethodHandlerRegisteredWithProvidedSkeletonIdentifier) +{ + GivenAMessagePassingServiceInstance().WithAClientInDifferentProcess().WithARegisteredUnsubscribeMethodHandler( + kSkeletonInstanceIdentifier); + + // Expecting that the registered unsubscribe method handler will be called with the provided ProxyInstanceIdentifier + EXPECT_CALL(mock_unsubscribe_method_handler_, Call(kProxyInstanceIdentifier)); + + // When a MessageWithReply message is received of type kUnsubscribeServiceMethod + const auto result = + received_send_message_with_reply_callback_(server_connection_mock_, CreateValidUnsubscribeMethodMessage()); + + // Then a valid result is returned + ASSERT_TRUE(result.has_value()); +} + +TEST_F(MessagePassingServiceInstanceHandleUnsubscribeMethodMessageTest, + RepliesSuccessWhenUnsubscribeMethodHandlerCalledSuccessfully) +{ + GivenAMessagePassingServiceInstance().WithAClientInDifferentProcess().WithARegisteredUnsubscribeMethodHandler( + kSkeletonInstanceIdentifier); + + // Expecting that a reply will be sent containing success + EXPECT_CALL(server_connection_mock_, Reply(_)) + .WillOnce(Invoke([this](auto reply_buffer) -> score::cpp::expected_blank { + const auto reply_result = DeserializeMethodReplyMessage(reply_buffer); + EXPECT_TRUE(reply_result.has_value()); + return {}; + })); + + // When a MessageWithReply message is received of type kUnsubscribeServiceMethod + score::cpp::ignore = + received_send_message_with_reply_callback_(server_connection_mock_, CreateValidUnsubscribeMethodMessage()); +} + } // namespace } // namespace score::mw::com::impl::lola diff --git a/score/mw/com/impl/bindings/lola/messaging/message_passing_service_instance_mock.h b/score/mw/com/impl/bindings/lola/messaging/message_passing_service_instance_mock.h index d1131ddeb..d083b4c95 100644 --- a/score/mw/com/impl/bindings/lola/messaging/message_passing_service_instance_mock.h +++ b/score/mw/com/impl/bindings/lola/messaging/message_passing_service_instance_mock.h @@ -44,6 +44,11 @@ class MessagePassingServiceInstanceMock : public IMessagePassingServiceInstance IMessagePassingService::AllowedConsumerUids), (override)); + MOCK_METHOD(ResultBlank, + RegisterOnServiceMethodUnsubscribedHandler, + (SkeletonInstanceIdentifier, IMessagePassingService::ServiceMethodUnsubscribedHandler), + (override)); + MOCK_METHOD(Result, RegisterMethodCallHandler, (ProxyMethodInstanceIdentifier, IMessagePassingService::MethodCallHandler, uid_t), @@ -63,10 +68,17 @@ class MessagePassingServiceInstanceMock : public IMessagePassingServiceInstance (const SkeletonInstanceIdentifier&, const ProxyInstanceIdentifier&, pid_t), (override)); + MOCK_METHOD(ResultBlank, + UnsubscribeServiceMethod, + (const SkeletonInstanceIdentifier&, const ProxyInstanceIdentifier&, pid_t), + (override)); + MOCK_METHOD(Result, CallMethod, (const ProxyMethodInstanceIdentifier&, std::size_t, pid_t), (override)); MOCK_METHOD(void, UnregisterOnServiceMethodSubscribedHandler, (SkeletonInstanceIdentifier), (override)); + MOCK_METHOD(void, UnregisterOnServiceMethodUnsubscribedHandler, (SkeletonInstanceIdentifier), (override)); + MOCK_METHOD(void, UnregisterMethodCallHandler, (ProxyMethodInstanceIdentifier), (override)); }; diff --git a/score/mw/com/impl/bindings/lola/messaging/message_passing_service_mock.h b/score/mw/com/impl/bindings/lola/messaging/message_passing_service_mock.h index 14a700ceb..db47be328 100644 --- a/score/mw/com/impl/bindings/lola/messaging/message_passing_service_mock.h +++ b/score/mw/com/impl/bindings/lola/messaging/message_passing_service_mock.h @@ -15,6 +15,7 @@ #include "score/mw/com/impl/bindings/lola/element_fq_id.h" #include "score/mw/com/impl/bindings/lola/messaging/i_message_passing_service.h" +#include "score/mw/com/impl/bindings/lola/messaging/method_unsubscription_registration_guard.h" #include "score/mw/com/impl/bindings/lola/proxy_instance_identifier.h" #include "score/mw/com/impl/configuration/quality_type.h" @@ -52,6 +53,10 @@ class MessagePassingServiceMock : public IMessagePassingService RegisterOnServiceMethodSubscribedHandler, (QualityType, SkeletonInstanceIdentifier, ServiceMethodSubscribedHandler, AllowedConsumerUids), (override)); + MOCK_METHOD(Result, + RegisterOnServiceMethodUnsubscribedHandler, + (QualityType, SkeletonInstanceIdentifier, ServiceMethodUnsubscribedHandler), + (override)); MOCK_METHOD(Result, RegisterMethodCallHandler, (QualityType, ProxyMethodInstanceIdentifier, MethodCallHandler, uid_t), @@ -60,6 +65,10 @@ class MessagePassingServiceMock : public IMessagePassingService SubscribeServiceMethod, (QualityType, const SkeletonInstanceIdentifier&, const ProxyInstanceIdentifier&, pid_t), (override)); + MOCK_METHOD(ResultBlank, + UnsubscribeServiceMethod, + (QualityType, const SkeletonInstanceIdentifier&, const ProxyInstanceIdentifier&, pid_t), + (override)); MOCK_METHOD(Result, CallMethod, (QualityType, const ProxyMethodInstanceIdentifier&, std::size_t, pid_t), @@ -69,6 +78,10 @@ class MessagePassingServiceMock : public IMessagePassingService UnregisterOnServiceMethodSubscribedHandler, (const QualityType asil_level, SkeletonInstanceIdentifier skeleton_instance_identifier), (override)); + MOCK_METHOD(void, + UnregisterOnServiceMethodUnsubscribedHandler, + (const QualityType asil_level, SkeletonInstanceIdentifier skeleton_instance_identifier), + (override)); MOCK_METHOD(void, UnregisterMethodCallHandler, (const QualityType asil_level, ProxyMethodInstanceIdentifier proxy_method_instance_identifier), diff --git a/score/mw/com/impl/bindings/lola/messaging/message_passing_service_test.cpp b/score/mw/com/impl/bindings/lola/messaging/message_passing_service_test.cpp index 080b77af9..594573d8c 100644 --- a/score/mw/com/impl/bindings/lola/messaging/message_passing_service_test.cpp +++ b/score/mw/com/impl/bindings/lola/messaging/message_passing_service_test.cpp @@ -14,6 +14,7 @@ #include "score/mw/com/impl/bindings/lola/messaging/message_passing_service_instance_factory_mock.h" #include "score/mw/com/impl/bindings/lola/messaging/message_passing_service_instance_mock.h" +#include "score/mw/com/impl/com_error.h" #include #include @@ -500,4 +501,140 @@ TEST_F(MessagePassingServiceQMDelegationTest, CallMethodReturnsAValue) EXPECT_TRUE(result.has_value()); } +TEST_F(MessagePassingServiceQMDelegationTest, RegisterOnServiceMethodUnsubscribedHandlerDispatchesToQmInstance) +{ + // Given some input parameters to the tested function call + IMessagePassingService::ServiceMethodUnsubscribedHandler callback; + + // Expecting a call to RegisterOnServiceMethodUnsubscribedHandler of ASIL-QM mock instance and no call to ASIL-B + EXPECT_CALL(*asil_qm_message_passing_service_instance_mock_, + RegisterOnServiceMethodUnsubscribedHandler(kSkeletonInstanceId, _)) + .WillOnce(Return(score::ResultBlank{})); + EXPECT_CALL(*asil_b_message_passing_service_instance_mock_, RegisterOnServiceMethodUnsubscribedHandler(_, _)) + .Times(0); + + // When calling RegisterOnServiceMethodUnsubscribedHandler with QualityType::kASIL_QM + const auto result = GivenAMessagePassingServiceWithAsilBAndQm().RegisterOnServiceMethodUnsubscribedHandler( + QualityType::kASIL_QM, kSkeletonInstanceId, std::move(callback)); + + // Then the result should have a value + EXPECT_TRUE(result.has_value()); +} + +TEST_F(MessagePassingServiceQMDelegationTest, RegisterOnServiceMethodUnsubscribedHandlerDispatchesToAsilBInstance) +{ + // Given some input parameters to the tested function call + IMessagePassingService::ServiceMethodUnsubscribedHandler callback; + + // Expecting a call to RegisterOnServiceMethodUnsubscribedHandler of ASIL-B mock instance and no call to QM + EXPECT_CALL(*asil_b_message_passing_service_instance_mock_, + RegisterOnServiceMethodUnsubscribedHandler(kSkeletonInstanceId, _)) + .WillOnce(Return(score::ResultBlank{})); + EXPECT_CALL(*asil_qm_message_passing_service_instance_mock_, RegisterOnServiceMethodUnsubscribedHandler(_, _)) + .Times(0); + + // When calling RegisterOnServiceMethodUnsubscribedHandler with QualityType::kASIL_B + const auto result = GivenAMessagePassingServiceWithAsilBAndQm().RegisterOnServiceMethodUnsubscribedHandler( + QualityType::kASIL_B, kSkeletonInstanceId, std::move(callback)); + + // Then the result should have a value + EXPECT_TRUE(result.has_value()); +} + +TEST_F(MessagePassingServiceQMDelegationTest, + RegisterOnServiceMethodUnsubscribedHandlerReturnsErrorWhenInstanceRegistrationFails) +{ + // Given some input parameters to the tested function call + IMessagePassingService::ServiceMethodUnsubscribedHandler callback; + + // Expecting a call to RegisterOnServiceMethodUnsubscribedHandler of ASIL-QM mock instance which returns an error + EXPECT_CALL(*asil_qm_message_passing_service_instance_mock_, + RegisterOnServiceMethodUnsubscribedHandler(kSkeletonInstanceId, _)) + .WillOnce(Return(MakeUnexpected(ComErrc::kBindingFailure))); + + // When calling RegisterOnServiceMethodUnsubscribedHandler + const auto result = GivenAMessagePassingServiceWithAsilBAndQm().RegisterOnServiceMethodUnsubscribedHandler( + QualityType::kASIL_QM, kSkeletonInstanceId, std::move(callback)); + + // Then the result should be an error + ASSERT_FALSE(result.has_value()); + EXPECT_EQ(result.error(), ComErrc::kBindingFailure); +} + +TEST_F(MessagePassingServiceQMDelegationTest, UnsubscribeServiceMethodDispatchesToQmInstance) +{ + // Expecting a call to UnsubscribeServiceMethod of ASIL-QM mock instance and no call to ASIL-B + EXPECT_CALL(*asil_qm_message_passing_service_instance_mock_, + UnsubscribeServiceMethod(kSkeletonInstanceId, kProxyInstanceId, kTargetNodeId)) + .WillOnce(Return(score::ResultBlank{})); + EXPECT_CALL(*asil_b_message_passing_service_instance_mock_, UnsubscribeServiceMethod(_, _, _)).Times(0); + + // When calling UnsubscribeServiceMethod with QualityType::kASIL_QM + const auto result = GivenAMessagePassingServiceWithAsilBAndQm().UnsubscribeServiceMethod( + QualityType::kASIL_QM, kSkeletonInstanceId, kProxyInstanceId, kTargetNodeId); + + // Then the result should have a value + EXPECT_TRUE(result.has_value()); +} + +TEST_F(MessagePassingServiceQMDelegationTest, UnsubscribeServiceMethodDispatchesToAsilBInstance) +{ + // Expecting a call to UnsubscribeServiceMethod of ASIL-B mock instance and no call to QM + EXPECT_CALL(*asil_b_message_passing_service_instance_mock_, + UnsubscribeServiceMethod(kSkeletonInstanceId, kProxyInstanceId, kTargetNodeId)) + .WillOnce(Return(score::ResultBlank{})); + EXPECT_CALL(*asil_qm_message_passing_service_instance_mock_, UnsubscribeServiceMethod(_, _, _)).Times(0); + + // When calling UnsubscribeServiceMethod with QualityType::kASIL_B + const auto result = GivenAMessagePassingServiceWithAsilBAndQm().UnsubscribeServiceMethod( + QualityType::kASIL_B, kSkeletonInstanceId, kProxyInstanceId, kTargetNodeId); + + // Then the result should have a value + EXPECT_TRUE(result.has_value()); +} + +TEST_F(MessagePassingServiceQMDelegationTest, UnregisterOnServiceMethodUnsubscribedHandlerDispatchesToQmInstance) +{ + // Expecting a call to RegisterOnServiceMethodUnsubscribedHandler of ASIL-QM mock instance (to set it up) + EXPECT_CALL(*asil_qm_message_passing_service_instance_mock_, + RegisterOnServiceMethodUnsubscribedHandler(kSkeletonInstanceId, _)) + .WillOnce(Return(score::ResultBlank{})); + + // and expecting that UnregisterOnServiceMethodUnsubscribedHandler is called on ASIL-QM (not ASIL-B) when the guard + // is destroyed + EXPECT_CALL(*asil_qm_message_passing_service_instance_mock_, + UnregisterOnServiceMethodUnsubscribedHandler(kSkeletonInstanceId)); + EXPECT_CALL(*asil_b_message_passing_service_instance_mock_, UnregisterOnServiceMethodUnsubscribedHandler(_)) + .Times(0); + + // When RegisterOnServiceMethodUnsubscribedHandler is called and the returned guard is destroyed + { + const auto guard = GivenAMessagePassingServiceWithAsilBAndQm().RegisterOnServiceMethodUnsubscribedHandler( + QualityType::kASIL_QM, kSkeletonInstanceId, {}); + ASSERT_TRUE(guard.has_value()); + } // guard destroyed here — triggers unregister +} + +TEST_F(MessagePassingServiceQMDelegationTest, UnregisterOnServiceMethodUnsubscribedHandlerDispatchesToAsilBInstance) +{ + // Expecting a call to RegisterOnServiceMethodUnsubscribedHandler of ASIL-B mock instance (to set it up) + EXPECT_CALL(*asil_b_message_passing_service_instance_mock_, + RegisterOnServiceMethodUnsubscribedHandler(kSkeletonInstanceId, _)) + .WillOnce(Return(score::ResultBlank{})); + + // and expecting that UnregisterOnServiceMethodUnsubscribedHandler is called on ASIL-B (not QM) when the guard + // is destroyed + EXPECT_CALL(*asil_b_message_passing_service_instance_mock_, + UnregisterOnServiceMethodUnsubscribedHandler(kSkeletonInstanceId)); + EXPECT_CALL(*asil_qm_message_passing_service_instance_mock_, UnregisterOnServiceMethodUnsubscribedHandler(_)) + .Times(0); + + // When RegisterOnServiceMethodUnsubscribedHandler is called and the returned guard is destroyed + { + const auto guard = GivenAMessagePassingServiceWithAsilBAndQm().RegisterOnServiceMethodUnsubscribedHandler( + QualityType::kASIL_B, kSkeletonInstanceId, {}); + ASSERT_TRUE(guard.has_value()); + } // guard destroyed here — triggers unregister +} + } // namespace score::mw::com::impl::lola::test diff --git a/score/mw/com/impl/bindings/lola/messaging/method_unsubscription_registration_guard.cpp b/score/mw/com/impl/bindings/lola/messaging/method_unsubscription_registration_guard.cpp new file mode 100644 index 000000000..1154b13a3 --- /dev/null +++ b/score/mw/com/impl/bindings/lola/messaging/method_unsubscription_registration_guard.cpp @@ -0,0 +1,35 @@ +/******************************************************************************** + * Copyright (c) 2025 Contributors to the Eclipse Foundation + * + * See the NOTICE file(s) distributed with this work for additional + * information regarding copyright ownership. + * + * This program and the accompanying materials are made available under the + * terms of the Apache License Version 2.0 which is available at + * https://www.apache.org/licenses/LICENSE-2.0 + * + * SPDX-License-Identifier: Apache-2.0 + ********************************************************************************/ +#include "score/mw/com/impl/bindings/lola/messaging/method_unsubscription_registration_guard.h" + +#include "score/mw/com/impl/bindings/lola/messaging/i_message_passing_service.h" + +#include "score/language/safecpp/scoped_function/move_only_scoped_function.h" + +namespace score::mw::com::impl::lola +{ + +MethodUnsubscriptionRegistrationGuard MethodUnsubscriptionRegistrationGuardFactory::Create( + IMessagePassingService& message_passing_service, + const QualityType asil_level, + const SkeletonInstanceIdentifier skeleton_instance_identifier, + const safecpp::Scope<>& message_passing_service_instance_scope) +{ + return MethodUnsubscriptionRegistrationGuard(safecpp::MoveOnlyScopedFunction{ + message_passing_service_instance_scope, [&message_passing_service, asil_level, skeleton_instance_identifier]() { + message_passing_service.UnregisterOnServiceMethodUnsubscribedHandler(asil_level, + skeleton_instance_identifier); + }}); +} + +} // namespace score::mw::com::impl::lola diff --git a/score/mw/com/impl/bindings/lola/messaging/method_unsubscription_registration_guard.h b/score/mw/com/impl/bindings/lola/messaging/method_unsubscription_registration_guard.h new file mode 100644 index 000000000..3a267e8b1 --- /dev/null +++ b/score/mw/com/impl/bindings/lola/messaging/method_unsubscription_registration_guard.h @@ -0,0 +1,45 @@ +/******************************************************************************** + * Copyright (c) 2025 Contributors to the Eclipse Foundation + * + * See the NOTICE file(s) distributed with this work for additional + * information regarding copyright ownership. + * + * This program and the accompanying materials are made available under the + * terms of the Apache License Version 2.0 which is available at + * https://www.apache.org/licenses/LICENSE-2.0 + * + * SPDX-License-Identifier: Apache-2.0 + ********************************************************************************/ +#ifndef SCORE_MW_COM_IMPL_BINDINGS_LOLA_MESSAGING_METHOD_UNSUBSCRIPTION_REGISTRATION_GUARD_H +#define SCORE_MW_COM_IMPL_BINDINGS_LOLA_MESSAGING_METHOD_UNSUBSCRIPTION_REGISTRATION_GUARD_H + +#include "score/mw/com/impl/bindings/lola/skeleton_instance_identifier.h" +#include "score/mw/com/impl/configuration/quality_type.h" + +#include "score/language/safecpp/scoped_function/move_only_scoped_function.h" +#include "score/language/safecpp/scoped_function/scope.h" +#include "score/scope_exit/scope_exit.h" + +namespace score::mw::com::impl::lola +{ + +class IMessagePassingService; + +using MethodUnsubscriptionRegistrationGuard = utils::ScopeExit>; + +/// \brief RAII class which will call UnregisterOnServiceMethodUnsubscribedHandler on destruction. +/// +/// Will be returned by MessagePassingService::RegisterOnServiceMethodUnsubscribedHandler() to allow a user to +/// unregister the registered handler. +class MethodUnsubscriptionRegistrationGuardFactory +{ + public: + static MethodUnsubscriptionRegistrationGuard Create(IMessagePassingService& message_passing_service, + const QualityType asil_level, + const SkeletonInstanceIdentifier skeleton_instance_identifier, + const safecpp::Scope<>& message_passing_service_instance_scope); +}; + +} // namespace score::mw::com::impl::lola + +#endif // SCORE_MW_COM_IMPL_BINDINGS_LOLA_MESSAGING_METHOD_UNSUBSCRIPTION_REGISTRATION_GUARD_H diff --git a/score/mw/com/impl/bindings/lola/messaging/method_unsubscription_registration_guard_test.cpp b/score/mw/com/impl/bindings/lola/messaging/method_unsubscription_registration_guard_test.cpp new file mode 100644 index 000000000..3bbdb852a --- /dev/null +++ b/score/mw/com/impl/bindings/lola/messaging/method_unsubscription_registration_guard_test.cpp @@ -0,0 +1,116 @@ +/******************************************************************************** + * Copyright (c) 2025 Contributors to the Eclipse Foundation + * + * See the NOTICE file(s) distributed with this work for additional + * information regarding copyright ownership. + * + * This program and the accompanying materials are made available under the + * terms of the Apache License Version 2.0 which is available at + * https://www.apache.org/licenses/LICENSE-2.0 + * + * SPDX-License-Identifier: Apache-2.0 + ********************************************************************************/ + +#include "score/mw/com/impl/bindings/lola/messaging/method_unsubscription_registration_guard.h" + +#include "score/mw/com/impl/bindings/lola/messaging/message_passing_service_mock.h" +#include "score/mw/com/impl/bindings/lola/skeleton_instance_identifier.h" +#include "score/mw/com/impl/configuration/quality_type.h" + +#include + +namespace score::mw::com::impl::lola::detail +{ +namespace +{ + +using namespace ::testing; + +const QualityType kAsilLevel{QualityType::kASIL_B}; +const SkeletonInstanceIdentifier kSkeletonInstanceIdentifier{LolaServiceId{12U}, + LolaServiceInstanceId::InstanceId{22U}}; + +class MethodUnsubscriptionRegistrationGuardFixture : public ::testing::Test +{ + public: + MethodUnsubscriptionRegistrationGuardFixture() + { + ON_CALL(message_passing_service_mock_, UnregisterOnServiceMethodUnsubscribedHandler(_, _)) + .WillByDefault(WithoutArgs(Invoke([this]() -> void { + unregister_on_service_method_unsubscribed_handler_called_ = true; + }))); + } + + MethodUnsubscriptionRegistrationGuardFixture& GivenAMethodUnsubscriptionRegistrationGuard() + { + score::cpp::ignore = + method_unsubscription_registration_guard_.emplace(MethodUnsubscriptionRegistrationGuardFactory::Create( + message_passing_service_mock_, kAsilLevel, kSkeletonInstanceIdentifier, scope_)); + return *this; + } + + MessagePassingServiceMock message_passing_service_mock_{}; + std::optional method_unsubscription_registration_guard_{}; + bool unregister_on_service_method_unsubscribed_handler_called_{false}; + + safecpp::Scope<> scope_{}; +}; + +TEST_F(MethodUnsubscriptionRegistrationGuardFixture, MethodUnsubscriptionRegistrationGuardUsesScopeExit) +{ + // Expecting that MethodUnsubscriptionRegistrationGuard is a type alias for utils::ScopeExit. If this is + // the case, then we only add basic tests here that UnregisterOnServiceMethodUnsubscribedHandler is called on + // destruction of the guard and that the scope of the guard is correctly handled. The more complex tests about + // testing whether the handler is called when move constructing / move assigning the guard is handled in the tests + // for ScopeExit. + static_assert(std::is_same_v>>); +} + +TEST_F(MethodUnsubscriptionRegistrationGuardFixture, CreatingGuardDoesNotCallUnregister) +{ + // When creating a MethodUnsubscriptionRegistrationGuard + GivenAMethodUnsubscriptionRegistrationGuard(); + + // Expecting that UnregisterOnServiceMethodUnsubscribedHandler is never called + EXPECT_CALL(message_passing_service_mock_, UnregisterOnServiceMethodUnsubscribedHandler(_, _)).Times(0); + + // Then UnregisterOnServiceMethodUnsubscribedHandler is not called + EXPECT_FALSE(unregister_on_service_method_unsubscribed_handler_called_); +} + +TEST_F(MethodUnsubscriptionRegistrationGuardFixture, DestroyingGuardCallsUnregister) +{ + GivenAMethodUnsubscriptionRegistrationGuard(); + + // Expecting that UnregisterOnServiceMethodUnsubscribedHandler is called with the asil_level and + // SkeletonInstanceIdentifier used to create the guard + EXPECT_CALL(message_passing_service_mock_, + UnregisterOnServiceMethodUnsubscribedHandler(kAsilLevel, kSkeletonInstanceIdentifier)); + + // When destroying the MethodUnsubscriptionRegistrationGuard + method_unsubscription_registration_guard_.reset(); + + // Then UnregisterOnServiceMethodUnsubscribedHandler is called + EXPECT_TRUE(unregister_on_service_method_unsubscribed_handler_called_); +} + +TEST_F(MethodUnsubscriptionRegistrationGuardFixture, DestroyingGuardAfterScopeHasExpiredDoesNotCallUnregister) +{ + GivenAMethodUnsubscriptionRegistrationGuard(); + + // and given that the scope has expired + scope_.Expire(); + + // Expecting that UnregisterOnServiceMethodUnsubscribedHandler is never called + EXPECT_CALL(message_passing_service_mock_, UnregisterOnServiceMethodUnsubscribedHandler(_, _)).Times(0); + + // When destroying the MethodUnsubscriptionRegistrationGuard + method_unsubscription_registration_guard_.reset(); + + // Then UnregisterOnServiceMethodUnsubscribedHandler is not called + EXPECT_FALSE(unregister_on_service_method_unsubscribed_handler_called_); +} + +} // namespace +} // namespace score::mw::com::impl::lola::detail diff --git a/score/mw/com/impl/bindings/lola/methods/method_resource_map.cpp b/score/mw/com/impl/bindings/lola/methods/method_resource_map.cpp index f782c01ec..3b4a6119e 100644 --- a/score/mw/com/impl/bindings/lola/methods/method_resource_map.cpp +++ b/score/mw/com/impl/bindings/lola/methods/method_resource_map.cpp @@ -90,4 +90,20 @@ auto MethodResourceMap::Clear() -> void resource_map_.clear(); } +void MethodResourceMap::Remove(const ProxyInstanceIdentifier proxy_instance_identifier) +{ + auto resources_it = resource_map_.find(proxy_instance_identifier.application_id); + if (resources_it == resource_map_.end()) + { + return; + } + + auto& inner_map = resources_it->second.inner_resource_map; + inner_map.erase(proxy_instance_identifier.proxy_instance_counter); + if (inner_map.empty()) + { + resource_map_.erase(resources_it); + } +} + } // namespace score::mw::com::impl::lola diff --git a/score/mw/com/impl/bindings/lola/methods/method_resource_map.h b/score/mw/com/impl/bindings/lola/methods/method_resource_map.h index c8f9f1bc3..89ef9168e 100644 --- a/score/mw/com/impl/bindings/lola/methods/method_resource_map.h +++ b/score/mw/com/impl/bindings/lola/methods/method_resource_map.h @@ -79,6 +79,13 @@ class MethodResourceMap auto CleanUpOldRegions(const ProxyInstanceIdentifier proxy_instance_identifier, const pid_t proxy_pid) -> CleanUpResult; + /// \brief Removes the ISharedMemoryResource corresponding to the provided ProxyInstanceIdentifier. + /// + /// This is called on Skeleton side when the Proxy notifies that it is being destroyed, so that the Skeleton can + /// close the proxy's shared memory region. If the provided ProxyInstanceIdentifier does not exist in the map, this + /// is a no-op (it may have already been cleaned up e.g. by CleanUpOldRegions). + void Remove(const ProxyInstanceIdentifier proxy_instance_identifier); + void Clear(); private: diff --git a/score/mw/com/impl/bindings/lola/methods/method_resource_map_test.cpp b/score/mw/com/impl/bindings/lola/methods/method_resource_map_test.cpp index f1e1cde88..4164fb602 100644 --- a/score/mw/com/impl/bindings/lola/methods/method_resource_map_test.cpp +++ b/score/mw/com/impl/bindings/lola/methods/method_resource_map_test.cpp @@ -246,6 +246,71 @@ TEST_F(MethodResourceMapContainsFixture, ContainsReturnsTrueWhenElementMatchingK EXPECT_TRUE(does_contain); } +using MethodResourceMapRemoveFixture = MethodResourceMapFixture; +TEST_F(MethodResourceMapRemoveFixture, RemoveDeletesEntryForGivenProxyInstanceIdentifier) +{ + GivenAMethodResourceMap().WithAnInsertedRegion(ProxyInstanceIdentifier{kProcessIdentifier1, kProxyInstanceCounter1}, + kDummyPid1); + + // When removing the element that was inserted + method_resource_map_->Remove(ProxyInstanceIdentifier{kProcessIdentifier1, kProxyInstanceCounter1}); + + // Then the element should no longer be contained in the map + EXPECT_FALSE(method_resource_map_->Contains(ProxyInstanceIdentifier{kProcessIdentifier1, kProxyInstanceCounter1}, + kDummyPid1)); +} + +TEST_F(MethodResourceMapRemoveFixture, RemoveForNonExistentEntryIsANoOp) +{ + GivenAMethodResourceMap().WithAnInsertedRegion(ProxyInstanceIdentifier{kProcessIdentifier1, kProxyInstanceCounter1}, + kDummyPid1); + + // When removing an element that was never inserted (different process identifier) + // Then the program should not terminate and should remain a no-op + method_resource_map_->Remove(ProxyInstanceIdentifier{kProcessIdentifier2, kProxyInstanceCounter1}); + + // and the originally inserted element should still be contained in the map + EXPECT_TRUE(method_resource_map_->Contains(ProxyInstanceIdentifier{kProcessIdentifier1, kProxyInstanceCounter1}, + kDummyPid1)); +} + +TEST_F(MethodResourceMapRemoveFixture, RemoveOnlyDeletesSpecifiedProxyNotOthers) +{ + GivenAMethodResourceMap() + .WithAnInsertedRegion(ProxyInstanceIdentifier{kProcessIdentifier1, kProxyInstanceCounter1}, kDummyPid1) + .WithAnInsertedRegion(ProxyInstanceIdentifier{kProcessIdentifier2, kProxyInstanceCounter1}, kDummyPid2); + + // When removing only one of the two inserted elements + method_resource_map_->Remove(ProxyInstanceIdentifier{kProcessIdentifier1, kProxyInstanceCounter1}); + + // Then only the removed element should no longer be contained in the map + EXPECT_FALSE(method_resource_map_->Contains(ProxyInstanceIdentifier{kProcessIdentifier1, kProxyInstanceCounter1}, + kDummyPid1)); + + // and the other element should still be contained in the map + EXPECT_TRUE(method_resource_map_->Contains(ProxyInstanceIdentifier{kProcessIdentifier2, kProxyInstanceCounter1}, + kDummyPid2)); +} + +TEST_F(MethodResourceMapRemoveFixture, RemoveOneOfTwoProxiesWithSameApplicationIdLeavesOtherIntact) +{ + // Given two proxies sharing the same application_id but with different instance counters + GivenAMethodResourceMap() + .WithAnInsertedRegion(ProxyInstanceIdentifier{kProcessIdentifier1, kProxyInstanceCounter1}, kDummyPid1) + .WithAnInsertedRegion(ProxyInstanceIdentifier{kProcessIdentifier1, kProxyInstanceCounter2}, kDummyPid1); + + // When removing one of the two proxies (same application_id, different counter) + method_resource_map_->Remove(ProxyInstanceIdentifier{kProcessIdentifier1, kProxyInstanceCounter1}); + + // Then the removed proxy should no longer be contained + EXPECT_FALSE(method_resource_map_->Contains(ProxyInstanceIdentifier{kProcessIdentifier1, kProxyInstanceCounter1}, + kDummyPid1)); + + // and the other proxy sharing the same application_id should still be contained + EXPECT_TRUE(method_resource_map_->Contains(ProxyInstanceIdentifier{kProcessIdentifier1, kProxyInstanceCounter2}, + kDummyPid1)); +} + using MethodResourceMapClearFixture = MethodResourceMapFixture; TEST_F(MethodResourceMapClearFixture, ClearingRemovesAllElements) { diff --git a/score/mw/com/impl/bindings/lola/proxy.cpp b/score/mw/com/impl/bindings/lola/proxy.cpp index 0ebbca10e..8d5a58980 100644 --- a/score/mw/com/impl/bindings/lola/proxy.cpp +++ b/score/mw/com/impl/bindings/lola/proxy.cpp @@ -432,6 +432,7 @@ Proxy::Proxy(std::shared_ptr control, proxy_instance_counter}, offered_state_machine_{}, are_proxy_methods_setup_{false}, + are_proxy_methods_subscribed_{false}, filesystem_{filesystem}, find_service_guard_{std::make_unique( [this](ServiceHandleContainer service_handle_container, FindServiceHandle) { @@ -446,7 +447,10 @@ Proxy::Proxy(std::shared_ptr control, { } -Proxy::~Proxy() = default; +Proxy::~Proxy() +{ + TeardownMethods(); +} void Proxy::ServiceAvailabilityChangeHandler(const bool is_service_available) { @@ -483,6 +487,7 @@ void Proxy::ServiceAvailabilityChangeHandler(const bool is_service_available) if (offered_state_machine_.GetCurrentState() == OfferedStateMachine::State::STOP_OFFERED) { std::lock_guard lock{proxy_method_registration_mutex_}; + are_proxy_methods_subscribed_.store(false); for (auto& proxy_method : proxy_methods_) { proxy_method.second.get().MarkUnsubscribed(); @@ -519,6 +524,7 @@ void Proxy::ServiceAvailabilityChangeHandler(const bool is_service_available) else { std::lock_guard lock{proxy_method_registration_mutex_}; + are_proxy_methods_subscribed_.store(true); for (auto& proxy_method : proxy_methods_) { proxy_method.second.get().MarkSubscribed(); @@ -724,6 +730,7 @@ score::Result Proxy::SetupMethods() if (subscription_result.has_value()) { std::lock_guard lock{proxy_method_registration_mutex_}; + are_proxy_methods_subscribed_.store(true); for (auto& proxy_method : proxy_methods_) { proxy_method.second.get().MarkSubscribed(); @@ -732,6 +739,44 @@ score::Result Proxy::SetupMethods() return subscription_result; } +void Proxy::TeardownMethods() noexcept +{ + // Skip teardown if method SHM was never created (method_shm_resource_ is null, nothing to clean up). + if (!are_proxy_methods_setup_.load()) + { + return; + } + + // Subscription state is tracked directly in Proxy (not via ProxyMethod references) because ProxyMethod + // objects may have already been destroyed by the time TeardownMethods() is called from ~Proxy(). + const bool is_subscribed = are_proxy_methods_subscribed_.load(); + if (is_subscribed) + { + // Best-effort: notify the Skeleton to clean up its registration guards for this Proxy's methods. + // On failure the guards remain until the Skeleton stops offering or restarts, at which point + // they will be cleaned up. + auto& lola_runtime = GetBindingRuntime(BindingType::kLoLa); + auto& lola_message_passing = lola_runtime.GetLolaMessaging(); + const SkeletonInstanceIdentifier skeleton_instance_identifier{ + GetLoLaServiceTypeDeployment(handle_).service_id_, + LolaServiceInstanceId{GetLoLaInstanceDeployment(handle_).instance_id_.value()}.GetId()}; + const auto result = lola_message_passing.UnsubscribeServiceMethod( + quality_type_, skeleton_instance_identifier, proxy_instance_identifier_, GetSourcePid()); + if (!(result.has_value())) + { + score::mw::log::LogWarn("lola") + << __func__ << " " << __LINE__ + << " TeardownMethods: UnsubscribeServiceMethod failed with error: " << result.error(); + } + } + + // Unlink the method SHM name from the filesystem, then release our mapping. + const auto method_shm_path_name = GetMethodChannelShmName(); + memory::shared::SharedMemoryFactory::Remove(method_shm_path_name); + method_shm_resource_.reset(); + memory::shared::SharedMemoryFactory::RemoveStaleArtefacts(method_shm_path_name); +} + memory::shared::SharedMemoryFactory::UserPermissions Proxy::GetSkeletonShmPermissions() const { SCORE_LANGUAGE_FUTURECPP_PRECONDITION_PRD_MESSAGE(data_ != nullptr, diff --git a/score/mw/com/impl/bindings/lola/proxy.h b/score/mw/com/impl/bindings/lola/proxy.h index 2020453d5..827b84002 100644 --- a/score/mw/com/impl/bindings/lola/proxy.h +++ b/score/mw/com/impl/bindings/lola/proxy.h @@ -198,6 +198,7 @@ class Proxy : public ProxyBinding static std::atomic current_proxy_instance_counter_; void ServiceAvailabilityChangeHandler(const bool is_service_available); + void TeardownMethods() noexcept; void InitializeSharedMemoryForMethods( memory::shared::ManagedMemoryResource& memory_resource, const std::vector>& method_data, @@ -258,6 +259,11 @@ class Proxy : public ProxyBinding /// simply ignore any duplicate messages. std::atomic are_proxy_methods_setup_; + /// Tracks whether the proxy methods are currently subscribed (i.e. the Skeleton has acknowledged the subscription). + /// Stored directly in Proxy so that TeardownMethods() can safely check this flag even after the ProxyMethod + /// objects (and their own is_subscribed_ members) have been destroyed. + std::atomic are_proxy_methods_subscribed_; + score::filesystem::Filesystem filesystem_; // We make find_service_guard_ the last member variable since it registers a handler which accesses member variables diff --git a/score/mw/com/impl/bindings/lola/proxy_method_handling_test.cpp b/score/mw/com/impl/bindings/lola/proxy_method_handling_test.cpp index 0898e8e47..3397969ec 100644 --- a/score/mw/com/impl/bindings/lola/proxy_method_handling_test.cpp +++ b/score/mw/com/impl/bindings/lola/proxy_method_handling_test.cpp @@ -146,6 +146,11 @@ class ProxyMethodHandlingFixture : public ProxyMockedMemoryFixture }))); } + void TearDown() override + { + proxy_.reset(); + } + ProxyMethodHandlingFixture& GivenAProxy() { SCORE_LANGUAGE_FUTURECPP_ASSERT(configuration_store_ != nullptr); @@ -420,8 +425,10 @@ TEST_F(ProxySetupMethodsPartialRestartFixture, RemovesStaleArtefactsIfShmFileAlr // which returns that it already exists (indicating that a previous Proxy was created which then crashed). EXPECT_CALL(filesystem_fake_.GetStandard(), Exists(StartsWith(kMethodShmChannelPrefix))).WillOnce(Return(true)); - // Expecting that RemoveStaleArtefacts will be called with the same shm path - EXPECT_CALL(shared_memory_factory_mock_guard_.mock_, RemoveStaleArtefacts(StartsWith(kMethodChannelPrefix))); + // Expecting that RemoveStaleArtefacts will be called with the same shm path on setup (partial restart cleanup) + // and again on proxy destruction (TeardownMethods cleanup). + EXPECT_CALL(shared_memory_factory_mock_guard_.mock_, RemoveStaleArtefacts(StartsWith(kMethodChannelPrefix))) + .Times(2); // When calling SetupMethods with the name of the registered ProxyMethod score::cpp::ignore = proxy_->SetupMethods(); @@ -1051,5 +1058,159 @@ TEST_F(ProxyMethodHandlingFixture, EnablingMethodThatDoesNotContainQueueSizeInCo SCORE_LANGUAGE_FUTURECPP_EXPECT_CONTRACT_VIOLATED(score::cpp::ignore = proxy_->SetupMethods()); } +using ProxyTeardownMethodsFixture = ProxyMethodHandlingFixture; + +TEST_F(ProxyTeardownMethodsFixture, DestroyingProxyWithoutCallingSetupMethodsDoesNotCallUnsubscribeServiceMethod) +{ + // Given a proxy with registered methods but SetupMethods was never called + GivenAConfigurationWithEnabledMethods({kDummyMethodName0}) + .GivenAProxy() + .GivenAMockedSharedMemoryResource() + .WithRegisteredProxyMethods( + {{kDummyMethodId0, + TypeErasedCallQueue::TypeErasedElementInfo{ + kValidInArgsTypeErasedDataInfo, kValidReturnTypeTypeErasedDataInfo, kDummyQueueSize0}}}); + + // Expecting that UnsubscribeServiceMethod is never called when methods were not set up + EXPECT_CALL(*mock_service_, UnsubscribeServiceMethod(_, _, _, _)).Times(0); + + // When the proxy is destroyed + proxy_.reset(); +} + +TEST_F(ProxyTeardownMethodsFixture, DestroyingProxyAfterSetupMethodsCallsUnsubscribeServiceMethod) +{ + // Given a proxy that has set up methods + GivenAConfigurationWithEnabledMethods({kDummyMethodName0}) + .GivenAProxy() + .GivenAMockedSharedMemoryResource() + .WithRegisteredProxyMethods( + {{kDummyMethodId0, + TypeErasedCallQueue::TypeErasedElementInfo{ + kValidInArgsTypeErasedDataInfo, kValidReturnTypeTypeErasedDataInfo, kDummyQueueSize0}}}); + + // Expecting that UnsubscribeServiceMethod is called exactly once on destruction + EXPECT_CALL(*mock_service_, UnsubscribeServiceMethod(_, _, _, _)).Times(1); + + score::cpp::ignore = proxy_->SetupMethods(); + + // When the proxy is destroyed + proxy_.reset(); +} + +TEST_F(ProxyTeardownMethodsFixture, DestroyingProxyCallsUnsubscribeServiceMethodWithCorrectSkeletonIdentifierAndPid) +{ + // Given a proxy that has set up methods + GivenAConfigurationWithEnabledMethods({kDummyMethodName0}) + .GivenAProxy() + .GivenAMockedSharedMemoryResource() + .WithRegisteredProxyMethods( + {{kDummyMethodId0, + TypeErasedCallQueue::TypeErasedElementInfo{ + kValidInArgsTypeErasedDataInfo, kValidReturnTypeTypeErasedDataInfo, kDummyQueueSize0}}}); + + // Expecting that UnsubscribeServiceMethod is called with the SkeletonInstanceIdentifier derived from the + // configuration (service_id = kLolaServiceId, instance_id = kLolaInstanceId) and the skeleton pid from shared + // memory (kDummyPid) + EXPECT_CALL(*mock_service_, UnsubscribeServiceMethod(_, _, _, kDummyPid)) + .WillOnce(WithArg<1>(Invoke([](auto skeleton_instance_identifier) -> ResultBlank { + EXPECT_EQ(skeleton_instance_identifier.service_id, kLolaServiceId); + EXPECT_EQ(skeleton_instance_identifier.instance_id, kLolaInstanceId); + return {}; + }))); + + score::cpp::ignore = proxy_->SetupMethods(); + + // When the proxy is destroyed + proxy_.reset(); +} + +TEST_F(ProxyTeardownMethodsFixture, DestroyingProxyCompletesNormallyEvenWhenUnsubscribeServiceMethodFails) +{ + // Given a proxy that has set up methods + GivenAConfigurationWithEnabledMethods({kDummyMethodName0}) + .GivenAProxy() + .GivenAMockedSharedMemoryResource() + .WithRegisteredProxyMethods( + {{kDummyMethodId0, + TypeErasedCallQueue::TypeErasedElementInfo{ + kValidInArgsTypeErasedDataInfo, kValidReturnTypeTypeErasedDataInfo, kDummyQueueSize0}}}); + + // Given that UnsubscribeServiceMethod returns an error (e.g. skeleton already stopped offering) + EXPECT_CALL(*mock_service_, UnsubscribeServiceMethod(_, _, _, _)) + .WillOnce(Return(MakeUnexpected(ComErrc::kCommunicationLinkError))); + + score::cpp::ignore = proxy_->SetupMethods(); + + // When the proxy is destroyed, TeardownMethods is best-effort and should not crash or throw + EXPECT_NO_FATAL_FAILURE(proxy_.reset()); +} + +TEST_F(ProxyTeardownMethodsFixture, + DestroyingProxyAfterSetupMethodsWithFailedSubscriptionDoesNotCallUnsubscribeServiceMethod) +{ + // Given a proxy that has set up methods but whose initial SubscribeServiceMethod failed (e.g. skeleton crashed + // during setup). The are_proxy_methods_setup_ flag is still set to true, but the ProxyMethods are not marked + // subscribed because the subscription call failed. + GivenAConfigurationWithEnabledMethods({kDummyMethodName0}) + .GivenAProxy() + .GivenAMockedSharedMemoryResource() + .WithRegisteredProxyMethods( + {{kDummyMethodId0, + TypeErasedCallQueue::TypeErasedElementInfo{ + kValidInArgsTypeErasedDataInfo, kValidReturnTypeTypeErasedDataInfo, kDummyQueueSize0}}}); + + // Expecting that UnsubscribeServiceMethod is never called because the ProxyMethods are not subscribed + EXPECT_CALL(*mock_service_, SubscribeServiceMethod(_, _, _, _)) + .WillOnce(Return(MakeUnexpected(ComErrc::kCommunicationLinkError))); + EXPECT_CALL(*mock_service_, UnsubscribeServiceMethod(_, _, _, _)).Times(0); + + score::cpp::ignore = proxy_->SetupMethods(); + + // When the proxy is destroyed + proxy_.reset(); +} + +TEST_F(ProxyTeardownMethodsFixture, DestroyingProxyAfterSetupMethodsRemovesShmRegion) +{ + // Given a proxy that has successfully set up methods + GivenAConfigurationWithEnabledMethods({kDummyMethodName0}) + .GivenAProxy() + .GivenAMockedSharedMemoryResource() + .WithRegisteredProxyMethods( + {{kDummyMethodId0, + TypeErasedCallQueue::TypeErasedElementInfo{ + kValidInArgsTypeErasedDataInfo, kValidReturnTypeTypeErasedDataInfo, kDummyQueueSize0}}}); + + // Expecting that a Remove call and a RemoveStaleArtefacts call are made for the method SHM region on destruction + EXPECT_CALL(shared_memory_factory_mock_guard_.mock_, Remove(StartsWith(kMethodChannelPrefix))).Times(1); + EXPECT_CALL(shared_memory_factory_mock_guard_.mock_, RemoveStaleArtefacts(StartsWith(kMethodChannelPrefix))) + .Times(1); + + score::cpp::ignore = proxy_->SetupMethods(); + + // When the proxy is destroyed + proxy_.reset(); +} + +TEST_F(ProxyTeardownMethodsFixture, DestroyingProxyWithoutCallingSetupMethodsDoesNotRemoveShmRegion) +{ + // Given a proxy with registered methods but SetupMethods was never called + GivenAConfigurationWithEnabledMethods({kDummyMethodName0}) + .GivenAProxy() + .GivenAMockedSharedMemoryResource() + .WithRegisteredProxyMethods( + {{kDummyMethodId0, + TypeErasedCallQueue::TypeErasedElementInfo{ + kValidInArgsTypeErasedDataInfo, kValidReturnTypeTypeErasedDataInfo, kDummyQueueSize0}}}); + + // Expecting that Remove and RemoveStaleArtefacts are never called because the SHM was never created + EXPECT_CALL(shared_memory_factory_mock_guard_.mock_, Remove(_)).Times(0); + EXPECT_CALL(shared_memory_factory_mock_guard_.mock_, RemoveStaleArtefacts(_)).Times(0); + + // When the proxy is destroyed + proxy_.reset(); +} + } // namespace } // namespace score::mw::com::impl::lola diff --git a/score/mw/com/impl/bindings/lola/skeleton.cpp b/score/mw/com/impl/bindings/lola/skeleton.cpp index 37fb3d003..688f9240e 100644 --- a/score/mw/com/impl/bindings/lola/skeleton.cpp +++ b/score/mw/com/impl/bindings/lola/skeleton.cpp @@ -329,6 +329,26 @@ auto Skeleton::PrepareOffer(SkeletonEventBindings& events, } method_subscription_registration_guard_qm_.emplace(std::move(qm_registration_result).value()); + // Register an unsubscription handler for QM proxies. The handler uses the same scope as the subscribe handler + // (on_service_method_subscribed_handler_scope_) so that unsubscriptions arriving after StopOffer are silently + // ignored (the scope will have been expired). + auto qm_unsubscription_result = lola_message_passing.RegisterOnServiceMethodUnsubscribedHandler( + QualityType::kASIL_QM, + skeleton_instance_identifier, + IMessagePassingService::ServiceMethodUnsubscribedHandler{ + on_service_method_subscribed_handler_scope_, + [this](const ProxyInstanceIdentifier proxy_instance_identifier) -> ResultBlank { + return OnServiceMethodsUnsubscribed(proxy_instance_identifier); + }}); + if (!(qm_unsubscription_result.has_value())) + { + method_subscription_registration_guard_qm_.reset(); + score::mw::log::LogError("lola") + << "Could not register QM service method unsubscription handler. Returning error."; + return MakeUnexpected(qm_unsubscription_result.error()); + } + method_unsubscription_registration_guard_qm_.emplace(std::move(qm_unsubscription_result).value()); + if (quality_type_ == QualityType::kASIL_B) { auto allowed_consumers_asil_b = GetAllowedConsumers(QualityType::kASIL_B); @@ -347,11 +367,32 @@ auto Skeleton::PrepareOffer(SkeletonEventBindings& events, if (!(asil_b_registration_result)) { method_subscription_registration_guard_qm_.reset(); + method_unsubscription_registration_guard_qm_.reset(); score::mw::log::LogError("lola") << "Could not register ASIL-B service method handler. Returning error."; return MakeUnexpected(asil_b_registration_result.error()); } score::cpp::ignore = method_subscription_registration_guard_asil_b_.emplace(std::move(asil_b_registration_result).value()); + + auto asil_b_unsubscription_result = lola_message_passing.RegisterOnServiceMethodUnsubscribedHandler( + QualityType::kASIL_B, + skeleton_instance_identifier, + IMessagePassingService::ServiceMethodUnsubscribedHandler{ + on_service_method_subscribed_handler_scope_, + [this](const ProxyInstanceIdentifier proxy_instance_identifier) -> ResultBlank { + return OnServiceMethodsUnsubscribed(proxy_instance_identifier); + }}); + if (!(asil_b_unsubscription_result.has_value())) + { + method_subscription_registration_guard_qm_.reset(); + method_unsubscription_registration_guard_qm_.reset(); + method_subscription_registration_guard_asil_b_.reset(); + score::mw::log::LogError("lola") + << "Could not register ASIL-B service method unsubscription handler. Returning error."; + return MakeUnexpected(asil_b_unsubscription_result.error()); + } + score::cpp::ignore = + method_unsubscription_registration_guard_asil_b_.emplace(std::move(asil_b_unsubscription_result).value()); } return {}; @@ -391,6 +432,8 @@ auto Skeleton::PrepareStopOffer(std::optional // Therefore, we first unregister all handlers and then expire the scopes. method_subscription_registration_guard_qm_.reset(); method_subscription_registration_guard_asil_b_.reset(); + method_unsubscription_registration_guard_qm_.reset(); + method_unsubscription_registration_guard_asil_b_.reset(); for (auto& skeleton_method : skeleton_methods_) { skeleton_method.second.get().UnregisterMethodCallHandlers(); @@ -574,6 +617,23 @@ Result Skeleton::OnServiceMethodsSubscribed(const ProxyInstanceIdentifier& return {}; } +ResultBlank Skeleton::OnServiceMethodsUnsubscribed(const ProxyInstanceIdentifier& proxy_instance_identifier) +{ + std::lock_guard lock{on_service_methods_subscribed_mutex_}; + + // Close the proxy's shared memory region + method_resources_.Remove(proxy_instance_identifier); + + // Unregister the method call handler for each SkeletonMethod corresponding to this proxy + for (auto& [method_id, skeleton_method_ref] : skeleton_methods_) + { + const ProxyMethodInstanceIdentifier proxy_method_instance_identifier{proxy_instance_identifier, method_id}; + skeleton_method_ref.get().OnProxyMethodUnsubscribeFinished(proxy_method_instance_identifier); + } + + return {}; +} + auto Skeleton::SubscribeMethods(const MethodData& method_data, const ProxyInstanceIdentifier proxy_instance_identifier, const uid_t proxy_uid, diff --git a/score/mw/com/impl/bindings/lola/skeleton.h b/score/mw/com/impl/bindings/lola/skeleton.h index 321f7602e..cee43f85c 100644 --- a/score/mw/com/impl/bindings/lola/skeleton.h +++ b/score/mw/com/impl/bindings/lola/skeleton.h @@ -20,6 +20,7 @@ #include "score/mw/com/impl/bindings/lola/i_shm_path_builder.h" #include "score/mw/com/impl/bindings/lola/messaging/method_call_registration_guard.h" #include "score/mw/com/impl/bindings/lola/messaging/method_subscription_registration_guard.h" +#include "score/mw/com/impl/bindings/lola/messaging/method_unsubscription_registration_guard.h" #include "score/mw/com/impl/bindings/lola/methods/method_data.h" #include "score/mw/com/impl/bindings/lola/methods/method_resource_map.h" #include "score/mw/com/impl/bindings/lola/methods/proxy_method_instance_identifier.h" @@ -175,6 +176,8 @@ class Skeleton final : public SkeletonBinding const QualityType asil_level, const pid_t proxy_pid); + ResultBlank OnServiceMethodsUnsubscribed(const ProxyInstanceIdentifier& proxy_instance_identifier); + using MethodIdsToUnsubscribe = std::vector; std::pair, MethodIdsToUnsubscribe> SubscribeMethods( @@ -225,6 +228,13 @@ class Skeleton final : public SkeletonBinding std::optional method_subscription_registration_guard_qm_; std::optional method_subscription_registration_guard_asil_b_; + /// \brief RAII guard objects which will unregister a ServiceMethodUnsubscribedHandler on destruction + /// + /// Each guard corresponds to the method unsubscription handler which was registered in Skeleton::PrepareOffer(). + /// The guard objects will be destroyed in Skeleton::PrepareStopOffer(). + std::optional method_unsubscription_registration_guard_qm_; + std::optional method_unsubscription_registration_guard_asil_b_; + bool was_old_shm_region_reopened_; score::filesystem::Filesystem filesystem_; diff --git a/score/mw/com/impl/bindings/lola/skeleton_method.cpp b/score/mw/com/impl/bindings/lola/skeleton_method.cpp index 0ecf18046..0c290edd1 100644 --- a/score/mw/com/impl/bindings/lola/skeleton_method.cpp +++ b/score/mw/com/impl/bindings/lola/skeleton_method.cpp @@ -103,18 +103,12 @@ Result SkeletonMethod::OnProxyMethodSubscribeFinished( } const std::lock_guard lock{registration_guards_mutex_}; - auto insertion_result = - registration_guards_.insert({proxy_method_instance_identifier.proxy_instance_identifier.application_id, - MethodHandlerCleanupPackage{proxy_pid, {}}}); - - insertion_result.first->second.registration_guards.push_back(std::move(registration_result).value()); - - /// ToDo: This check should be added back in when we go away from the intermetidate solution (issue-258913). - /// currently we can not make this check because we store the guards in a vector, which contains all guards for the - /// individual application, i.e. insertion failure is expected behaviour after the first guard is injected. - // SCORE_LANGUAGE_FUTURECPP_ASSERT_PRD_MESSAGE(insertion_result.second, - // "Any old registered handlers must have been unregistered (by destroying its registration " - // "guard) before registering the new one and storing its registration guard in the map!"); + const auto insertion_result = registration_guards_.emplace( + proxy_method_instance_identifier, std::make_pair(proxy_pid, std::move(registration_result).value())); + SCORE_LANGUAGE_FUTURECPP_ASSERT_PRD_MESSAGE( + insertion_result.second, + "Any old registered handlers must have been unregistered (by destroying its registration " + "guard) before registering the new one and storing its registration guard in the map!"); return {}; } @@ -122,11 +116,17 @@ Result SkeletonMethod::OnProxyMethodSubscribeFinished( void SkeletonMethod::OnProxyMethodUnsubscribe(const ProxyMethodInstanceIdentifier proxy_method_instance_identifier) { const std::lock_guard lock{registration_guards_mutex_}; - const auto num_elements_erased = - registration_guards_.erase(proxy_method_instance_identifier.proxy_instance_identifier.application_id); + const auto num_elements_erased = registration_guards_.erase(proxy_method_instance_identifier); SCORE_LANGUAGE_FUTURECPP_PRECONDITION_PRD(num_elements_erased != 0U); } +void SkeletonMethod::OnProxyMethodUnsubscribeFinished( + const ProxyMethodInstanceIdentifier proxy_method_instance_identifier) +{ + const std::lock_guard lock{registration_guards_mutex_}; + registration_guards_.erase(proxy_method_instance_identifier); +} + void SkeletonMethod::UnregisterMethodCallHandlers() { const std::lock_guard lock{registration_guards_mutex_}; @@ -151,17 +151,23 @@ void SkeletonMethod::Call(const std::optional> in_ar void SkeletonMethod::CleanUpOldHandlers(const GlobalConfiguration::ApplicationId application_id, pid_t proxy_pid) { - const std::lock_guard lock{registration_guards_mutex_}; - auto found_handler_cleanup_package_it = registration_guards_.find(application_id); - if (found_handler_cleanup_package_it == registration_guards_.end()) - { - return; - }; - - if (found_handler_cleanup_package_it->second.proxy_pid != proxy_pid) + // Linear scan to erase all stale guards from a crashed application (same application_id, different pid). + // If benchmarking identifies this as a bottleneck, the data structure can be revisited. + for (auto it = registration_guards_.begin(); it != registration_guards_.end();) { - registration_guards_.erase(application_id); + if (it->first.proxy_instance_identifier.application_id == application_id) + { + if (it->second.first == proxy_pid) + { + return; + } + it = registration_guards_.erase(it); + } + else + { + ++it; + } } } diff --git a/score/mw/com/impl/bindings/lola/skeleton_method.h b/score/mw/com/impl/bindings/lola/skeleton_method.h index cc71b70f1..a7de1387f 100644 --- a/score/mw/com/impl/bindings/lola/skeleton_method.h +++ b/score/mw/com/impl/bindings/lola/skeleton_method.h @@ -33,6 +33,7 @@ #include #include #include +#include namespace score::mw::com::impl::lola { @@ -58,6 +59,8 @@ class SkeletonMethod : public SkeletonMethodBinding void OnProxyMethodUnsubscribe(const ProxyMethodInstanceIdentifier proxy_method_instance_identifier); + void OnProxyMethodUnsubscribeFinished(const ProxyMethodInstanceIdentifier proxy_method_instance_identifier); + bool IsRegistered() const; void UnregisterMethodCallHandlers(); @@ -71,18 +74,8 @@ class SkeletonMethod : public SkeletonMethodBinding std::optional return_type_type_erased_info_; std::optional type_erased_callback_; - /// ToDo: We need to store the registration guard objects in a way that we can clean up old registration guards, - /// from old, crashed processes (e.g. by storing the PID of the process which registered the guards and checking if - /// the current pid is different). This is an intermetidate solution and should be revisited after changes are made - /// to the method_resource_map in the issue-258913. - struct MethodHandlerCleanupPackage - { - pid_t proxy_pid; - std::vector registration_guards; - }; - /// We store all registration guards associated with an application, since after the restart all old - /// MethodCallHandlers can be deleted, by the first method that happens to do the cleanup - std::unordered_map registration_guards_; + std::unordered_map> + registration_guards_; std::mutex registration_guards_mutex_; }; diff --git a/score/mw/com/impl/bindings/lola/skeleton_method_handling_test.cpp b/score/mw/com/impl/bindings/lola/skeleton_method_handling_test.cpp index bd05f8aec..3c1e76112 100644 --- a/score/mw/com/impl/bindings/lola/skeleton_method_handling_test.cpp +++ b/score/mw/com/impl/bindings/lola/skeleton_method_handling_test.cpp @@ -159,6 +159,14 @@ class SkeletonMethodHandlingFixture : public SkeletonMockedMemoryFixture skeleton_instance_identifier, method_call_registration_guard_scope_); }))); + + ON_CALL(message_passing_mock_, RegisterOnServiceMethodUnsubscribedHandler(_, _, _)) + .WillByDefault(WithArgs<0, 1>(Invoke([this](auto asil_level, auto skeleton_instance_identifier) { + return MethodUnsubscriptionRegistrationGuardFactory::Create(message_passing_mock_, + asil_level, + skeleton_instance_identifier, + method_call_registration_guard_scope_); + }))); } SkeletonMethodHandlingFixture& GivenASkeletonWithTwoMethods() @@ -216,6 +224,36 @@ class SkeletonMethodHandlingFixture : public SkeletonMockedMemoryFixture return *this; } + SkeletonMethodHandlingFixture& WhichCapturesRegisteredMethodUnsubscribedHandlers() + { + EXPECT_CALL(message_passing_mock_, + RegisterOnServiceMethodUnsubscribedHandler(QualityType::kASIL_QM, skeleton_instance_identifier_, _)) + .WillOnce(WithArgs<0, 1, 2>( + Invoke([this](auto asil_level, auto skeleton_instance_identifier, auto method_unsubscribed_handler) { + captured_method_unsubscribed_handler_qm_.emplace(std::move(method_unsubscribed_handler)); + return MethodUnsubscriptionRegistrationGuardFactory::Create(message_passing_mock_, + asil_level, + skeleton_instance_identifier, + method_call_registration_guard_scope_); + }))); + if (skeleton_->GetInstanceQualityType() == QualityType::kASIL_B) + { + EXPECT_CALL( + message_passing_mock_, + RegisterOnServiceMethodUnsubscribedHandler(QualityType::kASIL_B, skeleton_instance_identifier_, _)) + .WillOnce(WithArgs<0, 1, 2>(Invoke( + [this](auto asil_level, auto skeleton_instance_identifier, auto method_unsubscribed_handler) { + captured_method_unsubscribed_handler_b_.emplace(std::move(method_unsubscribed_handler)); + return MethodUnsubscriptionRegistrationGuardFactory::Create( + message_passing_mock_, + asil_level, + skeleton_instance_identifier, + method_call_registration_guard_scope_); + }))); + } + return *this; + } + SkeletonMethodHandlingFixture& WhichIsOffered() { score::cpp::ignore = skeleton_->PrepareOffer( @@ -256,6 +294,8 @@ class SkeletonMethodHandlingFixture : public SkeletonMockedMemoryFixture MockFunction dumb_mock_type_erased_callback_{}; std::optional captured_method_subscribed_handler_qm_{}; std::optional captured_method_subscribed_handler_b_{}; + std::optional captured_method_unsubscribed_handler_qm_{}; + std::optional captured_method_unsubscribed_handler_b_{}; safecpp::Scope<> method_call_registration_guard_scope_{}; }; @@ -1082,5 +1122,200 @@ TEST_F(SkeletonOnServiceMethodsSubscribedFixture, EXPECT_TRUE(scoped_handler_result.has_value()); } +using SkeletonPrepareOfferUnsubscribeFixture = SkeletonMethodHandlingFixture; +TEST_F(SkeletonPrepareOfferUnsubscribeFixture, PrepareOfferWillRegisterServiceMethodUnsubscribedHandler) +{ + GivenASkeletonWithTwoMethods(); + + // Expecting that RegisterOnServiceMethodUnsubscribedHandler is called on message passing for QM only + EXPECT_CALL(message_passing_mock_, + RegisterOnServiceMethodUnsubscribedHandler(QualityType::kASIL_QM, skeleton_instance_identifier_, _)); + EXPECT_CALL(message_passing_mock_, + RegisterOnServiceMethodUnsubscribedHandler(QualityType::kASIL_B, skeleton_instance_identifier_, _)) + .Times(0); + + // When calling PrepareOffer + const auto result = skeleton_->PrepareOffer( + kEmptyEventBindings, kEmptyFieldBindings, std::move(kEmptyRegisterShmObjectTraceCallback)); + + // Then a valid result is returned + EXPECT_TRUE(result.has_value()); +} + +TEST_F(SkeletonPrepareOfferUnsubscribeFixture, PrepareOfferOnAsilBSkeletonRegistersQmAndAsilBUnsubscribedHandlers) +{ + GivenAnAsilBSkeletonWithTwoMethods(); + + // Expecting that RegisterOnServiceMethodUnsubscribedHandler is called on message passing for QM and ASIL-B + EXPECT_CALL(message_passing_mock_, + RegisterOnServiceMethodUnsubscribedHandler(QualityType::kASIL_QM, skeleton_instance_identifier_, _)); + EXPECT_CALL(message_passing_mock_, + RegisterOnServiceMethodUnsubscribedHandler(QualityType::kASIL_B, skeleton_instance_identifier_, _)); + + // When calling PrepareOffer + const auto result = skeleton_->PrepareOffer( + kEmptyEventBindings, kEmptyFieldBindings, std::move(kEmptyRegisterShmObjectTraceCallback)); + + // Then a valid result is returned + EXPECT_TRUE(result.has_value()); +} + +TEST_F(SkeletonPrepareOfferUnsubscribeFixture, + PrepareOfferReturnsErrorIfRegisterServiceMethodUnsubscribedHandlerReturnsError) +{ + GivenASkeletonWithTwoMethods(); + + // Expecting that RegisterOnServiceMethodUnsubscribedHandler is called on message passing which returns an error + const auto error_code = ComErrc::kCommunicationLinkError; + EXPECT_CALL(message_passing_mock_, + RegisterOnServiceMethodUnsubscribedHandler(QualityType::kASIL_QM, skeleton_instance_identifier_, _)) + .WillOnce(Return(ByMove(MakeUnexpected(error_code)))); + + // Expecting that UnregisterOnServiceMethodUnsubscribedHandler is never called since registration failed before + // anything was registered + EXPECT_CALL(message_passing_mock_, UnregisterOnServiceMethodUnsubscribedHandler(_, _)).Times(0); + + // When calling PrepareOffer + const auto result = skeleton_->PrepareOffer( + kEmptyEventBindings, kEmptyFieldBindings, std::move(kEmptyRegisterShmObjectTraceCallback)); + + // Then an error is returned + ASSERT_FALSE(result.has_value()); + EXPECT_EQ(result.error(), error_code); +} + +TEST_F(SkeletonPrepareOfferUnsubscribeFixture, + PrepareOfferReturnsErrorIfAsilBRegisterServiceMethodUnsubscribedHandlerReturnsError) +{ + GivenAnAsilBSkeletonWithTwoMethods(); + + // Expecting that RegisterOnServiceMethodUnsubscribedHandler is called for QM (succeeds) and ASIL-B (fails) + const auto error_code = ComErrc::kCommunicationLinkError; + EXPECT_CALL(message_passing_mock_, + RegisterOnServiceMethodUnsubscribedHandler(QualityType::kASIL_QM, skeleton_instance_identifier_, _)); + EXPECT_CALL(message_passing_mock_, + RegisterOnServiceMethodUnsubscribedHandler(QualityType::kASIL_B, skeleton_instance_identifier_, _)) + .WillOnce(Return(ByMove(MakeUnexpected(error_code)))); + // Expecting that the QM unsubscription guard is unregistered when ASIL-B registration fails + EXPECT_CALL(message_passing_mock_, + UnregisterOnServiceMethodUnsubscribedHandler(QualityType::kASIL_QM, skeleton_instance_identifier_)); + + // When calling PrepareOffer + const auto result = skeleton_->PrepareOffer( + kEmptyEventBindings, kEmptyFieldBindings, std::move(kEmptyRegisterShmObjectTraceCallback)); + + // Then an error is returned + ASSERT_FALSE(result.has_value()); + EXPECT_EQ(result.error(), error_code); +} + +TEST_F(SkeletonPrepareOfferUnsubscribeFixture, PrepareOfferWillNotCallUnregisterUnsubscribedMethodHandler) +{ + GivenAnAsilBSkeletonWithTwoMethods(); + + // Expecting that RegisterOnServiceMethodUnsubscribedHandler will be called for QM and ASIL-B + EXPECT_CALL(message_passing_mock_, + RegisterOnServiceMethodUnsubscribedHandler(QualityType::kASIL_QM, skeleton_instance_identifier_, _)); + EXPECT_CALL(message_passing_mock_, + RegisterOnServiceMethodUnsubscribedHandler(QualityType::kASIL_B, skeleton_instance_identifier_, _)); + + // Expecting that UnregisterOnServiceMethodUnsubscribedHandler will not be called + EXPECT_CALL(message_passing_mock_, UnregisterOnServiceMethodUnsubscribedHandler(_, _)).Times(0); + + // When calling PrepareOffer + score::cpp::ignore = skeleton_->PrepareOffer( + kEmptyEventBindings, kEmptyFieldBindings, std::move(kEmptyRegisterShmObjectTraceCallback)); +} + +TEST_F(SkeletonPrepareStopOfferFixture, UnregistersQmAndAsilBUnsubscribedMethodHandlers) +{ + GivenAnAsilBSkeletonWithTwoMethods().WhichIsOffered(); + + // Expecting that UnregisterOnServiceMethodUnsubscribedHandler will be called for QM and ASIL-B + EXPECT_CALL(message_passing_mock_, + UnregisterOnServiceMethodUnsubscribedHandler(QualityType::kASIL_QM, skeleton_instance_identifier_)); + EXPECT_CALL(message_passing_mock_, + UnregisterOnServiceMethodUnsubscribedHandler(QualityType::kASIL_B, skeleton_instance_identifier_)); + + // When calling PrepareStopOffer + skeleton_->PrepareStopOffer({}); +} + +TEST_F(SkeletonPrepareStopOfferFixture, UnregistersOnlyQmUnsubscribedMethodHandlerForQmSkeleton) +{ + GivenASkeletonWithTwoMethods().WhichIsOffered(); + + // Expecting that UnregisterOnServiceMethodUnsubscribedHandler will be called for QM only + EXPECT_CALL(message_passing_mock_, + UnregisterOnServiceMethodUnsubscribedHandler(QualityType::kASIL_QM, skeleton_instance_identifier_)); + EXPECT_CALL(message_passing_mock_, + UnregisterOnServiceMethodUnsubscribedHandler(QualityType::kASIL_B, skeleton_instance_identifier_)) + .Times(0); + + // When calling PrepareStopOffer + skeleton_->PrepareStopOffer({}); +} + +using SkeletonOnServiceMethodsUnsubscribedFixture = SkeletonMethodHandlingFixture; +TEST_F(SkeletonOnServiceMethodsUnsubscribedFixture, CallingRemovesShmResource) +{ + GivenASkeletonWithTwoMethods() + .WhichCapturesRegisteredMethodSubscribedHandlers() + .WhichCapturesRegisteredMethodUnsubscribedHandlers() + .WhichIsOffered(); + + // Given that the registered method subscribed handler was called first to open the SHM resource + ASSERT_TRUE(captured_method_subscribed_handler_qm_.has_value()); + score::cpp::ignore = std::invoke(captured_method_subscribed_handler_qm_.value(), + proxy_instance_identifier_qm_, + test::kAllowedQmMethodConsumer, + kDummyPid); + + const auto shm_resource_ref_counter_after_subscribe = mock_method_memory_resource_qm_.use_count(); + + // When calling the registered unsubscribed handler for that proxy + ASSERT_TRUE(captured_method_unsubscribed_handler_qm_.has_value()); + const auto unsubscribe_result = + std::invoke(captured_method_unsubscribed_handler_qm_.value(), proxy_instance_identifier_qm_); + + // Then the result is valid + ASSERT_TRUE(unsubscribe_result.has_value()); + ASSERT_TRUE(unsubscribe_result->has_value()); + + // and the reference counter for the methods SharedMemoryResource should be decremented, indicating it's been + // removed from the Skeleton's state + EXPECT_EQ(mock_method_memory_resource_qm_.use_count(), shm_resource_ref_counter_after_subscribe - 1U); +} + +TEST_F(SkeletonOnServiceMethodsUnsubscribedFixture, CallingUnregistersMethodCallHandlersForAllMethods) +{ + GivenASkeletonWithTwoMethods() + .WhichCapturesRegisteredMethodSubscribedHandlers() + .WhichCapturesRegisteredMethodUnsubscribedHandlers() + .WhichIsOffered(); + + // Given that the registered method subscribed handler was called to register method call handlers + ASSERT_TRUE(captured_method_subscribed_handler_qm_.has_value()); + score::cpp::ignore = std::invoke(captured_method_subscribed_handler_qm_.value(), + proxy_instance_identifier_qm_, + test::kAllowedQmMethodConsumer, + kDummyPid); + + // Expecting that UnregisterMethodCallHandler will be called for both methods corresponding to the proxy + EXPECT_CALL(message_passing_mock_, + UnregisterMethodCallHandler(QualityType::kASIL_QM, foo_proxy_method_identifier_qm_)); + EXPECT_CALL(message_passing_mock_, + UnregisterMethodCallHandler(QualityType::kASIL_QM, dumb_proxy_method_identifier_qm_)); + + // When calling the registered unsubscribed handler for that proxy + ASSERT_TRUE(captured_method_unsubscribed_handler_qm_.has_value()); + const auto unsubscribe_result = + std::invoke(captured_method_unsubscribed_handler_qm_.value(), proxy_instance_identifier_qm_); + + // Then the result is valid + ASSERT_TRUE(unsubscribe_result.has_value()); + ASSERT_TRUE(unsubscribe_result->has_value()); +} + } // namespace } // namespace score::mw::com::impl::lola diff --git a/score/mw/com/impl/bindings/lola/skeleton_method_test.cpp b/score/mw/com/impl/bindings/lola/skeleton_method_test.cpp index 513c7cd93..36f7f540b 100644 --- a/score/mw/com/impl/bindings/lola/skeleton_method_test.cpp +++ b/score/mw/com/impl/bindings/lola/skeleton_method_test.cpp @@ -629,5 +629,99 @@ TEST_F(SkeletonMethodIsRegisteredFixture, IsRegisteredReturnsTrueIfRegisterHandl EXPECT_TRUE(is_registered); } +using SkeletonMethodOnProxyMethodUnsubscribeFinishedFixture = SkeletonMethodFixture; +TEST_F(SkeletonMethodOnProxyMethodUnsubscribeFinishedFixture, CallingBeforeSubscribingIsANoOp) +{ + GivenASkeletonMethod().WithARegisteredCallback(); + + // Expecting that UnregisterMethodCallHandler will never be called since nothing was subscribed + EXPECT_CALL(message_passing_mock_, UnregisterMethodCallHandler(_, _)).Times(0); + + // When calling OnProxyMethodUnsubscribeFinished with a proxy_method_instance_identifier_ that was never subscribed + // Then the program should NOT terminate (unlike OnProxyMethodUnsubscribe which has a PRECONDITION check) + unit_->OnProxyMethodUnsubscribeFinished(proxy_method_instance_identifier_); +} + +TEST_F(SkeletonMethodOnProxyMethodUnsubscribeFinishedFixture, CallingTwiceWithSameIdentifierIsANoOp) +{ + GivenASkeletonMethod().WithARegisteredCallback(); + + EXPECT_CALL(message_passing_mock_, + RegisterMethodCallHandler(QualityType::kASIL_QM, proxy_method_instance_identifier_, _, _)); + EXPECT_CALL(message_passing_mock_, + UnregisterMethodCallHandler(QualityType::kASIL_QM, proxy_method_instance_identifier_)) + .Times(1); // Must fire exactly once — not on the second UnsubscribeFinished call + + // Given that OnProxyMethodSubscribeFinished is called once for proxy_method_instance_identifier_ + const auto result = unit_->OnProxyMethodSubscribeFinished(kTypeErasedInfoWithInArgsAndReturn, + kValidInArgStorage, + kValidReturnStorage, + proxy_method_instance_identifier_, + method_call_handler_scope_, + kAllowedProxyUid, + kAllowedProxyPid, + QualityType::kASIL_QM); + EXPECT_TRUE(result.has_value()); + + // and given that OnProxyMethodUnsubscribeFinished is called once + unit_->OnProxyMethodUnsubscribeFinished(proxy_method_instance_identifier_); + + // When calling OnProxyMethodUnsubscribeFinished a second time with the same identifier + // Then the program should NOT terminate (unlike OnProxyMethodUnsubscribe which has a PRECONDITION check) + unit_->OnProxyMethodUnsubscribeFinished(proxy_method_instance_identifier_); +} + +TEST_F(SkeletonMethodOnProxyMethodUnsubscribeFinishedFixture, + CallingForOneProxyDoesNotAffectOtherProxyWithSameApplicationId) +{ + // Two proxy instances from the SAME application (same application_id, different proxy_instance_counter) + const ProxyInstanceIdentifier proxy_instance_identifier_same_app{kDummyProxyInstanceCounter + 1U, + kDummyApplicationId}; + const ProxyMethodInstanceIdentifier proxy_method_instance_identifier_same_app{proxy_instance_identifier_same_app, + unique_method_identifier_}; + + GivenASkeletonMethod().WithARegisteredCallback(); + + EXPECT_CALL(message_passing_mock_, RegisterMethodCallHandler(kAsilLevel, proxy_method_instance_identifier_, _, _)); + EXPECT_CALL(message_passing_mock_, + RegisterMethodCallHandler(kAsilLevel, proxy_method_instance_identifier_same_app, _, _)); + + // The first proxy's handler is removed by OnProxyMethodUnsubscribeFinished — must not fire on destruction + EXPECT_CALL(message_passing_mock_, UnregisterMethodCallHandler(kAsilLevel, proxy_method_instance_identifier_)) + .Times(1); // fired during OnProxyMethodUnsubscribeFinished + // The second proxy's handler must still be present and unregistered on destruction + EXPECT_CALL(message_passing_mock_, + UnregisterMethodCallHandler(kAsilLevel, proxy_method_instance_identifier_same_app)) + .Times(1); // fired on destruction + + // Given both proxies from the same application subscribed + ASSERT_TRUE(unit_ + ->OnProxyMethodSubscribeFinished(kTypeErasedInfoWithNoInArgsOrReturn, + kEmptyInArgStorage, + kEmptyReturnStorage, + proxy_method_instance_identifier_, + method_call_handler_scope_, + kAllowedProxyUid, + kAllowedProxyPid, + kAsilLevel) + .has_value()); + ASSERT_TRUE(unit_ + ->OnProxyMethodSubscribeFinished(kTypeErasedInfoWithNoInArgsOrReturn, + kEmptyInArgStorage, + kEmptyReturnStorage, + proxy_method_instance_identifier_same_app, + method_call_handler_scope_, + kAllowedProxyUid, + kAllowedProxyPid, + kAsilLevel) + .has_value()); + + // When OnProxyMethodUnsubscribeFinished is called for only the first proxy + unit_->OnProxyMethodUnsubscribeFinished(proxy_method_instance_identifier_); + + // Then the second proxy's handler is still registered and unregistered on destruction + unit_.reset(); +} + } // namespace } // namespace score::mw::com::impl::lola From 6c96a9522224a985c1623d0201df7c7b412f561d Mon Sep 17 00:00:00 2001 From: tejveerpratap Date: Thu, 7 May 2026 13:59:57 +0200 Subject: [PATCH 2/2] Resolved Comment and refactored CleanUpHandlers() logic --- .../messaging/i_message_passing_service.h | 8 ++--- .../message_passing_service_instance.cpp | 2 +- .../message_passing_service_instance.h | 4 +-- ..._passing_service_instance_methods_test.cpp | 3 +- ...unsubscription_registration_guard_test.cpp | 6 ++-- .../lola/methods/method_resource_map.h | 2 +- .../lola/methods/method_resource_map_test.cpp | 3 +- score/mw/com/impl/bindings/lola/proxy.cpp | 2 -- .../impl/bindings/lola/skeleton_method.cpp | 34 +++++++++++++------ 9 files changed, 35 insertions(+), 29 deletions(-) diff --git a/score/mw/com/impl/bindings/lola/messaging/i_message_passing_service.h b/score/mw/com/impl/bindings/lola/messaging/i_message_passing_service.h index 1a5feab82..ab04629e7 100644 --- a/score/mw/com/impl/bindings/lola/messaging/i_message_passing_service.h +++ b/score/mw/com/impl/bindings/lola/messaging/i_message_passing_service.h @@ -92,12 +92,10 @@ class IMessagePassingService /// service method. /// /// When a Proxy is destroyed, it will call UnsubscribeServiceMethod which will send a message to the Skeleton - /// process. The Skeleton process will then call the ServiceMethodUnsubscribedHandler registered in - /// RegisterOnServiceMethodUnsubscribedHandler. The handler should close the proxy's shared memory region and - /// unregister the method call handler corresponding to the proxy's ProxyMethodInstanceIdentifier from each - /// SkeletonMethod. + /// process. The Skeleton process will then call the ServiceMethodUnsubscribedHandler to close the proxy's shared + /// memory region and unregister the method call handler corresponding to the proxy's ProxyMethodInstanceIdentifier + /// from each SkeletonMethod. /// - /// \param proxy_instance_identifier unique identifier of the Proxy instance which unsubscribed from the method. using ServiceMethodUnsubscribedHandler = safecpp::CopyableScopedFunction; diff --git a/score/mw/com/impl/bindings/lola/messaging/message_passing_service_instance.cpp b/score/mw/com/impl/bindings/lola/messaging/message_passing_service_instance.cpp index adeabe796..b866cec78 100644 --- a/score/mw/com/impl/bindings/lola/messaging/message_passing_service_instance.cpp +++ b/score/mw/com/impl/bindings/lola/messaging/message_passing_service_instance.cpp @@ -1326,7 +1326,7 @@ void MessagePassingServiceInstance::RegisterEventNotificationRemote(const Elemen { score::mw::log::LogError("lola") << "MessagePassingService: RegisterEventNotificationRemote called for event" << event_id.ToString() - << "and node_id" << target_node_id << "although event is " << " currently located at node" + << "and node_id" << target_node_id << "although event is currently located at node" << registration_count_inserted.first->second.node_id; registration_count_inserted.first->second.node_id = target_node_id; registration_count_inserted.first->second.counter = 1U; diff --git a/score/mw/com/impl/bindings/lola/messaging/message_passing_service_instance.h b/score/mw/com/impl/bindings/lola/messaging/message_passing_service_instance.h index b9f7811cf..68cf9426a 100644 --- a/score/mw/com/impl/bindings/lola/messaging/message_passing_service_instance.h +++ b/score/mw/com/impl/bindings/lola/messaging/message_passing_service_instance.h @@ -147,8 +147,8 @@ class MessagePassingServiceInstance : public IMessagePassingServiceInstance enum class MessageWithReplyType : std::uint8_t { kSubscribeServiceMethod = 1U, - kCallMethod = 2U, - kUnsubscribeServiceMethod = 3U, + kCallMethod, + kUnsubscribeServiceMethod, }; struct RegisteredNotificationHandler diff --git a/score/mw/com/impl/bindings/lola/messaging/message_passing_service_instance_methods_test.cpp b/score/mw/com/impl/bindings/lola/messaging/message_passing_service_instance_methods_test.cpp index 008b59578..71a3876e3 100644 --- a/score/mw/com/impl/bindings/lola/messaging/message_passing_service_instance_methods_test.cpp +++ b/score/mw/com/impl/bindings/lola/messaging/message_passing_service_instance_methods_test.cpp @@ -1360,7 +1360,8 @@ TEST_F(MessagePassingServiceInstanceUnregisterSubscribeMethodHandlerTest, } using MessagePassingServiceInstanceRegisterUnsubscribeHandlerTest = MessagePassingServiceInstanceMethodsFixture; -TEST_F(MessagePassingServiceInstanceRegisterUnsubscribeHandlerTest, ReregisteringHandlerReturnsError) +TEST_F(MessagePassingServiceInstanceRegisterUnsubscribeHandlerTest, + ReregisteringHandlerWhenOneIsAlreadyRegisteredReturnsError) { ::testing::MockFunction mock_unsubscribe_method_handler_2{}; safecpp::Scope<> unsubscribe_method_handler_scope_2{}; diff --git a/score/mw/com/impl/bindings/lola/messaging/method_unsubscription_registration_guard_test.cpp b/score/mw/com/impl/bindings/lola/messaging/method_unsubscription_registration_guard_test.cpp index 3bbdb852a..0bde17652 100644 --- a/score/mw/com/impl/bindings/lola/messaging/method_unsubscription_registration_guard_test.cpp +++ b/score/mw/com/impl/bindings/lola/messaging/method_unsubscription_registration_guard_test.cpp @@ -69,10 +69,9 @@ TEST_F(MethodUnsubscriptionRegistrationGuardFixture, MethodUnsubscriptionRegistr TEST_F(MethodUnsubscriptionRegistrationGuardFixture, CreatingGuardDoesNotCallUnregister) { - // When creating a MethodUnsubscriptionRegistrationGuard GivenAMethodUnsubscriptionRegistrationGuard(); - // Expecting that UnregisterOnServiceMethodUnsubscribedHandler is never called + // When UnregisterOnServiceMethodUnsubscribedHandler is never called EXPECT_CALL(message_passing_service_mock_, UnregisterOnServiceMethodUnsubscribedHandler(_, _)).Times(0); // Then UnregisterOnServiceMethodUnsubscribedHandler is not called @@ -83,8 +82,7 @@ TEST_F(MethodUnsubscriptionRegistrationGuardFixture, DestroyingGuardCallsUnregis { GivenAMethodUnsubscriptionRegistrationGuard(); - // Expecting that UnregisterOnServiceMethodUnsubscribedHandler is called with the asil_level and - // SkeletonInstanceIdentifier used to create the guard + // Expecting that UnregisterOnServiceMethodUnsubscribedHandler is called EXPECT_CALL(message_passing_service_mock_, UnregisterOnServiceMethodUnsubscribedHandler(kAsilLevel, kSkeletonInstanceIdentifier)); diff --git a/score/mw/com/impl/bindings/lola/methods/method_resource_map.h b/score/mw/com/impl/bindings/lola/methods/method_resource_map.h index 89ef9168e..59405de15 100644 --- a/score/mw/com/impl/bindings/lola/methods/method_resource_map.h +++ b/score/mw/com/impl/bindings/lola/methods/method_resource_map.h @@ -83,7 +83,7 @@ class MethodResourceMap /// /// This is called on Skeleton side when the Proxy notifies that it is being destroyed, so that the Skeleton can /// close the proxy's shared memory region. If the provided ProxyInstanceIdentifier does not exist in the map, this - /// is a no-op (it may have already been cleaned up e.g. by CleanUpOldRegions). + /// is a no-op (it may have already been cleaned up). void Remove(const ProxyInstanceIdentifier proxy_instance_identifier); void Clear(); diff --git a/score/mw/com/impl/bindings/lola/methods/method_resource_map_test.cpp b/score/mw/com/impl/bindings/lola/methods/method_resource_map_test.cpp index 4164fb602..4b2ae18c7 100644 --- a/score/mw/com/impl/bindings/lola/methods/method_resource_map_test.cpp +++ b/score/mw/com/impl/bindings/lola/methods/method_resource_map_test.cpp @@ -266,10 +266,9 @@ TEST_F(MethodResourceMapRemoveFixture, RemoveForNonExistentEntryIsANoOp) kDummyPid1); // When removing an element that was never inserted (different process identifier) - // Then the program should not terminate and should remain a no-op method_resource_map_->Remove(ProxyInstanceIdentifier{kProcessIdentifier2, kProxyInstanceCounter1}); - // and the originally inserted element should still be contained in the map + // Then the program should not terminate and the originally inserted element should still be contained in the map EXPECT_TRUE(method_resource_map_->Contains(ProxyInstanceIdentifier{kProcessIdentifier1, kProxyInstanceCounter1}, kDummyPid1)); } diff --git a/score/mw/com/impl/bindings/lola/proxy.cpp b/score/mw/com/impl/bindings/lola/proxy.cpp index 8d5a58980..a968f82b7 100644 --- a/score/mw/com/impl/bindings/lola/proxy.cpp +++ b/score/mw/com/impl/bindings/lola/proxy.cpp @@ -753,8 +753,6 @@ void Proxy::TeardownMethods() noexcept if (is_subscribed) { // Best-effort: notify the Skeleton to clean up its registration guards for this Proxy's methods. - // On failure the guards remain until the Skeleton stops offering or restarts, at which point - // they will be cleaned up. auto& lola_runtime = GetBindingRuntime(BindingType::kLoLa); auto& lola_message_passing = lola_runtime.GetLolaMessaging(); const SkeletonInstanceIdentifier skeleton_instance_identifier{ diff --git a/score/mw/com/impl/bindings/lola/skeleton_method.cpp b/score/mw/com/impl/bindings/lola/skeleton_method.cpp index 0c290edd1..816f41213 100644 --- a/score/mw/com/impl/bindings/lola/skeleton_method.cpp +++ b/score/mw/com/impl/bindings/lola/skeleton_method.cpp @@ -28,10 +28,12 @@ #include #include +#include #include #include #include #include +#include namespace score::mw::com::impl::lola { @@ -152,23 +154,33 @@ void SkeletonMethod::Call(const std::optional> in_ar void SkeletonMethod::CleanUpOldHandlers(const GlobalConfiguration::ApplicationId application_id, pid_t proxy_pid) { const std::lock_guard lock{registration_guards_mutex_}; + const bool already_registered = std::any_of( + registration_guards_.cbegin(), registration_guards_.cend(), [&application_id, proxy_pid](const auto& entry) { + return entry.first.proxy_instance_identifier.application_id == application_id && + entry.second.first == proxy_pid; + }); + + if (already_registered) + { + return; + } + // Linear scan to erase all stale guards from a crashed application (same application_id, different pid). // If benchmarking identifies this as a bottleneck, the data structure can be revisited. - for (auto it = registration_guards_.begin(); it != registration_guards_.end();) + // NOTE: this can be replaced with erase_if in c++20 + std::vector keys_to_erase; + for (const auto& entry : registration_guards_) { - if (it->first.proxy_instance_identifier.application_id == application_id) + if (entry.first.proxy_instance_identifier.application_id == application_id) { - if (it->second.first == proxy_pid) - { - return; - } - it = registration_guards_.erase(it); - } - else - { - ++it; + keys_to_erase.push_back(entry.first); } } + + for (const auto& key : keys_to_erase) + { + registration_guards_.erase(key); + } } } // namespace score::mw::com::impl::lola