From fb8e64a043166dc5b28c4b7a179fbf484140bf0a Mon Sep 17 00:00:00 2001 From: Krishna Date: Mon, 18 May 2026 16:24:52 +0200 Subject: [PATCH] mw/com: fix non_trivial_constructor flaky test Proxy destruction raced with the skeleton's StopOfferService async notification: the lola Proxy's FindServiceGuard callback iterates event_bindings_ and proxy_methods_ while the proxy is mid-destruction, causing a use-after-free. Mirror the skeleton's StopOfferService chain on the proxy side. The wrapper destructor calls Unsubscribe(), which tells each event/field to unsubscribe and then dispatches to the binding to stop its async source (PrepareUnsubscribe) and then clear its registration maps (FinalizeUnsubscribe). --- score/mw/com/impl/BUILD | 2 + score/mw/com/impl/bindings/lola/BUILD | 1 + .../lola/generic_proxy_event_test.cpp | 10 ++ score/mw/com/impl/bindings/lola/proxy.cpp | 123 +++++++++------- score/mw/com/impl/bindings/lola/proxy.h | 18 ++- .../impl/bindings/lola/proxy_event_common.cpp | 5 - .../impl/bindings/lola/proxy_event_common.h | 2 +- .../bindings/lola/proxy_event_common_test.cpp | 13 +- .../impl/bindings/lola/proxy_event_test.cpp | 10 ++ score/mw/com/impl/bindings/lola/skeleton.cpp | 13 ++ score/mw/com/impl/bindings/lola/skeleton.h | 8 +- .../lola/skeleton_method_handling_test.cpp | 68 ++++++++- ...subscription_state_machine_events_test.cpp | 1 + .../lola/test/proxy_component_test.cpp | 16 +- .../lola/test/proxy_event_test_resources.cpp | 11 ++ .../lola/test/proxy_event_test_resources.h | 2 + .../mw/com/impl/bindings/mock_binding/proxy.h | 12 ++ score/mw/com/impl/generic_proxy.cpp | 30 ++++ score/mw/com/impl/generic_proxy.h | 13 ++ .../impl/mocking/proxy_event_mock_test.cpp | 4 +- .../proxy_wrapper_class_test_view_test.cpp | 25 +++- .../plumbing/proxy_binding_factory_test.cpp | 20 ++- ...proxy_event_field_binding_factory_test.cpp | 14 ++ .../proxy_method_binding_factory_test.cpp | 14 ++ score/mw/com/impl/proxy_base.cpp | 20 +++ score/mw/com/impl/proxy_base.h | 3 + score/mw/com/impl/proxy_binding.h | 8 + score/mw/com/impl/proxy_binding_test.cpp | 2 + score/mw/com/impl/proxy_event_base.cpp | 24 ++- score/mw/com/impl/proxy_event_base.h | 3 + score/mw/com/impl/proxy_event_base_test.cpp | 42 +++++- score/mw/com/impl/proxy_event_test.cpp | 2 - score/mw/com/impl/test/proxy_resources.h | 5 + .../tracing/test/proxy_event_tracing_test.cpp | 12 ++ score/mw/com/impl/traits.h | 38 +++++ score/mw/com/impl/traits_test.cpp | 137 ++++++++++++++++++ 36 files changed, 634 insertions(+), 97 deletions(-) diff --git a/score/mw/com/impl/BUILD b/score/mw/com/impl/BUILD index 3d3363b74..1aebce75e 100644 --- a/score/mw/com/impl/BUILD +++ b/score/mw/com/impl/BUILD @@ -56,6 +56,7 @@ cc_library( "//score/mw/com:__subpackages__", ], deps = [ + ":flag_owner", ":generic_proxy_event", ":handle_type", ":proxy_base", @@ -428,6 +429,7 @@ cc_library( ], deps = [ ":error", + ":flag_owner", ":proxy_binding", ":proxy_event_binding", ":sample_reference_tracker", diff --git a/score/mw/com/impl/bindings/lola/BUILD b/score/mw/com/impl/bindings/lola/BUILD index 2de19d8d4..47ea680cf 100644 --- a/score/mw/com/impl/bindings/lola/BUILD +++ b/score/mw/com/impl/bindings/lola/BUILD @@ -358,6 +358,7 @@ cc_library( ":subscription_state_machine", ":transaction_log_id", ":transaction_log_rollback_executor", + "//score/mw/com/impl:find_service_handle", "//score/mw/com/impl:generic_proxy_event_binding", "//score/mw/com/impl:instance_identifier", "//score/mw/com/impl:instance_specifier", diff --git a/score/mw/com/impl/bindings/lola/generic_proxy_event_test.cpp b/score/mw/com/impl/bindings/lola/generic_proxy_event_test.cpp index 34d4cc51d..ee67bd9bd 100644 --- a/score/mw/com/impl/bindings/lola/generic_proxy_event_test.cpp +++ b/score/mw/com/impl/bindings/lola/generic_proxy_event_test.cpp @@ -39,6 +39,16 @@ class LolaGenericProxyEventFixture : public LolaProxyEventResources return *this; } + // ProxyEventCommon no longer Unsubscribes on destruction; release state-machine state explicitly. + void TearDown() override + { + if (generic_proxy_event_ != nullptr) + { + generic_proxy_event_->Unsubscribe(); + } + ProxyMockedMemoryFixture::TearDown(); + } + std::unique_ptr generic_proxy_event_{nullptr}; }; TEST_F(LolaGenericProxyEventFixture, CanConstructAGenericProxyEvent) diff --git a/score/mw/com/impl/bindings/lola/proxy.cpp b/score/mw/com/impl/bindings/lola/proxy.cpp index e8cb56091..67ae9ae31 100644 --- a/score/mw/com/impl/bindings/lola/proxy.cpp +++ b/score/mw/com/impl/bindings/lola/proxy.cpp @@ -263,50 +263,6 @@ ServiceDataStorage& GetServiceDataStorage(const memory::shared::ManagedMemoryRes } // namespace detail_proxy -class FindServiceGuard final -{ - public: - // Suppress "AUTOSAR C++14 A15-5-3" rule findings. This rule states: "The std::terminate() function shall not be - // called implicitly". std::terminate() is implicitly called from 'find_service_handle_result.value()' in case - // find_service_handle_result doesn't have a value but as we check before with 'has_value()' so no way for throwing - // std::bad_optional_access which leds to std::terminate(). - // coverity[autosar_cpp14_a15_5_3_violation : FALSE] - FindServiceGuard(FindServiceHandler find_service_handler, - EnrichedInstanceIdentifier enriched_instance_identifier) - : service_availability_change_handle_{nullptr} - { - auto& service_discovery = impl::Runtime::getInstance().GetServiceDiscovery(); - const auto find_service_handle_result = service_discovery.StartFindService( - std::move(find_service_handler), std::move(enriched_instance_identifier)); - if (!find_service_handle_result.has_value()) - { - score::mw::log::LogFatal("lola") - << "StartFindService failed with error" << find_service_handle_result.error() << ". Terminating."; - std::terminate(); - } - service_availability_change_handle_ = std::make_unique(find_service_handle_result.value()); - } - - ~FindServiceGuard() noexcept - { - auto& service_discovery = impl::Runtime::getInstance().GetServiceDiscovery(); - const auto stop_find_service_result = service_discovery.StopFindService(*service_availability_change_handle_); - if (!stop_find_service_result.has_value()) - { - score::mw::log::LogError("lola") - << "StopFindService failed with error" << stop_find_service_result.error() << ". Ignoring error."; - } - } - - FindServiceGuard(const FindServiceGuard&) = delete; - FindServiceGuard& operator=(const FindServiceGuard&) = delete; - FindServiceGuard(FindServiceGuard&& other) = delete; - FindServiceGuard& operator=(FindServiceGuard&& other) = delete; - - private: - std::unique_ptr service_availability_change_handle_; -}; - std::atomic Proxy::current_proxy_instance_counter_{0U}; ElementFqId Proxy::EventNameToElementFqIdConverter::Convert(const std::string_view event_name) const noexcept @@ -430,20 +386,20 @@ Proxy::Proxy(std::shared_ptr control, offered_state_machine_{}, are_proxy_methods_setup_{false}, filesystem_{filesystem}, - find_service_guard_{std::make_unique( - [this](ServiceHandleContainer service_handle_container, FindServiceHandle) { - // Suppress Autosar C++14 A8-5-3 states that auto variables shall not be initialized using braced - // initialization. This is a false positive, we don't use auto here - // coverity[autosar_cpp14_a8_5_3_violation : FALSE] - std::lock_guard lock{proxy_event_registration_mutex_}; - is_service_instance_available_ = !service_handle_container.empty(); - ServiceAvailabilityChangeHandler(is_service_instance_available_); - }, - EnrichedInstanceIdentifier{handle_})} + find_service_handle_{} { + StartProxyAutoReconnect(); } -Proxy::~Proxy() = default; +Proxy::~Proxy() +{ + if (!prepare_unsubscribe_called_ || !finalize_unsubscribe_called_) + { + score::mw::log::LogFatal("lola") + << "Proxy destroyed without prior PrepareUnsubscribe + FinalizeUnsubscribe. Terminating."; + std::terminate(); + } +} void Proxy::ServiceAvailabilityChangeHandler(const bool is_service_available) { @@ -929,4 +885,61 @@ void Proxy::RegisterMethod(const UniqueMethodIdentifier method_id, ProxyMethod& SCORE_LANGUAGE_FUTURECPP_ASSERT_PRD_MESSAGE(was_inserted, "Method IDs must be unique!"); } +void Proxy::PrepareUnsubscribe() +{ + StopProxyAutoReconnect(); + prepare_unsubscribe_called_ = true; +} + +void Proxy::FinalizeUnsubscribe() +{ + { + std::lock_guard lock{proxy_event_registration_mutex_}; + event_bindings_.clear(); + is_service_instance_available_ = false; + } + { + std::lock_guard lock{proxy_method_registration_mutex_}; + proxy_methods_.clear(); + } + finalize_unsubscribe_called_ = true; +} + +void Proxy::StartProxyAutoReconnect() +{ + auto& service_discovery = impl::Runtime::getInstance().GetServiceDiscovery(); + const auto find_service_handle_result = service_discovery.StartFindService( + [this](ServiceHandleContainer service_handle_container, FindServiceHandle) { + std::lock_guard lock{proxy_event_registration_mutex_}; + is_service_instance_available_ = !service_handle_container.empty(); + ServiceAvailabilityChangeHandler(is_service_instance_available_); + }, + EnrichedInstanceIdentifier{handle_}); + if (!find_service_handle_result.has_value()) + { + score::mw::log::LogFatal("lola") + << "StartFindService failed with error" << find_service_handle_result.error() << ". Terminating."; + std::terminate(); + } + find_service_handle_ = find_service_handle_result.value(); +} + +void Proxy::StopProxyAutoReconnect() +{ + if (!find_service_handle_.has_value()) + { + return; + } + // StopFindService waits for any in-flight handler to finish; must run before we take + // proxy_event_registration_mutex_ since the handler also takes it. + auto& service_discovery = impl::Runtime::getInstance().GetServiceDiscovery(); + const auto stop_find_service_result = service_discovery.StopFindService(*find_service_handle_); + if (!stop_find_service_result.has_value()) + { + score::mw::log::LogError("lola") + << "StopFindService failed with error" << stop_find_service_result.error() << ". Ignoring error."; + } + find_service_handle_.reset(); +} + } // namespace score::mw::com::impl::lola diff --git a/score/mw/com/impl/bindings/lola/proxy.h b/score/mw/com/impl/bindings/lola/proxy.h index c4cca4320..98504f3ab 100644 --- a/score/mw/com/impl/bindings/lola/proxy.h +++ b/score/mw/com/impl/bindings/lola/proxy.h @@ -27,6 +27,7 @@ #include "score/mw/com/impl/configuration/lola_service_instance_id.h" #include "score/mw/com/impl/configuration/lola_service_type_deployment.h" #include "score/mw/com/impl/configuration/quality_type.h" +#include "score/mw/com/impl/find_service_handle.h" #include "score/mw/com/impl/handle_type.h" #include "score/mw/com/impl/proxy_binding.h" #include "score/mw/com/impl/proxy_event_binding_base.h" @@ -62,7 +63,6 @@ namespace score::mw::com::impl::lola { class IShmPathBuilder; -class FindServiceGuard; namespace detail_proxy { @@ -200,9 +200,15 @@ class Proxy : public ProxyBinding void RegisterMethod(const UniqueMethodIdentifier method_id, ProxyMethod& proxy_method) noexcept; + void PrepareUnsubscribe() override; + void FinalizeUnsubscribe() override; + private: static std::atomic current_proxy_instance_counter_; + void StartProxyAutoReconnect(); + void StopProxyAutoReconnect(); + void ServiceAvailabilityChangeHandler(const bool is_service_available); void InitializeSharedMemoryForMethods( memory::shared::ManagedMemoryResource& memory_resource, @@ -261,9 +267,13 @@ class Proxy : public ProxyBinding score::filesystem::Filesystem filesystem_; - // We make find_service_guard_ the last member variable since it registers a handler which accesses member variables - // of this class, so they should be initialised first. - std::unique_ptr find_service_guard_; + /// Handle returned by ServiceDiscovery::StartFindService. Held so we can call StopFindService later from + /// StopProxyAutoReconnect(). + std::optional find_service_handle_; + + /// Set by PrepareUnsubscribe / FinalizeUnsubscribe respectively. ~Proxy terminates unless both were called. + bool prepare_unsubscribe_called_{false}; + bool finalize_unsubscribe_called_{false}; }; template diff --git a/score/mw/com/impl/bindings/lola/proxy_event_common.cpp b/score/mw/com/impl/bindings/lola/proxy_event_common.cpp index 1071ad29e..ccbbfe1bf 100644 --- a/score/mw/com/impl/bindings/lola/proxy_event_common.cpp +++ b/score/mw/com/impl/bindings/lola/proxy_event_common.cpp @@ -42,11 +42,6 @@ ProxyEventCommon::ProxyEventCommon(Proxy& parent, const ElementFqId element_fq_i { } -ProxyEventCommon::~ProxyEventCommon() -{ - Unsubscribe(); -} - Result ProxyEventCommon::Subscribe(const std::size_t max_sample_count) { std::stringstream sstream{}; diff --git a/score/mw/com/impl/bindings/lola/proxy_event_common.h b/score/mw/com/impl/bindings/lola/proxy_event_common.h index 55565b1dc..85b2c6f15 100644 --- a/score/mw/com/impl/bindings/lola/proxy_event_common.h +++ b/score/mw/com/impl/bindings/lola/proxy_event_common.h @@ -53,7 +53,7 @@ class ProxyEventCommon final public: ProxyEventCommon(Proxy& parent, const ElementFqId element_fq_id, const std::string_view event_name); - ~ProxyEventCommon(); + ~ProxyEventCommon() = default; ProxyEventCommon(const ProxyEventCommon&) = delete; ProxyEventCommon(ProxyEventCommon&&) noexcept = delete; diff --git a/score/mw/com/impl/bindings/lola/proxy_event_common_test.cpp b/score/mw/com/impl/bindings/lola/proxy_event_common_test.cpp index 0974f30c8..a86d1562e 100644 --- a/score/mw/com/impl/bindings/lola/proxy_event_common_test.cpp +++ b/score/mw/com/impl/bindings/lola/proxy_event_common_test.cpp @@ -100,6 +100,16 @@ class LolaProxyEventCommonFixture : public ProxyMockedMemoryFixture UnregisterEventNotification(QualityType::kASIL_QM, kElementFqId, my_handler_no, kDummyPid)); } + // ProxyEventCommon no longer Unsubscribes on destruction; release state-machine state explicitly. + void TearDown() override + { + if (proxy_event_ != nullptr) + { + proxy_event_->Unsubscribe(); + } + ProxyMockedMemoryFixture::TearDown(); + } + protected: ::testing::MockFunction event_handler_{}; std::unique_ptr proxy_event_{nullptr}; @@ -280,7 +290,8 @@ TYPED_TEST(LolaProxyEventCommonFixture, DestroyingTheProxyEventWillUnregisterEve // Then the receive handler should not be unregistered EXPECT_FALSE(handler_unregistered); - // and when the ProxyEvent is destroyed + // and when the ProxyEvent is unsubscribed and destroyed + this->proxy_event_->Unsubscribe(); this->proxy_event_.reset(); // Then the receive handler should be unregistered diff --git a/score/mw/com/impl/bindings/lola/proxy_event_test.cpp b/score/mw/com/impl/bindings/lola/proxy_event_test.cpp index 06aea0d11..4d5f49bb4 100644 --- a/score/mw/com/impl/bindings/lola/proxy_event_test.cpp +++ b/score/mw/com/impl/bindings/lola/proxy_event_test.cpp @@ -143,6 +143,16 @@ class LolaProxyEventFixture : public LolaProxyEventResources return test_proxy_event_->GetNewSamples(std::move(receiver), guard_factory); } + // ProxyEventCommon no longer Unsubscribes on destruction; release state-machine state explicitly. + void TearDown() override + { + if (test_proxy_event_ != nullptr) + { + test_proxy_event_->Unsubscribe(); + } + ProxyMockedMemoryFixture::TearDown(); + } + std::unique_ptr test_proxy_event_{nullptr}; std::unique_ptr sample_reference_tracker_{}; }; diff --git a/score/mw/com/impl/bindings/lola/skeleton.cpp b/score/mw/com/impl/bindings/lola/skeleton.cpp index 1f0982fce..65f2c62a0 100644 --- a/score/mw/com/impl/bindings/lola/skeleton.cpp +++ b/score/mw/com/impl/bindings/lola/skeleton.cpp @@ -358,6 +358,7 @@ auto Skeleton::PrepareOffer(SkeletonEventBindings& events, method_subscription_registration_guard_asil_b_.emplace(std::move(asil_b_registration_result).value()); } + prepare_offer_called_ = true; return {}; } @@ -370,6 +371,8 @@ auto Skeleton::PrepareOffer(SkeletonEventBindings& events, auto Skeleton::PrepareStopOffer(std::optional unregister_shm_object_callback) noexcept -> void { + prepare_stop_offer_called_ = true; + if (unregister_shm_object_callback.has_value()) { // Suppress "AUTOSAR C++14 A15-4-2" rule finding. This rule states: "I a function is declared to be @@ -443,6 +446,16 @@ auto Skeleton::PrepareStopOffer(std::optional memory_manager_.Reset(); } +Skeleton::~Skeleton() noexcept +{ + if (prepare_offer_called_ && !prepare_stop_offer_called_) + { + score::mw::log::LogFatal("lola") + << "Skeleton was offered but destroyed without prior PrepareStopOffer. Terminating."; + std::terminate(); + } +} + QualityType Skeleton::GetInstanceQualityType() const { return quality_type_; diff --git a/score/mw/com/impl/bindings/lola/skeleton.h b/score/mw/com/impl/bindings/lola/skeleton.h index 321f7602e..68e498b0a 100644 --- a/score/mw/com/impl/bindings/lola/skeleton.h +++ b/score/mw/com/impl/bindings/lola/skeleton.h @@ -99,7 +99,7 @@ class Skeleton final : public SkeletonBinding std::unique_ptr> service_instance_existence_flock_mutex_and_lock); - ~Skeleton() noexcept override = default; + ~Skeleton() noexcept override; Skeleton(const Skeleton&) = delete; Skeleton& operator=(const Skeleton&) = delete; @@ -240,6 +240,12 @@ class Skeleton final : public SkeletonBinding /// This scope will be manually expired in PrepareStopOffer which will prevent any Proxy from subscribing to /// this method. safecpp::Scope<> on_service_method_subscribed_handler_scope_; + + /// Set by PrepareOffer / PrepareStopOffer respectively. ~Skeleton terminates if PrepareOffer was called but + /// PrepareStopOffer was not — i.e. the skeleton registered method handlers and is being destroyed without + /// unregistering them. + bool prepare_offer_called_{false}; + bool prepare_stop_offer_called_{false}; }; template 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..f2b17a031 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 @@ -279,6 +279,8 @@ TEST_F(SkeletonPrepareOfferFixture, PrepareOfferWillRegisterServiceMethodSubscri // Then a valid result is returned EXPECT_TRUE(result.has_value()); + + skeleton_->PrepareStopOffer({}); } TEST_F(SkeletonPrepareOfferFixture, PrepareOfferOnAsilBSkeletonWillRegisterQmAndAsilBServiceMethodSubscribedHandler) @@ -298,6 +300,8 @@ TEST_F(SkeletonPrepareOfferFixture, PrepareOfferOnAsilBSkeletonWillRegisterQmAnd // Then a valid result is returned EXPECT_TRUE(result.has_value()); + + skeleton_->PrepareStopOffer({}); } TEST_F(SkeletonPrepareOfferFixture, PrepareOfferReturnsErrorIfRegisterServiceMethodSubscribedHandlerReturnsError) @@ -373,6 +377,8 @@ TEST_F(SkeletonPrepareOfferFixture, PrepareOfferWillNotRegisterServiceMethodSubs // Then a valid result is returned EXPECT_TRUE(result.has_value()); + + skeleton_->PrepareStopOffer({}); } TEST_F(SkeletonPrepareOfferFixture, PrepareOfferWillNotRegisterServiceMethodSubscribedHandlerWhenNoMethodsExistAsilB) @@ -393,24 +399,32 @@ TEST_F(SkeletonPrepareOfferFixture, PrepareOfferWillNotRegisterServiceMethodSubs // Then a valid result is returned EXPECT_TRUE(result.has_value()); + + skeleton_->PrepareStopOffer({}); } TEST_F(SkeletonPrepareOfferFixture, PrepareOfferWillNotCallUnregisterSubscribedMethodHandler) { GivenAnAsilBSkeletonWithTwoMethods(); - // Expecting that RegisterOnServiceMethodSubscribedHandler will be called for QM and ASIL-B + // Enforce ordering: both Registers happen first (during PrepareOffer), and only then are the matching Unregisters + // called (during PrepareStopOffer). If PrepareOffer wrongly issued an Unregister it would not match the next + // expected Register and the test would fail. + InSequence seq{}; EXPECT_CALL(message_passing_mock_, RegisterOnServiceMethodSubscribedHandler(QualityType::kASIL_QM, skeleton_instance_identifier_, _, _)); EXPECT_CALL(message_passing_mock_, RegisterOnServiceMethodSubscribedHandler(QualityType::kASIL_B, skeleton_instance_identifier_, _, _)); - - // Expecting that UnregisterOnServiceMethodSubscribedHandler will not be called for each method for QM and ASIL-B - EXPECT_CALL(message_passing_mock_, UnregisterOnServiceMethodSubscribedHandler(_, _)).Times(0); + EXPECT_CALL(message_passing_mock_, + UnregisterOnServiceMethodSubscribedHandler(QualityType::kASIL_QM, skeleton_instance_identifier_)); + EXPECT_CALL(message_passing_mock_, + UnregisterOnServiceMethodSubscribedHandler(QualityType::kASIL_B, skeleton_instance_identifier_)); // When calling PrepareOffer score::cpp::ignore = skeleton_->PrepareOffer( kEmptyEventBindings, kEmptyFieldBindings, std::move(kEmptyRegisterShmObjectTraceCallback)); + + skeleton_->PrepareStopOffer({}); } TEST_F(SkeletonPrepareOfferFixture, CallingAsilBWillUnregisterQmHandlerOnAsilBRegistrationFailure) @@ -589,6 +603,8 @@ TEST_F(SkeletonOnServiceMethodsSubscribedFixture, // Then the handler should return a valid result ASSERT_TRUE(scoped_handler_result.has_value()); ASSERT_TRUE(scoped_handler_result->has_value()); + + skeleton_->PrepareStopOffer({}); } TEST_F(SkeletonOnServiceMethodsSubscribedFixture, @@ -630,6 +646,8 @@ TEST_F(SkeletonOnServiceMethodsSubscribedFixture, ASSERT_TRUE(scoped_handler_result_b.has_value()); ASSERT_TRUE(scoped_handler_result_b->has_value()); + + skeleton_->PrepareStopOffer({}); } TEST_F(SkeletonOnServiceMethodsSubscribedFixture, CallingReturnsErrorIfRegisteringMethodCallHandlerReturnedError) @@ -653,6 +671,8 @@ TEST_F(SkeletonOnServiceMethodsSubscribedFixture, CallingReturnsErrorIfRegisteri ASSERT_TRUE(scoped_handler_result.has_value()); ASSERT_FALSE(scoped_handler_result->has_value()); EXPECT_EQ(scoped_handler_result->error(), error_code); + + skeleton_->PrepareStopOffer({}); } TEST_F(SkeletonOnServiceMethodsSubscribedFixture, CallingOpensShmIfAlreadyCalledWithDifferentApplicationIdAndSamePid) @@ -683,6 +703,8 @@ TEST_F(SkeletonOnServiceMethodsSubscribedFixture, CallingOpensShmIfAlreadyCalled // Then the result should be valid EXPECT_TRUE(scoped_handler_result.has_value()); + + skeleton_->PrepareStopOffer({}); } TEST_F(SkeletonOnServiceMethodsSubscribedFixture, @@ -714,6 +736,8 @@ TEST_F(SkeletonOnServiceMethodsSubscribedFixture, // Then the result should be valid EXPECT_TRUE(scoped_handler_result.has_value()); + + skeleton_->PrepareStopOffer({}); } TEST_F(SkeletonOnServiceMethodsSubscribedFixture, @@ -743,6 +767,8 @@ TEST_F(SkeletonOnServiceMethodsSubscribedFixture, // Then the result should be valid EXPECT_TRUE(scoped_handler_result.has_value()); + + skeleton_->PrepareStopOffer({}); } TEST_F(SkeletonOnServiceMethodsSubscribedFixture, CallingDoesNotOpenShmIfAlreadyCalledWithSameProxyInstanceIdAndPid) @@ -768,6 +794,8 @@ TEST_F(SkeletonOnServiceMethodsSubscribedFixture, CallingDoesNotOpenShmIfAlready // Then the result should be valid EXPECT_TRUE(scoped_handler_result.has_value()); + + skeleton_->PrepareStopOffer({}); } TEST_F(SkeletonOnServiceMethodsSubscribedFixture, CallingRemovesOldRegionsFromCallWithSameApplicationIdAndDifferentPid) @@ -802,6 +830,8 @@ TEST_F(SkeletonOnServiceMethodsSubscribedFixture, CallingRemovesOldRegionsFromCa // Then the reference counter for the first methods SharedMemoryResource should be have been decremented, // indicating that it's been removed from the Skeleton's state EXPECT_EQ(mock_method_memory_resource_qm_.use_count(), first_initial_shm_resource_ref_counter); + + skeleton_->PrepareStopOffer({}); } TEST_F(SkeletonOnServiceMethodsSubscribedFixture, CallingRegistersAMethodCallHandlerPerMethodWithInfoFromMethodData) @@ -848,6 +878,8 @@ TEST_F(SkeletonOnServiceMethodsSubscribedFixture, CallingRegistersAMethodCallHan proxy_instance_identifier_qm_, test::kAllowedQmMethodConsumer, kDummyPid); + + skeleton_->PrepareStopOffer({}); } TEST_F(SkeletonOnServiceMethodsSubscribedFixture, CallingQmOpensSharedMemoryWithProxyUidAsAllowedProvider) @@ -874,6 +906,8 @@ TEST_F(SkeletonOnServiceMethodsSubscribedFixture, CallingQmOpensSharedMemoryWith // Then the result should be valid EXPECT_TRUE(scoped_handler_result.has_value()); + + skeleton_->PrepareStopOffer({}); } TEST_F(SkeletonOnServiceMethodsSubscribedFixture, CallingAsilBOpensSharedMemoryWithProxyUidAsAllowedProvider) @@ -900,6 +934,8 @@ TEST_F(SkeletonOnServiceMethodsSubscribedFixture, CallingAsilBOpensSharedMemoryW // Then the result should be valid EXPECT_TRUE(scoped_handler_result.has_value()); + + skeleton_->PrepareStopOffer({}); } TEST_F(SkeletonOnServiceMethodsSubscribedFixture, CallingStoresSharedMemoryInClassState) @@ -918,6 +954,8 @@ TEST_F(SkeletonOnServiceMethodsSubscribedFixture, CallingStoresSharedMemoryInCla // Then the reference counter for the methods SharedMemoryResource should be incremented, indicating that it's // been stored in the Skeleton's state EXPECT_EQ(mock_method_memory_resource_qm_.use_count(), initial_shm_resource_ref_counter + 1U); + + skeleton_->PrepareStopOffer({}); } TEST_F(SkeletonOnServiceMethodsSubscribedFixture, FailingToOpenSharedMemoryReturnsError) @@ -938,6 +976,8 @@ TEST_F(SkeletonOnServiceMethodsSubscribedFixture, FailingToOpenSharedMemoryRetur ASSERT_TRUE(scoped_handler_result.has_value()); ASSERT_FALSE(scoped_handler_result->has_value()); EXPECT_EQ(scoped_handler_result->error(), ComErrc::kBindingFailure); + + skeleton_->PrepareStopOffer({}); } TEST_F(SkeletonOnServiceMethodsSubscribedFixture, FailingToGetUsableBaseAddressForRetrievingMethodDataTerminates) @@ -955,6 +995,8 @@ TEST_F(SkeletonOnServiceMethodsSubscribedFixture, FailingToGetUsableBaseAddressF proxy_instance_identifier_qm_, test::kAllowedQmMethodConsumer, kDummyPid)); + + skeleton_->PrepareStopOffer({}); } TEST_F(SkeletonOnServiceMethodsSubscribedFixture, CallingAsilQmWithoutInArgsOrReturnStillOpensSharedMemory) @@ -973,6 +1015,8 @@ TEST_F(SkeletonOnServiceMethodsSubscribedFixture, CallingAsilQmWithoutInArgsOrRe // Then the result should be valid EXPECT_TRUE(scoped_handler_result.has_value()); + + skeleton_->PrepareStopOffer({}); } TEST_F(SkeletonOnServiceMethodsSubscribedFixture, CallingAsilBWithoutInArgsOrReturnStillOpensSharedMemory) @@ -991,13 +1035,18 @@ TEST_F(SkeletonOnServiceMethodsSubscribedFixture, CallingAsilBWithoutInArgsOrRet // Then the result should be valid EXPECT_TRUE(scoped_handler_result.has_value()); + + skeleton_->PrepareStopOffer({}); } TEST_F(SkeletonOnServiceMethodsSubscribedFixture, CallingAsilBWillNotCallUnregisterMethodCallHandler) { GivenAnAsilBSkeletonWithTwoMethods().WhichCapturesRegisteredMethodSubscribedHandlers().WhichIsOffered(); - // Expecting that RegisterMethodCallHandler will be called for each method for QM and ASIL-B + // Enforce ordering: all four Registers happen first (during the subscribed-handler invocations below), and only + // then are the matching Unregisters called (during PrepareStopOffer). If the subscribed-handler invocations + // wrongly issued an Unregister it would not match the next expected Register and the test would fail. + InSequence seq{}; EXPECT_CALL(message_passing_mock_, RegisterMethodCallHandler(QualityType::kASIL_QM, foo_proxy_method_identifier_qm_, _, _)); EXPECT_CALL(message_passing_mock_, @@ -1008,8 +1057,7 @@ TEST_F(SkeletonOnServiceMethodsSubscribedFixture, CallingAsilBWillNotCallUnregis EXPECT_CALL(message_passing_mock_, RegisterMethodCallHandler(QualityType::kASIL_B, dumb_proxy_method_identifier_b_, _, _)); - // Expecting that UnregisterMethodCallHandler will not be called for each method for QM and ASIL-B - EXPECT_CALL(message_passing_mock_, UnregisterMethodCallHandler(_, _)).Times(0); + EXPECT_CALL(message_passing_mock_, UnregisterMethodCallHandler(_, _)).Times(4); // When calling the registered method subscribed handler for both QM and AsilB ASSERT_TRUE(captured_method_subscribed_handler_qm_.has_value()); @@ -1025,6 +1073,8 @@ TEST_F(SkeletonOnServiceMethodsSubscribedFixture, CallingAsilBWillNotCallUnregis test::kAllowedAsilBMethodConsumer, kDummyPid); EXPECT_TRUE(scoped_handler_result_2.has_value()); + + skeleton_->PrepareStopOffer({}); } TEST_F(SkeletonOnServiceMethodsSubscribedFixture, CallingWillUnregisterRegisteredMethodCallHandlersOnSubscriptionError) @@ -1052,6 +1102,8 @@ TEST_F(SkeletonOnServiceMethodsSubscribedFixture, CallingWillUnregisterRegistere test::kAllowedQmMethodConsumer, kDummyPid); EXPECT_TRUE(scoped_handler_result.has_value()); + + skeleton_->PrepareStopOffer({}); } TEST_F(SkeletonOnServiceMethodsSubscribedFixture, @@ -1080,6 +1132,8 @@ TEST_F(SkeletonOnServiceMethodsSubscribedFixture, test::kAllowedQmMethodConsumer, kDummyPid); EXPECT_TRUE(scoped_handler_result.has_value()); + + skeleton_->PrepareStopOffer({}); } } // namespace diff --git a/score/mw/com/impl/bindings/lola/subscription_state_machine_events_test.cpp b/score/mw/com/impl/bindings/lola/subscription_state_machine_events_test.cpp index 16d6e4cab..952a70585 100644 --- a/score/mw/com/impl/bindings/lola/subscription_state_machine_events_test.cpp +++ b/score/mw/com/impl/bindings/lola/subscription_state_machine_events_test.cpp @@ -63,6 +63,7 @@ class StateMachineEventsFixture : public LolaProxyEventResources // Specifically, it's important that the Unsubscribe is recorded so that when ~TransactionLogRegistrationGuard // unregisters the TransactionLog, there are no open transactions. state_machine_.UnsubscribeEvent(); + ProxyMockedMemoryFixture::TearDown(); } void EnterSubscriptionPending(const std::size_t max_samples) noexcept diff --git a/score/mw/com/impl/bindings/lola/test/proxy_component_test.cpp b/score/mw/com/impl/bindings/lola/test/proxy_component_test.cpp index b8d7b7719..856479fad 100644 --- a/score/mw/com/impl/bindings/lola/test/proxy_component_test.cpp +++ b/score/mw/com/impl/bindings/lola/test/proxy_component_test.cpp @@ -92,6 +92,12 @@ class ProxyWithRealMemFixture : public ::testing::Test void TearDown() override { + if (created_proxy_ != nullptr) + { + created_proxy_->PrepareUnsubscribe(); + created_proxy_->FinalizeUnsubscribe(); + } + for (const auto& file : shm_files_) { std::ignore = score::filesystem::IStandardFilesystem::instance().Remove(std::string{"/dev/shm"} + file); @@ -165,6 +171,8 @@ class ProxyWithRealMemFixture : public ::testing::Test std::unique_ptr config_store_{nullptr}; + std::unique_ptr created_proxy_{nullptr}; + std::vector shm_files_{}; RuntimeMockGuard runtime_mock_{}; lola::RuntimeMock lola_runtime_mock_{}; @@ -191,10 +199,10 @@ TEST_F(ProxyWithRealMemFixture, IsEventProvidedOnlyReturnsTrueIfEventIsInSharedM ON_CALL(service_discovery_mock_, StartFindService(::testing::_, EnrichedInstanceIdentifier{handle})) .WillByDefault(::testing::Return(make_FindServiceHandle(10U))); - auto proxy = Proxy::Create(handle); - ASSERT_NE(proxy, nullptr); - EXPECT_TRUE(proxy->IsEventProvided(kEventName)); - EXPECT_FALSE(proxy->IsEventProvided(kNonProvidedEventName)); + created_proxy_ = Proxy::Create(handle); + ASSERT_NE(created_proxy_, nullptr); + EXPECT_TRUE(created_proxy_->IsEventProvided(kEventName)); + EXPECT_FALSE(created_proxy_->IsEventProvided(kNonProvidedEventName)); } class ProxyServiceDiscoveryFixture : public ProxyWithRealMemFixture diff --git a/score/mw/com/impl/bindings/lola/test/proxy_event_test_resources.cpp b/score/mw/com/impl/bindings/lola/test/proxy_event_test_resources.cpp index 884d739c8..7c71ca05f 100644 --- a/score/mw/com/impl/bindings/lola/test/proxy_event_test_resources.cpp +++ b/score/mw/com/impl/bindings/lola/test/proxy_event_test_resources.cpp @@ -91,6 +91,17 @@ void ProxyMockedMemoryFixture::ExpectDataSegmentOpened() })); } +void ProxyMockedMemoryFixture::TearDown() +{ + // lola Proxy terminates on destruction if PrepareUnsubscribe + FinalizeUnsubscribe were not called first, + // so we run them here for tests that constructed a proxy directly. + if (proxy_ != nullptr) + { + proxy_->PrepareUnsubscribe(); + proxy_->FinalizeUnsubscribe(); + } +} + void ProxyMockedMemoryFixture::InitialiseProxyWithConstructor(const InstanceIdentifier& instance_identifier) { EnrichedInstanceIdentifier enriched_instance_identifier{instance_identifier}; diff --git a/score/mw/com/impl/bindings/lola/test/proxy_event_test_resources.h b/score/mw/com/impl/bindings/lola/test/proxy_event_test_resources.h index a8240c6cc..8d9ee106c 100644 --- a/score/mw/com/impl/bindings/lola/test/proxy_event_test_resources.h +++ b/score/mw/com/impl/bindings/lola/test/proxy_event_test_resources.h @@ -169,6 +169,8 @@ class ProxyMockedMemoryFixture : public ::testing::Test ProxyMockedMemoryFixture() noexcept; + void TearDown() override; + void InitialiseProxyWithConstructor(const InstanceIdentifier& instance_identifier); void InitialiseProxyWithCreate(const InstanceIdentifier& instance_identifier); diff --git a/score/mw/com/impl/bindings/mock_binding/proxy.h b/score/mw/com/impl/bindings/mock_binding/proxy.h index 812581d18..839d35d03 100644 --- a/score/mw/com/impl/bindings/mock_binding/proxy.h +++ b/score/mw/com/impl/bindings/mock_binding/proxy.h @@ -32,6 +32,8 @@ class Proxy : public ProxyBinding MOCK_METHOD(void, RegisterEventBinding, (std::string_view, ProxyEventBindingBase&), (noexcept, override)); MOCK_METHOD(void, UnregisterEventBinding, (std::string_view), (noexcept, override)); MOCK_METHOD(Result, SetupMethods, (), (override)); + MOCK_METHOD(void, PrepareUnsubscribe, (), (override)); + MOCK_METHOD(void, FinalizeUnsubscribe, (), (override)); }; class ProxyFacade : public ProxyBinding @@ -61,6 +63,16 @@ class ProxyFacade : public ProxyBinding return proxy_.SetupMethods(); } + void PrepareUnsubscribe() override + { + proxy_.PrepareUnsubscribe(); + } + + void FinalizeUnsubscribe() override + { + proxy_.FinalizeUnsubscribe(); + } + private: Proxy& proxy_; }; diff --git a/score/mw/com/impl/generic_proxy.cpp b/score/mw/com/impl/generic_proxy.cpp index 3e2091287..d61d05f1e 100644 --- a/score/mw/com/impl/generic_proxy.cpp +++ b/score/mw/com/impl/generic_proxy.cpp @@ -90,6 +90,36 @@ GenericProxy::GenericProxy(std::unique_ptr proxy_binding, HandleTy { } +GenericProxy::~GenericProxy() noexcept +{ + if (is_proxy_owner_.IsSet()) + { + this->Unsubscribe(); + } +} + +GenericProxy::GenericProxy(GenericProxy&& other) noexcept + : ProxyBase{std::move(other)}, + events_{std::move(other.events_)}, + is_proxy_owner_{std::move(other.is_proxy_owner_)} +{ +} + +GenericProxy& GenericProxy::operator=(GenericProxy&& other) noexcept +{ + if (&other != this) + { + if (is_proxy_owner_.IsSet()) + { + this->Unsubscribe(); + } + ProxyBase::operator=(std::move(other)); + events_ = std::move(other.events_); + is_proxy_owner_ = std::move(other.is_proxy_owner_); + } + return *this; +} + void GenericProxy::FillEventMap(const std::vector& event_names) noexcept { for (const auto event_name : event_names) diff --git a/score/mw/com/impl/generic_proxy.h b/score/mw/com/impl/generic_proxy.h index afd71027c..528dc01c6 100644 --- a/score/mw/com/impl/generic_proxy.h +++ b/score/mw/com/impl/generic_proxy.h @@ -18,6 +18,7 @@ #ifndef SCORE_MW_COM_IMPL_GENERIC_PROXY_H #define SCORE_MW_COM_IMPL_GENERIC_PROXY_H +#include "score/mw/com/impl/flag_owner.h" #include "score/mw/com/impl/generic_proxy_event.h" #include "score/mw/com/impl/handle_type.h" #include "score/mw/com/impl/proxy_base.h" @@ -69,6 +70,14 @@ class GenericProxy : public ProxyBase */ static Result Create(HandleType instance_handle) noexcept; + ~GenericProxy() noexcept; + + GenericProxy(const GenericProxy&) = delete; + GenericProxy& operator=(const GenericProxy&) = delete; + + GenericProxy(GenericProxy&& other) noexcept; + GenericProxy& operator=(GenericProxy&& other) noexcept; + /** * \api * \brief Returns a reference to the event map. @@ -82,6 +91,10 @@ class GenericProxy : public ProxyBase void FillEventMap(const std::vector& event_names) noexcept; EventMap events_; + + /// Flag which is checked before calling Unsubscribe in the destructor. + /// Cleared on move so the moved-from instance does not call Unsubscribe. + FlagOwner is_proxy_owner_{true}; }; } // namespace score::mw::com::impl diff --git a/score/mw/com/impl/mocking/proxy_event_mock_test.cpp b/score/mw/com/impl/mocking/proxy_event_mock_test.cpp index 30966ed05..01197caa7 100644 --- a/score/mw/com/impl/mocking/proxy_event_mock_test.cpp +++ b/score/mw/com/impl/mocking/proxy_event_mock_test.cpp @@ -104,7 +104,9 @@ TYPED_TEST(ProxyEventFieldMockFixture, SubscribeReturnsErrorWhenMockReturnsError TYPED_TEST(ProxyEventFieldMockFixture, UnsubscribeDispatchesToMockAfterInjectingMock) { - // Given a ProxyEvent constructed with an empty binding and an injected mock + // Given a subscribed ProxyEvent with an injected mock + EXPECT_CALL(this->proxy_service_element_mock_, Subscribe(kDummyMaxSampleCount)).WillOnce(Return(Result{})); + std::ignore = this->unit_.Subscribe(kDummyMaxSampleCount); // Expecting that Unsubscribe will be called on the mock EXPECT_CALL(this->proxy_service_element_mock_, Unsubscribe()); diff --git a/score/mw/com/impl/mocking/proxy_wrapper_class_test_view_test.cpp b/score/mw/com/impl/mocking/proxy_wrapper_class_test_view_test.cpp index 674527449..c66b563ea 100644 --- a/score/mw/com/impl/mocking/proxy_wrapper_class_test_view_test.cpp +++ b/score/mw/com/impl/mocking/proxy_wrapper_class_test_view_test.cpp @@ -172,12 +172,21 @@ TEST_F(ProxyWrapperTestClassCreateFixture, CallingFunctionsOnMockProxyDispatches proxy.some_event_2.InjectMock(proxy_event_mock_2); proxy.some_field.InjectMock(proxy_field_mock); - // Expecting that OfferService will be called on the Proxy mock and Unsubscribe on the event and field mocks + // Subscribe first so the per-event subscribed flag is set; otherwise Unsubscribe is a silent no-op. + EXPECT_CALL(proxy_event_mock, Subscribe(_)).WillOnce(Return(score::Result{})); + EXPECT_CALL(proxy_event_mock_2, Subscribe(_)).WillOnce(Return(score::Result{})); + EXPECT_CALL(proxy_field_mock, Subscribe(_)).WillOnce(Return(score::Result{})); + + // Expecting that Unsubscribe is called on each event and field mock exactly once. EXPECT_CALL(proxy_event_mock, Unsubscribe()); EXPECT_CALL(proxy_event_mock_2, Unsubscribe()); EXPECT_CALL(proxy_field_mock, Unsubscribe()); - // When calling Unsubscribe on the events and fields. + // When subscribing and then unsubscribing the events and fields. + std::ignore = proxy.some_event.Subscribe(1); + std::ignore = proxy.some_event_2.Subscribe(1); + std::ignore = proxy.some_field.Subscribe(1); + proxy.some_event.Unsubscribe(); proxy.some_event_2.Unsubscribe(); proxy.some_field.Unsubscribe(); @@ -217,10 +226,14 @@ TEST_F(ProxyWrapperTestClassEventsOnlyCreateFixture, CallingFunctionsOnMockProxy auto& proxy_event_mock = std::get<0>(events_tuple).mock; proxy.some_event.InjectMock(proxy_event_mock); + // Subscribe first so the subscribed flag is set; otherwise Unsubscribe is a silent no-op. + EXPECT_CALL(proxy_event_mock, Subscribe(_)).WillOnce(Return(score::Result{})); + // Expecting that Unsubscribe is called on the event EXPECT_CALL(proxy_event_mock, Unsubscribe()); - // When calling Unsubscribe on the event + // When subscribing and then unsubscribing the event + std::ignore = proxy.some_event.Subscribe(1); proxy.some_event.Unsubscribe(); } @@ -256,10 +269,14 @@ TEST_F(ProxyWrapperTestClassFieldsOnlyCreateFixture, CallingFunctionsOnMockProxy auto& proxy_field_mock = (std::get<0>(fields_tuple).mock); proxy.some_field.InjectMock(proxy_field_mock); + // Subscribe first so the subscribed flag is set; otherwise Unsubscribe is a silent no-op. + EXPECT_CALL(proxy_field_mock, Subscribe(_)).WillOnce(Return(score::Result{})); + // Expecting that Unsubscribe is called on the field EXPECT_CALL(proxy_field_mock, Unsubscribe()); - // When calling Unsubscribe on the field + // When subscribing and then unsubscribing the field + std::ignore = proxy.some_field.Subscribe(1); proxy.some_field.Unsubscribe(); } diff --git a/score/mw/com/impl/plumbing/proxy_binding_factory_test.cpp b/score/mw/com/impl/plumbing/proxy_binding_factory_test.cpp index 6d6bc791f..94674d95e 100644 --- a/score/mw/com/impl/plumbing/proxy_binding_factory_test.cpp +++ b/score/mw/com/impl/plumbing/proxy_binding_factory_test.cpp @@ -76,7 +76,21 @@ class SkeletonBindingGuard std::unique_ptr skeleton_binding_; }; -using ProxyBindingFactoryCreateFixture = lola::ProxyMockedMemoryFixture; +class ProxyBindingFactoryCreateFixture : public lola::ProxyMockedMemoryFixture +{ + protected: + void TearDown() override + { + if (created_binding_ != nullptr) + { + created_binding_->PrepareUnsubscribe(); + created_binding_->FinalizeUnsubscribe(); + } + lola::ProxyMockedMemoryFixture::TearDown(); + } + + std::unique_ptr created_binding_{nullptr}; +}; score::cpp::optional GetInstanceId(score::mw::com::impl::InstanceIdentifier identifier) { const InstanceIdentifierView identifier_view{identifier}; @@ -108,10 +122,10 @@ TEST_F(ProxyBindingFactoryCreateFixture, CanCreateLoLaProxy) .WillByDefault(Return(make_FindServiceHandle(10U))); // When creating a proxy with that - const auto result = ProxyBindingFactory::Create(handle); + created_binding_ = ProxyBindingFactory::Create(handle); // Then no nullptr is returned - EXPECT_NE(result, nullptr); + EXPECT_NE(created_binding_, nullptr); } TEST_F(ProxyBindingFactoryCreateFixture, CannotCreateBlank) diff --git a/score/mw/com/impl/plumbing/proxy_event_field_binding_factory_test.cpp b/score/mw/com/impl/plumbing/proxy_event_field_binding_factory_test.cpp index 465ff432f..7e2cd4f3a 100644 --- a/score/mw/com/impl/plumbing/proxy_event_field_binding_factory_test.cpp +++ b/score/mw/com/impl/plumbing/proxy_event_field_binding_factory_test.cpp @@ -140,6 +140,20 @@ class ProxyServiceElementBindingFactoryParamaterisedFixture : public lola::Proxy ServiceElementTypes service_element_type_{GetParam()}; std::unique_ptr proxy_base_{nullptr}; DummyInstanceIdentifierBuilder dummy_instance_identifier_builder_{}; + + void TearDown() override + { + if (proxy_base_ != nullptr) + { + auto* const binding = ProxyBaseView{*proxy_base_}.GetBinding(); + if (binding != nullptr) + { + binding->PrepareUnsubscribe(); + binding->FinalizeUnsubscribe(); + } + } + lola::ProxyMockedMemoryFixture::TearDown(); + } }; INSTANTIATE_TEST_CASE_P(ProxyServiceElementBindingFactoryParamaterisedFixture, diff --git a/score/mw/com/impl/plumbing/proxy_method_binding_factory_test.cpp b/score/mw/com/impl/plumbing/proxy_method_binding_factory_test.cpp index 5abbf7cec..45cd6a052 100644 --- a/score/mw/com/impl/plumbing/proxy_method_binding_factory_test.cpp +++ b/score/mw/com/impl/plumbing/proxy_method_binding_factory_test.cpp @@ -105,6 +105,20 @@ class ProxyMethodFactoryFixture : public lola::ProxyMockedMemoryFixture return ProxyBaseView{*proxy_base_}.GetBinding(); } + void TearDown() override + { + if (proxy_base_ != nullptr) + { + auto* const binding = ProxyBaseView{*proxy_base_}.GetBinding(); + if (binding != nullptr) + { + binding->PrepareUnsubscribe(); + binding->FinalizeUnsubscribe(); + } + } + lola::ProxyMockedMemoryFixture::TearDown(); + } + private: std::unique_ptr proxy_base_{nullptr}; DummyInstanceIdentifierBuilder dummy_instance_identifier_builder_{}; diff --git a/score/mw/com/impl/proxy_base.cpp b/score/mw/com/impl/proxy_base.cpp index 26209a6b3..ab6d3f960 100644 --- a/score/mw/com/impl/proxy_base.cpp +++ b/score/mw/com/impl/proxy_base.cpp @@ -174,6 +174,26 @@ Result ProxyBase::SetupMethods() return {}; } +void ProxyBase::Unsubscribe() +{ + if (proxy_binding_ != nullptr) + { + proxy_binding_->PrepareUnsubscribe(); + } + for (auto& event : events_) + { + event.second.get().Unsubscribe(); + } + for (auto& field : fields_) + { + field.second.get().Unsubscribe(); + } + if (proxy_binding_ != nullptr) + { + proxy_binding_->FinalizeUnsubscribe(); + } +} + ProxyBaseView::ProxyBaseView(ProxyBase& proxy_base) noexcept : proxy_base_(proxy_base) {} ProxyBinding* ProxyBaseView::GetBinding() noexcept diff --git a/score/mw/com/impl/proxy_base.h b/score/mw/com/impl/proxy_base.h index 60e608832..57713cf11 100644 --- a/score/mw/com/impl/proxy_base.h +++ b/score/mw/com/impl/proxy_base.h @@ -131,6 +131,9 @@ class ProxyBase ProxyBase(ProxyBase&& other) noexcept; ProxyBase& operator=(ProxyBase&& other) noexcept; + /// \brief Unsubscribes all events and fields and tears down binding-level state. + void Unsubscribe(); + bool AreBindingsValid() const noexcept { const bool is_proxy_binding_valid{proxy_binding_ != nullptr}; diff --git a/score/mw/com/impl/proxy_binding.h b/score/mw/com/impl/proxy_binding.h index c24524d36..2c6fa9b48 100644 --- a/score/mw/com/impl/proxy_binding.h +++ b/score/mw/com/impl/proxy_binding.h @@ -62,6 +62,14 @@ class ProxyBinding virtual void UnregisterEventBinding(const std::string_view service_element_name) noexcept = 0; virtual Result SetupMethods() = 0; + + /// Called by ProxyBase::Unsubscribe before unsubscribing the service elements. Stops any asynchronous activity + /// in the binding so that no callback can race with the element teardown that follows. + virtual void PrepareUnsubscribe() = 0; + + /// Called by ProxyBase::Unsubscribe after the service elements have been unsubscribed. Clears any remaining + /// binding-internal registration state. + virtual void FinalizeUnsubscribe() = 0; }; } // namespace score::mw::com::impl diff --git a/score/mw/com/impl/proxy_binding_test.cpp b/score/mw/com/impl/proxy_binding_test.cpp index fd7ba4dd2..a1fbaf256 100644 --- a/score/mw/com/impl/proxy_binding_test.cpp +++ b/score/mw/com/impl/proxy_binding_test.cpp @@ -35,6 +35,8 @@ class MyProxy final : public ProxyBinding { return {}; } + void PrepareUnsubscribe() override {} + void FinalizeUnsubscribe() override {} }; TEST(ProxyBindingTest, ProxyBindingShouldNotBeCopyable) diff --git a/score/mw/com/impl/proxy_event_base.cpp b/score/mw/com/impl/proxy_event_base.cpp index 4e748c0d8..2d53c4b45 100644 --- a/score/mw/com/impl/proxy_event_base.cpp +++ b/score/mw/com/impl/proxy_event_base.cpp @@ -116,7 +116,12 @@ Result ProxyEventBase::Subscribe(const std::size_t max_sample_count) noexc { if (proxy_event_base_mock_ != nullptr) { - return proxy_event_base_mock_->Subscribe(max_sample_count); + auto mock_result = proxy_event_base_mock_->Subscribe(max_sample_count); + if (mock_result.has_value()) + { + is_subscribed_flag_.Set(); + } + return mock_result; } tracing::TraceSubscribe(tracing_data_, *binding_base_, max_sample_count); @@ -142,14 +147,30 @@ Result ProxyEventBase::Subscribe(const std::size_t max_sample_count) noexc return MakeUnexpected(ComErrc::kMaxSampleCountNotRealizable); } } + is_subscribed_flag_.Set(); return {}; } void ProxyEventBase::Unsubscribe() noexcept { + // Unsubscribe before a successful Subscribe is a silent no-op. + if (!is_subscribed_flag_.IsSet()) + { + return; + } + if (proxy_event_base_mock_ != nullptr) { proxy_event_base_mock_->Unsubscribe(); + is_subscribed_flag_.Clear(); + return; + } + + // binding_base_ is null when binding creation failed during ProxyWrapperClass::Create(). so we return early in this + // case, as there is no binding to unsubscribe from. + if (binding_base_ == nullptr) + { + is_subscribed_flag_.Clear(); return; } @@ -169,6 +190,7 @@ void ProxyEventBase::Unsubscribe() noexcept std::terminate(); } } + is_subscribed_flag_.Clear(); } std::size_t ProxyEventBase::GetFreeSampleCount() const noexcept diff --git a/score/mw/com/impl/proxy_event_base.h b/score/mw/com/impl/proxy_event_base.h index 1e8ccf594..449e9db17 100644 --- a/score/mw/com/impl/proxy_event_base.h +++ b/score/mw/com/impl/proxy_event_base.h @@ -15,6 +15,7 @@ #define SCORE_MW_COM_IMPL_PROXY_EVENT_BASE_H #include "score/mw/com/impl/event_receive_handler.h" +#include "score/mw/com/impl/flag_owner.h" #include "score/mw/com/impl/proxy_binding.h" #include "score/mw/com/impl/proxy_event_binding_base.h" #include "score/mw/com/impl/sample_reference_tracker.h" @@ -198,6 +199,8 @@ class ProxyEventBase IProxyEventBase* proxy_event_base_mock_; + FlagOwner is_subscribed_flag_{}; + private: /// \brief Expires the #receive_handler_scope_ in case not being called in the context of an EventReceiveHandler /// (because trying to expire the scope in which we are running, would lead to a deadlock) diff --git a/score/mw/com/impl/proxy_event_base_test.cpp b/score/mw/com/impl/proxy_event_base_test.cpp index 6ec683ec8..034bd3d4e 100644 --- a/score/mw/com/impl/proxy_event_base_test.cpp +++ b/score/mw/com/impl/proxy_event_base_test.cpp @@ -257,8 +257,9 @@ TYPED_TEST(ProxyEventBaseUnsubscribeFixture, CallingUnsubscribeWhileSubscribedCa this->RecordProperty("Priority", "1"); this->RecordProperty("DerivationTechnique", "Analysis of requirements"); - // Given a Service Element, that is connected to a mock binding + // Given a subscribed Service Element, that is connected to a mock binding this->CreateServiceElement(); + ProxyEventBaseAttorney{*this->service_element_}.MarkAsSubscribed(); // and that GetSubscriptionState will be called once and indicate that the Proxy is already subscribed. EXPECT_CALL(*this->mock_service_element_binding_, GetSubscriptionState()) @@ -280,8 +281,9 @@ TYPED_TEST(ProxyEventBaseUnsubscribeFixture, CallingUnsubscribeWhileSubscription this->RecordProperty("Priority", "1"); this->RecordProperty("DerivationTechnique", "Analysis of requirements"); - // Given a Service Element, that is connected to a mock binding + // Given a subscribed Service Element, that is connected to a mock binding this->CreateServiceElement(); + ProxyEventBaseAttorney{*this->service_element_}.MarkAsSubscribed(); // and that GetSubscriptionState will be called once and indicate that the Proxy subscription is pending. EXPECT_CALL(*this->mock_service_element_binding_, GetSubscriptionState()) @@ -305,10 +307,6 @@ TYPED_TEST(ProxyEventBaseUnsubscribeFixture, CallingUnsubscribeWhileNotSubscribe // Given a Service Element, that is connected to a mock binding this->CreateServiceElement(); - // and that GetSubscriptionState will be called once and indicate that the Proxy is not subscribed. - EXPECT_CALL(*this->mock_service_element_binding_, GetSubscriptionState()) - .WillOnce(Return(SubscriptionState::kNotSubscribed)); - // Expecting that the Unsubscribe will not be called on the binding EXPECT_CALL(*this->mock_service_element_binding_, Unsubscribe()).Times(0); @@ -316,13 +314,41 @@ TYPED_TEST(ProxyEventBaseUnsubscribeFixture, CallingUnsubscribeWhileNotSubscribe this->service_element_->Unsubscribe(); } +TYPED_TEST(ProxyEventBaseUnsubscribeFixture, CallingUnsubscribeTwiceAfterSubscribeOnlyDispatchesToBindingOnce) +{ + this->RecordProperty("Verifies", "SCR-21286218"); + this->RecordProperty("Description", + "Checks that a second Unsubscribe is a silent no-op after the first one cleared the flag"); + this->RecordProperty("TestType", "Requirements-based test"); + this->RecordProperty("Priority", "1"); + this->RecordProperty("DerivationTechnique", "Analysis of requirements"); + + // Given a subscribed Service Element, that is connected to a mock binding + this->CreateServiceElement(); + ProxyEventBaseAttorney{*this->service_element_}.MarkAsSubscribed(); + + // and that GetSubscriptionState will be called exactly once during the first Unsubscribe and indicate that + // the Proxy is already subscribed. The second Unsubscribe must short-circuit on the flag and never reach + // the binding. + EXPECT_CALL(*this->mock_service_element_binding_, GetSubscriptionState()) + .WillOnce(Return(SubscriptionState::kSubscribed)); + + // Expecting that the Unsubscribe will be called on the binding exactly once across both calls + EXPECT_CALL(*this->mock_service_element_binding_, Unsubscribe()).Times(1); + + // When Unsubscribe is called twice on the Service Element + this->service_element_->Unsubscribe(); + this->service_element_->Unsubscribe(); +} + TYPED_TEST(ProxyEventBaseUnsubscribeDeathTest, CallingUnsubscribeWhileASampleIsStillReferencedTerminates) { const auto CallUnsubscribeWhileSampleIsStillAllocated = [this]() { - // Given a Service Element, that is connected to a mock binding + // Given a subscribed Service Element, that is connected to a mock binding this->CreateServiceElement(); + ProxyEventBaseAttorney{*this->service_element_}.MarkAsSubscribed(); - // Expecting that GetSubscriptionState will be called once and indicate that the Proxy is not subscribed. + // Expecting that GetSubscriptionState will be called once and indicate that the Proxy is subscribed. EXPECT_CALL(*this->mock_service_element_binding_, GetSubscriptionState()) .WillOnce(Return(SubscriptionState::kSubscribed)); diff --git a/score/mw/com/impl/proxy_event_test.cpp b/score/mw/com/impl/proxy_event_test.cpp index d6b24f899..31656e0df 100644 --- a/score/mw/com/impl/proxy_event_test.cpp +++ b/score/mw/com/impl/proxy_event_test.cpp @@ -283,8 +283,6 @@ TYPED_TEST(ProxyEventFixture, UnsubscribeWhileNotHoldingSamplePtrs) using Base = ProxyEventFixture; Base::mock_proxy_event_.PushFakeSample(3U); - EXPECT_CALL(Base::mock_proxy_event_, GetSubscriptionState()) - .WillOnce(::testing::Return(SubscriptionState::kNotSubscribed)); Base::proxy_event_.Unsubscribe(); // Then nothing bad happens diff --git a/score/mw/com/impl/test/proxy_resources.h b/score/mw/com/impl/test/proxy_resources.h index f33ff549d..92fe6831c 100644 --- a/score/mw/com/impl/test/proxy_resources.h +++ b/score/mw/com/impl/test/proxy_resources.h @@ -70,6 +70,11 @@ class ProxyEventBaseAttorney return proxy_event_base_.tracing_data_; } + void MarkAsSubscribed() noexcept + { + proxy_event_base_.is_subscribed_flag_.Set(); + } + private: ProxyEventBase& proxy_event_base_; }; diff --git a/score/mw/com/impl/tracing/test/proxy_event_tracing_test.cpp b/score/mw/com/impl/tracing/test/proxy_event_tracing_test.cpp index 6549d0700..7cf731ba1 100644 --- a/score/mw/com/impl/tracing/test/proxy_event_tracing_test.cpp +++ b/score/mw/com/impl/tracing/test/proxy_event_tracing_test.cpp @@ -751,6 +751,9 @@ TYPED_TEST(ProxyEventTracingUnsubscribeFixture, UnsubscribeCallsAreTracedWhenEna // When a Proxy containing a ProxyEvent is created based on a lola deployment this->CreateProxy(); + // and the event is in the subscribed state + ProxyEventBaseAttorney{this->proxy_->my_service_element_}.MarkAsSubscribed(); + // and unsubscribe is called on the event this->proxy_->my_service_element_.Unsubscribe(); } @@ -812,6 +815,9 @@ TYPED_TEST(ProxyEventTracingUnsubscribeFixture, // When a Proxy containing a ProxyEvent is created based on a lola deployment this->CreateProxy(); + // and the event is in the subscribed state + ProxyEventBaseAttorney{this->proxy_->my_service_element_}.MarkAsSubscribed(); + // and unsubscribe is called on the event this->proxy_->my_service_element_.Unsubscribe(); @@ -881,6 +887,9 @@ TYPED_TEST(ProxyEventTracingUnsubscribeFixture, // When a Proxy containing a ProxyEvent is created based on a lola deployment this->CreateProxy(); + // and the event is in the subscribed state + ProxyEventBaseAttorney{this->proxy_->my_service_element_}.MarkAsSubscribed(); + // and unsubscribe is called on the event this->proxy_->my_service_element_.Unsubscribe(); @@ -929,6 +938,9 @@ TYPED_TEST(ProxyEventTracingUnsubscribeFixture, UnsubscribeCallsAreNotTracedWhen // When a Proxy containing a ProxyEvent is created based on a lola deployment this->CreateProxy(); + // and the event is in the subscribed state + ProxyEventBaseAttorney{this->proxy_->my_service_element_}.MarkAsSubscribed(); + // and unsubscribe is called on the event this->proxy_->my_service_element_.Unsubscribe(); } diff --git a/score/mw/com/impl/traits.h b/score/mw/com/impl/traits.h index 1c51a0587..4c351d7db 100644 --- a/score/mw/com/impl/traits.h +++ b/score/mw/com/impl/traits.h @@ -358,6 +358,38 @@ class ProxyWrapperClass : public Interface return proxy_wrapper; } + ~ProxyWrapperClass() + { + if (is_proxy_owner_.IsSet()) + { + this->Unsubscribe(); + } + } + + ProxyWrapperClass(const ProxyWrapperClass&) = delete; + ProxyWrapperClass& operator=(const ProxyWrapperClass&) = delete; + + ProxyWrapperClass(ProxyWrapperClass&& other) noexcept + : Interface{std::move(static_cast&&>(other))}, + is_proxy_owner_{std::move(other.is_proxy_owner_)} + { + } + + ProxyWrapperClass& operator=(ProxyWrapperClass&& other) noexcept + { + if (&other != this) + { + if (is_proxy_owner_.IsSet()) + { + this->Unsubscribe(); + } + + Interface::operator=(std::move(static_cast&&>(other))); + is_proxy_owner_ = std::move(other.is_proxy_owner_); + } + return *this; + } + private: /// \brief Constructs ProxyWrapperClass explicit ProxyWrapperClass(HandleType instance_handle, std::unique_ptr proxy_binding) @@ -379,6 +411,12 @@ class ProxyWrapperClass : public Interface } static std::optional>>> creation_results_; + + /// \brief Flag which is checked before calling Unsubscribe in the destructor of this class + /// + /// This flag is always set for a Proxy except when a Proxy is moved. In this case, this flag will be cleared + /// in the moved-from class so that that object doesn't call Unsubscribe on destruction. + FlagOwner is_proxy_owner_{true}; }; template