Fix flaky test by KrishaDeshkool · Pull Request #385 · eclipse-score/communication

KrishaDeshkool · 2026-05-05T13:57:37Z

Issue: #386

bemerybmw · 2026-05-18T07:48:25Z

    // 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_};
+    if (event_bindings_.size() == 0U)


We already do this check below with a warning log. Why do we do it again here?

ah! this would be needed if we still had the ��EventBindingRegistrationGuard around,
after ShutdownBinding() clears event_bindings_, each EventBindingRegistrationGuard dtor still fires and calls UnregisterEventBinding. without this early-return, the LogWarn below would spam once per event/field on every clean shutdown.

now that EventBindingRegistrationGuard would be removed, i can remove this as well.

bemerybmw · 2026-05-18T07:58:47Z

        return proxy_wrapper;
    }

+    ~ProxyWrapperClass()


This could actually be a scope exit so that ProxyWrapperClass can have defaulted destructor / move ops. We use ScopeExit in a few places, you can check some existing uses.

Could you do the same thing for SkeletonWrapperClass?

hmm.. okay i think we cannot use ScopeExit here,
why?
if we want to call the Unsubscribe inside the callback we pass in to scope exit,
something like ScopeExit guard_{[this]() { this->Unsubscribe(); }};
Then we have to capture "this",
then when we move the wrapper this stored inside the lambda would be invalidated and it crashes.
i think this approach worked in other places where we do not capture to this but we capture something that class in which this scopeexit object lives are captured.

bemerybmw · 2026-05-18T08:05:26Z

        return;
    }

+    if (binding_base_ == nullptr)


This is required in case the binding was not created so the ProxyWrapperClass returns an error? Could you add a comment?

bemerybmw · 2026-05-18T08:16:00Z

    virtual Result<void> SetupMethods() = 0;
+
+    /// Stops any asynchronous activity in the binding
+    virtual void PrepareUnsubscribe() noexcept = 0;


New functions should not be noexcept.

bemerybmw · 2026-05-18T08:19:02Z


    virtual Result<void> SetupMethods() = 0;
+
+    /// Stops any asynchronous activity in the binding


These are functions implemented by every binding so we should be clear about what should be done in one function vs the other. I think that they should be PrepareUnsubscribe and FinalizeUnsubscribe. PrepareUnsubscribe will always be called before destroying the service elements and FinalizeUnsubscribe will be called after destroying the service elements. We previously had something like that on skeleton side but we removed it. Maybe we should reintroduce it for symmetry? What do you think @crimson11?

They need to be properly documented.

bemerybmw · 2026-05-18T09:44:54Z

+        return std::move(proxy_result).value();
+    }
+
+    GeneratedProxyUnsubscribeRaiiFixture& GivenAProxy()


Why do we need CreateProxy in addition to this function?

ah right! this is no longer required since i do not have the move tests which had createProxy to create a second proxy, will remove now.

bemerybmw · 2026-05-18T09:45:40Z

+};
+
+using GeneratedProxyDestructionFixture = GeneratedProxyUnsubscribeRaiiFixture;
+TEST_F(GeneratedProxyDestructionFixture, CallsUnsubscribeOnDestructionOfOwnedProxy)


Why OwnedProxy?

leftover from an earlier change where i had one test that compared owned vs moved-from proxies in the same body i split it into separate tests (DestroyingMovedToProxyCallsUnsubscribe, DestroyingMovedFromProxyDoesNotCallUnsubscribe), the "owned" here is redundant. will rename to CallsUnsubscribeOnDestruction.

bemerybmw · 2026-05-18T09:48:17Z

+}
+
+using GeneratedProxyMoveConstructionFixture = GeneratedProxyUnsubscribeRaiiFixture;
+TEST_F(GeneratedProxyMoveConstructionFixture, MoveConstructingDoesNotCallUnsubscribe)


When you change ProxyWrapperClass to use ScopeExit then you can also remove the move tests (assuming that you add in a test like this one: https://github.com/KrishaDeshkool/communication/blob/ed71f9b487ae18305253dfb8367c0fe4590b3b9b/score/mw/com/impl/bindings/lola/messaging/method_subscription_registration_guard_test.cpp#L60)

leaving it as is since scope exit does not work here (atleast not without lot of rework)

bemerybmw · 2026-05-18T09:49:39Z

This commit should be squashed with the previous one since tests should be merged alongside the changes.

both commits would be merged at once anyway since this is one PR,
but if you are fine with all the things in one commit then i will do it like this,
usually ppl ask for separate commit for tests in the past. since you are reviewing it i will squash as you asked.

bemerybmw · 2026-05-18T09:50:20Z

    EXPECT_CALL(proxy_event_mock_2, Unsubscribe());
-    EXPECT_CALL(proxy_field_mock, Unsubscribe());
+    // Times(2): once from the explicit call below, once from the RAII destructor in ~ProxyWrapperClass
+    EXPECT_CALL(proxy_field_mock, Unsubscribe()).Times(2);


We currently have nothing stopping Unsubscribe from being called multiple times or even from being called before subscribe: https://github.com/KrishaDeshkool/communication/blob/ed71f9b487ae18305253dfb8367c0fe4590b3b9b/score/mw/com/impl/proxy_event_base.cpp#L148. We have a requirement that says that in the latter case, nothing should happen (

communication/score/mw/com/dependability/requirements/component_requirements/component_requirements_ipc.trlc

Line 2107 in 56c36d4

/* broken_link_c/issue/21286218 */

). We now will also call Unsubscribe in the destructor of a ProxyWrapperClass which would recall unsubscribe on an event if already called. I think that we should have a flag in ProxyEventBase that is set when subscribed and unset when unsubscribed like we do on skeleton side. (https://github.com/KrishaDeshkool/communication/blob/ed71f9b487ae18305253dfb8367c0fe4590b3b9b/score/mw/com/impl/proxy_event_base.cpp#L148)

I guess that we should do similar checks on skeleton side / in all events / fields as described in point 3 of #385 (comment)

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).

KrishaDeshkool force-pushed the fix_flaky_test branch 2 times, most recently from 268298b to 7e9975a Compare May 5, 2026 13:58

KrishaDeshkool marked this pull request as ready for review May 5, 2026 14:25

KrishaDeshkool requested review from LittleHuba, bemerybmw, castler, crimson11, hoe-jo and limdor as code owners May 5, 2026 14:25

KrishaDeshkool force-pushed the fix_flaky_test branch 2 times, most recently from e3f2cdc to 15f4629 Compare May 11, 2026 15:17

KrishaDeshkool marked this pull request as draft May 12, 2026 08:12

KrishaDeshkool force-pushed the fix_flaky_test branch 2 times, most recently from 5201764 to 96a9ed1 Compare May 12, 2026 14:28

KrishaDeshkool marked this pull request as ready for review May 12, 2026 15:18

KrishaDeshkool marked this pull request as draft May 13, 2026 08:46

KrishaDeshkool force-pushed the fix_flaky_test branch 4 times, most recently from 445b759 to 20a0d63 Compare May 13, 2026 09:13

KrishaDeshkool marked this pull request as ready for review May 13, 2026 09:15

bemerybmw requested changes May 18, 2026

View reviewed changes

KrishaDeshkool force-pushed the fix_flaky_test branch from 20a0d63 to fb8e64a Compare May 19, 2026 09:27


		virtual Result<void> SetupMethods() = 0;

		/// Stops any asynchronous activity in the binding

Conversation

KrishaDeshkool commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bemerybmw May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

KrishaDeshkool commented May 5, 2026 •

edited

Loading

bemerybmw May 18, 2026 •

edited

Loading