mw/com: Make ProxyBase::AreBindingsValid symmetrical with Skeleton side

score/mw/com/design/skeleton_proxy/README.md

@@ -92,6 +92,12 @@ The following sequence shows the instantiation of a service class up to its serv
 
 <img alt="SKELETON_CREATE_OFFER_SEQ" src="https://www.plantuml.com/plantuml/proxy?src=https://raw.githubusercontent.com/eclipse-score/communication/refs/heads/main/score/mw/com/design/skeleton_proxy/skeleton_create_offer_seq.puml">
 
+On construction, the generated skeleton (`DummySkeleton`) checks that all the bindings of its contained service elements
+(events/fields/methods) are valid. This is done by calling the `AreBindingsValid()` on `SkeletonBase`. This will then 
+iterate over the maps of references to its service elements described [below](#binding-independent-level-registration-of-skeleton-eventsfields-at-their-parent-skeleton) 
+and check if each binding was successfully created. If this is not the case, the construction of the skeleton instance 
+returns an error.
+
 #### Binding independent level Registration of skeleton events/fields at their parent skeleton
 
 Due to our architectural constraints, the `impl::SkeletonBase` (the base class of the generated skeleton/`DummySkeleton`)
@@ -168,54 +174,30 @@ On `ProxyBase` level we have two types of methods:
   a generated proxy instance, to detect, whether the instance can be successfully returned from
   `static Result<generated proxy class> <generated proxy class>::Create()` or not.
 
-The implementation of the latter one contains some indirection &ndash; again due to our architectural constraints, which
-are the same at proxy and skeleton side and have been laid out
-[here on the skeleton side](#binding-independent-level-registration-of-skeleton-eventsfields-at-their-parent-skeleton).
-
-The indirection is that for `ProxyBase::AreBindingsValid()` to work, the binding independent `impl::ProxyEvent`s set
-the member of its semantic parent `ProxyBase::are_service_element_bindings_valid_` to `false` during their construction,
-if they detect, that they don't have a valid underlying `pImpl` member of type `ProxyEventBindingBase` to dispatch to.
-This is an indication, that the binding specific implementation of the `ProxyEvent` couldn't be successfully created by
-the proxy side factories.
-
-So, the binding independent `impl::ProxyBase` doesn't "know" its semantically aggregated `ProxyEvent`s.
-Although the `ProxyEvent`s get a reference to their parent `ProxyBase` during construction (and therefore "know" their
-parent in the context of their `ctor`), they don't store this reference (as it isn't really needed currently and
-would require additional logic to update this reference if the parent `impl::ProxyBase` gets moved).
-Instead, they only set once (see above) the `ProxyBase::are_service_element_bindings_valid_` member variable of their
-parent during construction as this is currently the only feedback needed between proxy and its proxy events on binding
-independent level.
+Similarly to the skeleton side, the `impl::SkeletonBase` doesn't "know" its event/field/method children. Therefore,
+the `impl::ProxyEventBase` registers itself with the `impl::ProxyBase` in the same way as `impl::SkeletonEventBase`
+(as explained
+[here on the skeleton side](#binding-independent-level-registration-of-skeleton-eventsfields-at-their-parent-skeleton). 
 
 #### Binding level Registration of proxy events/fields at their parent proxy
 
 On the binding **specific** level things look different!
 I.e. the binding specific proxy needs to interact with its dependent/child proxy events of type `ProxyEventBindingBase`
 (at least the `LoLa`/shared-memory specific does, so we introduced it on the `ProxyBinding` interface level) .
 
-Therefore, `impl::ProxyBinding` has a method `RegisterEventBinding` (and `UnregisterEventBinding`) and our `LoLa`
-binding specific proxy `lola::Proxy`, which implements `impl::ProxyBinding`, has a member `event_bindings_` (a map
-containing its child proxy events), which it populates in its implementation of `RegisterEventBinding` with its
-dependent proxy events.
-
-The call to `RegisterEventBinding` happens during creation of a binding independent `impl::ProxyEvent` via an `RAII`
-style member `event_binding_registration_guard_` of type `EventBindingRegistrationGuard`, which the `impl::ProxyEvent`
-owns.
-This `EventBindingRegistrationGuard` takes over two functionalities:
-
-- setting `ProxyBase::are_service_element_bindings_valid_` member of its parent ProxyBase instance (see previous chapter)
-- calling `RegisterEventBinding` on the underlying `ProxyBinding` of the given `ProxyBase` on construction of the
-  `EventBindingRegistrationGuard` and calling `UnregisterEventBinding` on destruction. Since
-  `event_binding_registration_guard_` is owned by the `impl::ProxyEvent`, the registration / unregistration is done on
-  construction / destruction of an `impl::ProxyEvent`.
+Therefore, when each `lola::ProxyEvent` gets created (as a member of the generated proxy class), it registers itself 
+at its parent `lola::Proxy` via `lola::Proxy::RegisterEvent()`. The `lola::Proxy` stores the reference to each 
+child `lola::ProxyEvent`s in a map which it can later use.
 
 So **after** construction of user facing generated proxy class instance (`DummyProxy`), we have the following structure
 in place:
 
 1. Only an instance of the generated proxy class gets returned from the call to `<DummyProxy>::Create()` in case its
    binding on proxy level (its `pImpl` target) implementing `ProxyBinding` could be constructed and also for **all** its
-   aggregated events/fields their related `ProxyEventBinding`s (`pImpl` targets) could be constructed.
+   aggregated events/fields their related `ProxyEventBinding`s (`pImpl` targets) could be constructed. If this is not 
+   the case, then an error will be retrurned from `<DummyProxy>::Create()`.
 2. The `ProxyBinding` (`ProxyBase::proxy_binding_`) has complete access to all its child events/fields as
-   `ProxyBinding::RegisterEventBinding` has been called for all contained events/fields.
+   `lola::Proxy::RegisterEvent()` has been called for all contained events/fields.
 
 
 #### Extract type agnostic code

score/mw/com/impl/bindings/lola/generic_proxy_event.cpp

@@ -31,6 +31,7 @@ GenericProxyEvent::GenericProxyEvent(Proxy& parent, const ElementFqId element_fq
       proxy_event_common_{parent, element_fq_id, event_name},
       meta_info_{parent.GetEventMetaInfo(element_fq_id)}
 {
+    parent.RegisterEvent(event_name, *this);
 }
 
 Result<void> GenericProxyEvent::Subscribe(const std::size_t max_sample_count) noexcept

score/mw/com/impl/bindings/lola/proxy.cpp

@@ -639,32 +639,6 @@ bool Proxy::IsEventProvided(const std::string_view event_name) const noexcept
     return event_exists;
 }
 
-void Proxy::RegisterEventBinding(const std::string_view service_element_name,
-                                 ProxyEventBindingBase& proxy_event_binding) noexcept
-{
-    // 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_};
-    const auto insert_result = event_bindings_.emplace(service_element_name, proxy_event_binding);
-    SCORE_LANGUAGE_FUTURECPP_ASSERT_PRD_MESSAGE(insert_result.second,
-                                                "Failed to insert proxy event binding into event binding map.");
-    proxy_event_binding.NotifyServiceInstanceChangedAvailability(is_service_instance_available_, GetSourcePid());
-}
-
-void Proxy::UnregisterEventBinding(const std::string_view service_element_name) noexcept
-{
-    // 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_};
-    const auto number_of_elements_removed = event_bindings_.erase(service_element_name);
-    if (number_of_elements_removed == 0U)
-    {
-        score::mw::log::LogWarn("lola") << "UnregisterEventBinding that was never registered. Ignoring.";
-    }
-}
-
 score::Result<void> Proxy::SetupMethods()
 {
     auto enabled_method_data = GetMethodIdAndQueueSizeForEnabledMethods();
@@ -921,6 +895,16 @@ pid_t Proxy::GetSourcePid() const noexcept
     return service_data_storage.skeleton_pid_;
 }
 
+void Proxy::RegisterEvent(const std::string_view service_element_name,
+                          ProxyEventBindingBase& proxy_event_binding) noexcept
+{
+    std::lock_guard lock{proxy_event_registration_mutex_};
+    const auto insert_result = event_bindings_.emplace(service_element_name, proxy_event_binding);
+    SCORE_LANGUAGE_FUTURECPP_ASSERT_PRD_MESSAGE(insert_result.second,
+                                                "Failed to insert proxy event binding into event binding map.");
+    proxy_event_binding.NotifyServiceInstanceChangedAvailability(is_service_instance_available_, GetSourcePid());
+}
+
 void Proxy::RegisterMethod(const UniqueMethodIdentifier method_id, ProxyMethod& proxy_method) noexcept
 {
     std::lock_guard lock{proxy_method_registration_mutex_};

score/mw/com/impl/bindings/lola/proxy.h

@@ -167,24 +167,6 @@ class Proxy : public ProxyBinding
     /// \return True if the event name exists, otherwise, false
     bool IsEventProvided(const std::string_view event_name) const noexcept override;
 
-    /// \brief Adds a reference to a Proxy service element binding to an internal map
-    ///
-    /// Will insert the provided ProxyEventBindingBase& into a map stored within the class which will be used to call
-    /// NotifyServiceInstanceChangedAvailability on all saved Proxy service elements by the FindServiceHandler of
-    /// find_service_guard_. It will then call NotifyServiceInstanceChangedAvailability on the provided
-    /// ProxyEventBindingBase. Since this function first locks proxy_event_registration_mutex_, it is ensured that the
-    /// provided Proxy service element will be notified synchronously about the availability of the provider and will
-    /// then be notified of any future changes via the callback, without missing any notifications.
-    void RegisterEventBinding(const std::string_view service_element_name,
-                              ProxyEventBindingBase& proxy_event_binding) noexcept override;
-
-    /// \brief Removes the reference to a Proxy service element binding from an internal map
-    ///
-    /// This must be called by a Proxy service element before destructing to ensure that the FindService handler in
-    /// find_service_guard_ does not call NotifyServiceInstanceChangedAvailability on a Proxy service element after it's
-    /// been destructed.
-    void UnregisterEventBinding(const std::string_view service_element_name) noexcept override;
-
     score::Result<void> SetupMethods() override;
 
     QualityType GetQualityType() const noexcept;
@@ -198,6 +180,8 @@ class Proxy : public ProxyBinding
         return proxy_instance_identifier_;
     }
 
+    void RegisterEvent(const std::string_view service_element_name,
+                       ProxyEventBindingBase& proxy_event_binding) noexcept;
     void RegisterMethod(const UniqueMethodIdentifier method_id, ProxyMethod& proxy_method) noexcept;
 
   private:
@@ -231,7 +215,7 @@ class Proxy : public ProxyBinding
     HandleType handle_;
     std::unordered_map<std::string_view, std::reference_wrapper<ProxyEventBindingBase>> event_bindings_;
 
-    /// Mutex which synchronises registration of Proxy service elements via Proxy::RegisterEventBinding with the
+    /// Mutex which synchronises registration of Proxy service elements via Proxy::RegisterEvent with the
     /// FindServiceHandler in find_service_guard_ which will call NotifyServiceInstanceChangedAvailability on all
     /// currently registered Proxy service elements.
     std::mutex proxy_event_registration_mutex_;

score/mw/com/impl/bindings/lola/proxy_event.h

@@ -66,6 +66,7 @@ class ProxyEvent final : public ProxyEventBinding<SampleType>
           proxy_event_common_{parent, element_fq_id, event_name},
           samples_{parent.GetEventDataStorage<SampleType>(element_fq_id)}
     {
+        parent.RegisterEvent(event_name, *this);
     }
 
     ProxyEvent(const ProxyEvent&) = delete;

score/mw/com/impl/bindings/lola/proxy_event_test.cpp

@@ -32,6 +32,21 @@
 
 namespace score::mw::com::impl::lola
 {
+
+class ProxyTestAttorney
+{
+  public:
+    explicit ProxyTestAttorney(Proxy& proxy) : proxy_{proxy} {}
+
+    const std::unordered_map<std::string_view, std::reference_wrapper<ProxyEventBindingBase>>& GetEvents() const
+    {
+        return proxy_.event_bindings_;
+    }
+
+  private:
+    Proxy& proxy_;
+};
+
 namespace
 {
 
@@ -152,6 +167,10 @@ class LolaProxyEventFixture : public LolaProxyEventResources
 using MyTypes = ::testing::Types<ProxyEventStruct, GenericProxyEventStruct>;
 TYPED_TEST_SUITE(LolaProxyEventFixture, MyTypes, );
 
+template <typename T>
+using LolaProxyEventConstructionFixture = LolaProxyEventFixture<T>;
+TYPED_TEST_SUITE(LolaProxyEventConstructionFixture, MyTypes, );
+
 template <typename T>
 using LolaProxyEventGetNewSamplesFixture = LolaProxyEventFixture<T>;
 TYPED_TEST_SUITE(LolaProxyEventGetNewSamplesFixture, MyTypes, );
@@ -164,6 +183,22 @@ template <typename T>
 using LolaProxyEventDeathTest = LolaProxyEventFixture<T>;
 TYPED_TEST_SUITE(LolaProxyEventDeathTest, MyTypes, );
 
+TYPED_TEST(LolaProxyEventConstructionFixture, ConstructingRegistersEventWithParent)
+{
+    // Given a proxy with an events map which is initially empty
+    auto& events_map = ProxyTestAttorney(*this->proxy_).GetEvents();
+    ASSERT_EQ(events_map.size(), 0U);
+
+    // When constructing a ProxyEvent
+    this->GivenAProxyEvent(this->element_fq_id_, this->event_name_);
+
+    // Then the event should have been registered in the parent Proxy's map of events
+    EXPECT_EQ(events_map.size(), 1U);
+    auto registered_event_it = events_map.find(this->event_name_);
+    EXPECT_NE(registered_event_it, events_map.end());
+    EXPECT_EQ(&registered_event_it->second.get(), &(*this->test_proxy_event_));
+}
+
 TYPED_TEST(LolaProxyEventGetNewSamplesFixture, CallsReceiverForEachAccessibleSample)
 {
     this->RecordProperty("Verifies", "SCR-14035773, SCR-21350367, SCR-6225206");

score/mw/com/impl/bindings/lola/proxy_test.cpp

@@ -427,14 +427,14 @@ TEST_F(ProxyAutoReconnectFixture, WhenStopFindServiceReturnsErrorOnProxyDestruct
     // Then the program does not terminate
 }
 
-TEST_F(ProxyAutoReconnectFixture, RegisterEventBindingCallsNotifyOnEventWithFalseWhenProviderInitiallyDoesNotExist)
+TEST_F(ProxyAutoReconnectFixture, RegisterEventCallsNotifyOnEventWithFalseWhenProviderInitiallyDoesNotExist)
 {
     const auto valid_find_service_handle = make_FindServiceHandle(10U);
     mock_binding::ProxyEvent<std::uint8_t> proxy_event{};
 
     // Expecting that StartFindService is called but the handler is not called since the provider does not exist
-    ON_CALL(service_discovery_mock_, StartFindService(_, EnrichedInstanceIdentifier{identifier_}))
-        .WillByDefault(Return(valid_find_service_handle));
+    EXPECT_CALL(service_discovery_mock_, StartFindService(_, EnrichedInstanceIdentifier{identifier_}))
+        .WillOnce(Return(valid_find_service_handle));
 
     // Then expecting that NotifyServiceInstanceChangedAvailability is called on the event with is_available false
     const bool is_available{false};
@@ -445,10 +445,10 @@ TEST_F(ProxyAutoReconnectFixture, RegisterEventBindingCallsNotifyOnEventWithFals
     EXPECT_NE(proxy_, nullptr);
 
     // and the ProxyEvent registers itself with the Proxy
-    proxy_->RegisterEventBinding("Event0", proxy_event);
+    proxy_->RegisterEvent("Event0", proxy_event);
 }
 
-TEST_F(ProxyAutoReconnectFixture, RegisterEventBindingCallsNotifyOnEventWithTrueWhenProviderInitiallyExists)
+TEST_F(ProxyAutoReconnectFixture, RegisterEventCallsNotifyOnEventWithTrueWhenProviderInitiallyExists)
 {
     const auto valid_find_service_handle = make_FindServiceHandle(10U);
     mock_binding::ProxyEvent<std::uint8_t> proxy_event{};
@@ -470,10 +470,10 @@ TEST_F(ProxyAutoReconnectFixture, RegisterEventBindingCallsNotifyOnEventWithTrue
     EXPECT_NE(proxy_, nullptr);
 
     // and the ProxyEvent registers itself with the Proxy
-    proxy_->RegisterEventBinding("Event0", proxy_event);
+    proxy_->RegisterEvent("Event0", proxy_event);
 }
 
-TEST_F(ProxyAutoReconnectFixture, RegisterEventBindingCallsNotifyOnEventWithLatestValueFromFindServiceHandler)
+TEST_F(ProxyAutoReconnectFixture, RegisterEventCallsNotifyOnEventWithLatestValueFromFindServiceHandler)
 {
     const auto valid_find_service_handle = make_FindServiceHandle(10U);
     mock_binding::ProxyEvent<std::uint8_t> proxy_event_0{};
@@ -523,14 +523,14 @@ TEST_F(ProxyAutoReconnectFixture, RegisterEventBindingCallsNotifyOnEventWithLate
     EXPECT_NE(proxy_, nullptr);
 
     // and the first ProxyEvent registers itself with the Proxy
-    proxy_->RegisterEventBinding("Event0", proxy_event_0);
+    proxy_->RegisterEvent("Event0", proxy_event_0);
 
     // And then the FindService handler is called with an empty service handle container
     ServiceHandleContainer<HandleType> empty_service_handle_container{};
     saved_find_service_handler(empty_service_handle_container, valid_find_service_handle);
 
     // and the second ProxyEvent registers itself with the Proxy
-    proxy_->RegisterEventBinding("Event1", proxy_event_1);
+    proxy_->RegisterEvent("Event1", proxy_event_1);
 
     // And then the FindService handler is called again with a non-empty service handle container
     ServiceHandleContainer<HandleType> filled_service_handle_container{make_HandleType(identifier_)};
@@ -562,8 +562,8 @@ TEST_F(ProxyEventBindingFixture, RegisteringEventBindingWillCallNotifyServiceIns
     // Expecting that NotifyServiceInstanceChangedAvailability will be called on the binding
     EXPECT_CALL(mock_proxy_event_base_binding, NotifyServiceInstanceChangedAvailability(_, _));
 
-    // When calling RegisterEventBinding
-    proxy_->RegisterEventBinding(kDummyEventName, mock_proxy_event_base_binding);
+    // When calling RegisterEvent
+    proxy_->RegisterEvent(kDummyEventName, mock_proxy_event_base_binding);
 }
 
 TEST_F(ProxyEventBindingFixture,
@@ -582,47 +582,14 @@ TEST_F(ProxyEventBindingFixture,
     InitialiseProxyWithCreate(identifier_);
 
     // and that two bindings are registered
-    proxy_->RegisterEventBinding(kDummyEventName, mock_proxy_event_base_binding);
-    proxy_->RegisterEventBinding("some_other_event", mock_proxy_event_base_binding_2);
-
-    // When the find service handler is called
-    const auto find_service_handler = find_service_handler_promise_.get_future().get();
-    find_service_handler(ServiceHandleContainer<HandleType>{}, kDummyFindServiceHandle);
-}
-
-TEST_F(ProxyEventBindingFixture,
-       UnregisteringEventBindingAfterRegisteringWillMakeBindingUnavailableToServiceAvailabilityChangeHandler)
-{
-    mock_binding::ProxyEventBase mock_proxy_event_base_binding{};
-
-    // Expecting that NotifyServiceInstanceChangedAvailability will only be called on the binding once when it's
-    // registered and not again when the find service handler is called
-    EXPECT_CALL(mock_proxy_event_base_binding, NotifyServiceInstanceChangedAvailability(_, _));
-
-    // Given a constructed Proxy which registered an event binding
-    WhichCapturesFindServiceHandler(identifier_);
-    InitialiseProxyWithCreate(identifier_);
-    proxy_->RegisterEventBinding(kDummyEventName, mock_proxy_event_base_binding);
-
-    // and then unregistered the event binding
-    proxy_->UnregisterEventBinding(kDummyEventName);
+    proxy_->RegisterEvent(kDummyEventName, mock_proxy_event_base_binding);
+    proxy_->RegisterEvent("some_other_event", mock_proxy_event_base_binding_2);
 
     // When the find service handler is called
     const auto find_service_handler = find_service_handler_promise_.get_future().get();
     find_service_handler(ServiceHandleContainer<HandleType>{}, kDummyFindServiceHandle);
 }
 
-TEST_F(ProxyEventBindingFixture, UnregisteringEventBindingBeforeRegisteringWillNotTerminate)
-{
-    // Given a constructed Proxy
-    InitialiseProxyWithCreate(identifier_);
-
-    // When calling UnregisterEventBinding when RegisterEventBinding was never called
-    proxy_->UnregisterEventBinding(kDummyEventName);
-
-    // Then we don't terminate
-}
-
 using ProxyGetEventMetaInfoFixture = ProxyMockedMemoryFixture;
 TEST_F(ProxyGetEventMetaInfoFixture, GetEventMetaInfoWillReturnDataForEventThatWasCreatedBySkeleton)
 {