Deferred connection evaluation #41

phyBrackets · 2023-09-18T01:48:04Z

Implementing #23

LeonMatthesKDAB

The overall approach is absolutely fine 😊

However, there are many details that I think still need work.
I especially have concerns regarding the correct behavior of disconnecting, as well as possible double-evaluation.

src/kdbindings/signal.h

src/kdbindings/connection_evaluator.h

src/kdbindings/signal.h

tests/signal/tst_signal.cpp

LeonMatthesKDAB · 2023-09-20T10:03:37Z

src/kdbindings/connection_evaluator.h

+     * interference with reentrant emissions. Finally, it iterates through the copied
+     * connections and executes each one by calling the associated function object.
+     */
+    void evaluateDeferredConnections()


Overall I think the approach of simply copying (or moving out of) the connections is quite cool, as it allows to quickly release the mutex again.

However, this approach conflicts with deleting connections.
When the following happens asynchronously

emit a deferred signal

disconnect the connection

evaluateDeferredConnections

The connection should either be evaluated before the disconnect call returns, or it should not be evaluated, if the disconnect call returns before evaluateDeferredConnections is called. As it's highly likely that the connection includes dangling pointers after the call to disconnect the connection.

So if an evaluation is in progress the call to disconnect cannot return until the evaluation has completely finished.

If you still want to keep the approach of simply moving the connections over to release the mutex early, you could add a second mutex for "disconnect", which would be held longer, whilst the mutex for adding new connections could already be released.
However, this double-locking may be less performant in the end, depending on how many connections are queued, as we now need to lock two mutexes when evaluating and when disconnecting.

I really have a doubt about this, how i am thinking about is, so follow the order
-> emit a deferred signal
-> disconnect the connection
-> evaluateDeferredConnections

disconnecting the signal after the emit or before the evaluation wouldn't be have any issue, as evaluation depends on the list of connections in the evaluator which has already queued up through the call to emit. Call to disconnect removes the connection from the m_connections of the Signal class, which is fine.

I can only think about the issue when evaluation or disconnect either be used in mulithreading context. Or i might be thinking wrong?

I am adding the test case for this, to look up.

Like we discussed with @seanharmer and @lemirep , both approaches are feasible.
Removing queued invocations on disconnect is really a lot better to protect from dangling references.
But as Sean noted, it might just be something that needs to be expected with a new paradigm.

So if you're not sure what to do yet, feel free to explore your initial implementation a bit further (i.e. still evaluating queued invocations even after disconnect).
I'd just like to see the implications of this in use in an actual application.
Basically: How easy is it to foot-gun yourself with it?

phyBrackets · 2023-09-20T17:18:14Z

Thanks for the review @LeonMatthesKDAB , I have updated the patch.

LeonMatthesKDAB

Already much improved.

We still need to decide on what to do regarding disconnects.
I can definitely see both sides here. Maybe best to pull in more opinions here.
It's definitely possible for users to work around the dangling pointer issues with shared_ptr and friends, but might be annoying to deal with.

src/kdbindings/signal.h

LeonMatthesKDAB · 2023-09-22T14:37:36Z

tests/signal/tst_signal.cpp

+
+        signal.emit(4);
+
+        connection.disconnect();


The reason I believe it is dangerous to have the previously emitted connection still be evaluated is:

Imagine that val was a int * allocated with int* val = new int;

I would expect this to then work:

connection.disconnect(); delete val;

Then calling evaluateDeferredConnections would still reference val, as it's part of the connected closure.
This access would then be a dangling pointer and most likely SegFault, or write to random memory.

That's why I'm concerned about this behavior, you simply cannot guarantee that if you emitted a signal that references some resources those resources aren't referenced anymore after disconnecting.
Though that's exactly what disconnect is meant for in a non-deferred scenario.
The current behavior needs additional synchronization to ensure that both disconnect and evaluateDeferredConnections are called before any resources are freed.
Especially as those two calls are likely to happen in completely different threads the synchronization might be very difficult.

On the other hand I also get that it may be unintuitive that a signal emission doesn't actually execute a slot if it's not evaluated before it's disconnected.
Maybe @seanharmer or @lemirep have a preference on this, as they're using KDBindings quite extensively afaik.

Thanks for the explanation! That does make sense. Now assuming there is no trick to extend the lifetime of closure object, seems like we have to safely synchronize it, or basically make sure that evaluation happen before the call to disconnect.

I understand your point here, but I am concerned about how we used to understand signal, slot emit and disconnect. My intution while looking at code that contains structure like connectedDeferred -> emit -> disconnect -> evaluateDeferred , I would like to see it as the classic implementation, means if signal is emitted before disconnection, it should be evaluated , no matter if the call to evaluation is after disconnecting the signal. But sadly that does have the problem with dangling reference. I would also love to see , views from @seanharmer and @lemirep

On the other hand I also get that it may be unintuitive that a signal emission doesn't actually execute a slot if it's not
evaluated before it's disconnected.

tests/signal/tst_signal.cpp

src/kdbindings/connection_evaluator.h

src/kdbindings/signal.h

…entations

LeonMatthesKDAB

Unfortunately there are still some lifetime issues here.
Most notably, the ConnectionHandle of a deferred connection may currently not be moved or deconstructed.

Furthermore, only a connection to a single evaluator is possible, which I think is too restrictive.

We can still explore the route of not dequeuing on disconnect, if you think it's worth it.

src/kdbindings/signal.h

src/kdbindings/connection_evaluator.h

src/kdbindings/signal.h

LeonMatthesKDAB · 2023-12-07T13:11:46Z

src/kdbindings/connection_evaluator.h

+     * interference with reentrant emissions. Finally, it iterates through the copied
+     * connections and executes each one by calling the associated function object.
+     */
+    void evaluateDeferredConnections()


Like we discussed with @seanharmer and @lemirep , both approaches are feasible.
Removing queued invocations on disconnect is really a lot better to protect from dangling references.
But as Sean noted, it might just be something that needs to be expected with a new paradigm.

So if you're not sure what to do yet, feel free to explore your initial implementation a bit further (i.e. still evaluating queued invocations even after disconnect).
I'd just like to see the implications of this in use in an actual application.
Basically: How easy is it to foot-gun yourself with it?

src/kdbindings/connection_evaluator.h

LeonMatthesKDAB

Still some issues remaining, though we're getting closer.

I've noticed a pretty problematic edge-case though (didn't think of this myself until just now either).
When disconnecting a ConnectionHandle after the corresponding Signal has been deconstructed, the ConnectionHandle has no chance of finding the corresponding evaluator.
Therefore, the evaluations cannot be dequeued as shown by this test:

SUBCASE("Disconnect deferred connection from deleted signal")
    {
        auto signal = new Signal<>();
        auto evaluator = std::make_shared<ConnectionEvaluator>();
        bool called = false;

        auto handle = signal->connectDeferred(evaluator, [&called](const auto &) { called = true; });
        signal->emit();
        delete signal;

        handle.disconnect();
        REQUIRE(called == false);
        evaluator->evaluateDeferredConnections();
        REQUIRE(called == false);
    }

Possible solutions:

Add a weak_ptr<ConnectionEvaluator> to ConnectionHandle -- adds quite a lot of data to an otherwise small Handle though
Make the weak_ptr in ConnectionHandle a shared_ptr - breaking change as it affects isActive and may lead to quite a lot of dead signals remaining in memory
Simply dequeue all connections when a signal is destructed? - somewhat strange semantically, but easiest solution - would like to experiment with this.

src/kdbindings/connection_evaluator.h

LeonMatthesKDAB · 2023-12-15T13:07:44Z

src/kdbindings/signal.h

@@ -242,14 +259,13 @@ class Signal
     * The `KDBindings::Signal` class itself is not thread-safe. While the `ConnectionEvaluator` is inherently
     * thread-safe, ensure that any concurrent access to this Signal is protected externally to maintain thread safety.
     */
-    ConnectionHandle connectDeferred(const std::shared_ptr<ConnectionEvaluator> &evaluator, std::function<void(Args...)> const &slot)
+    ConnectionHandle connectDeferred(const std::shared_ptr<ConnectionEvaluator> &evaluator, std::function<void(const ConnectionHandle &, Args...)> const &slot)


The fact that the lambda used by connectDeferred is passed a ConnectionHandle is an implementation detail and should not be visible to the user.

LeonMatthesKDAB · 2023-12-15T13:09:01Z

tests/signal/tst_signal.cpp

-        auto connection1 = signal1.connectDeferred(evaluator, [&val](int value) {
-            val += value;
-        });
+    auto connection1 = signal1.connectDeferred(evaluator, [&val](const ConnectionHandle& handle, int value) {


Incorrect indentation here.

LeonMatthesKDAB · 2023-12-15T13:11:55Z

src/kdbindings/signal.h

@@ -178,11 +194,12 @@ class Signal
        friend class Signal;
        struct Connection {
            std::function<void(Args...)> slot;
-            bool isDeferred{ false };
+            std::function<void(const ConnectionHandle &, Args...)> slotDeferred;


Ideally this would be a union, as adding a second std::function is quite expensive (sizeof(std::function) ~~ 32 byte), but I guess it's good enough for now.

LeonMatthesKDAB · 2023-12-15T13:13:41Z

src/kdbindings/signal.h

@@ -153,8 +162,7 @@ class Signal
            }
        }

-        // Calls all connected functions
-        void emit(Args... p) const
+        void emit(Args... p) /*const*/


Why can't this stay const? shared_from_this has a const version, doesn't it?

LeonMatthesKDAB · 2023-12-15T13:20:26Z

src/kdbindings/signal.h

        }

        // Disconnects a previously connected function
-        void disconnect(const Private::GenerationalIndex &id) override
+        void disconnect(const Private::GenerationalIndex &id, const ConnectionHandle &handle) override


Either pass in the handle and get the id out of it, or only pass the id and construct the handle on-the-fly, there's no point in having both as parameters.
The handle already contains the id...

LeonMatthesKDAB · 2023-12-15T13:22:43Z

src/kdbindings/signal.h

        struct Connection {
            std::function<void(Args...)> slot;
+            std::function<void(const ConnectionHandle &, Args...)> slotDeferred;
+            std::weak_ptr<ConnectionEvaluator> m_connectionEvaluator; // Use shared_ptr for stronger ownership.


What is the shared_ptr comment about?
Is it meant as a TODO?

I can't see any reason why we'd want to use shared_ptr. If the connection evaluator is already deleted, we don't need to disconnect from it anymore, and we don't want to queue new invocations into it either, they can't be evaluated anymore anyhow.

LeonMatthesKDAB · 2023-12-18T13:18:35Z

Rebased onto main in #48 .

Deferred connection evaluation

9e3b2d2

phyBrackets requested review from seanharmer and LeonMatthesKDAB September 18, 2023 01:48

LeonMatthesKDAB requested changes Sep 20, 2023

View reviewed changes

phyBrackets and others added 2 commits September 20, 2023 22:30

handle the evaluator object lifetime and added tests

4981326

update connection_evaluator.h

bdc4e01

LeonMatthesKDAB reviewed Sep 22, 2023

View reviewed changes

test and documentation update

a86884e

phyBrackets force-pushed the connection_evaluator branch from 22b384b to a86884e Compare September 25, 2023 04:04

MiKom self-requested a review September 27, 2023 12:45

MiKom requested changes Sep 27, 2023

View reviewed changes

src/kdbindings/connection_evaluator.h Outdated Show resolved Hide resolved

src/kdbindings/connection_evaluator.h Outdated Show resolved Hide resolved

MiKom reviewed Sep 27, 2023

View reviewed changes

src/kdbindings/connection_evaluator.h Outdated Show resolved Hide resolved

src/kdbindings/connection_evaluator.h Outdated Show resolved Hide resolved

src/kdbindings/signal.h Outdated Show resolved Hide resolved

phyBrackets and others added 3 commits October 23, 2023 00:18

implementing dequeueSlotInvocation on disconnect and update the docum…

31d3fbc

…entations

Update connection_evaluator.h

d16b483

Adding the connection_handle file

ae55479

LeonMatthesKDAB requested changes Dec 7, 2023

View reviewed changes

change the deferred slot signature and manage connectionHandle correctly

a5512d3

LeonMatthesKDAB reviewed Dec 15, 2023

View reviewed changes

LeonMatthesKDAB mentioned this pull request Dec 15, 2023

Add logging when dequeuing a deferred emission #47

Open

deque on deconstruct and nits

fcdccb5

LeonMatthesKDAB closed this Dec 18, 2023

LeonMatthesKDAB deleted the connection_evaluator branch January 19, 2024 13:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deferred connection evaluation #41

Deferred connection evaluation #41

phyBrackets commented Sep 18, 2023

LeonMatthesKDAB left a comment

LeonMatthesKDAB Sep 20, 2023

phyBrackets Sep 20, 2023

LeonMatthesKDAB Dec 7, 2023

phyBrackets commented Sep 20, 2023

LeonMatthesKDAB left a comment

LeonMatthesKDAB Sep 22, 2023

phyBrackets Sep 24, 2023

LeonMatthesKDAB left a comment

LeonMatthesKDAB Dec 7, 2023

LeonMatthesKDAB left a comment

LeonMatthesKDAB Dec 15, 2023

LeonMatthesKDAB Dec 15, 2023

LeonMatthesKDAB Dec 15, 2023

LeonMatthesKDAB Dec 15, 2023

LeonMatthesKDAB Dec 15, 2023

LeonMatthesKDAB Dec 15, 2023

LeonMatthesKDAB commented Dec 18, 2023

Deferred connection evaluation #41

Deferred connection evaluation #41

Conversation

phyBrackets commented Sep 18, 2023

LeonMatthesKDAB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phyBrackets commented Sep 20, 2023

LeonMatthesKDAB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeonMatthesKDAB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeonMatthesKDAB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeonMatthesKDAB commented Dec 18, 2023