Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Connect reflective: Allow slots to access their own ConnectionHandle #70

Merged
merged 6 commits into from
Jul 3, 2024

Conversation

LeonMatthesKDAB
Copy link
Contributor

Follow up to #68

This version now reuses the mechanism from connectDeferred that already created the appropriate ConnectionHandle.

Open Question: Can we make this compatible with connectDeferred somehow in a thread-safe manner?
This would be quite useful for single-shot signals across threads.

@phyBrackets
Copy link
Contributor

Hi @LeonMatthesKDAB , Thanks for the follow up. If I understand your point correctly then we basically wants something this to be possible, right?

    Signal<int> sig;
    auto evaluator = std::make_shared<ConnectionEvaluator>();
    bool disconnected = false;

    auto slot = [&](ConnectionHandle &handle, int value) {
        if (value > 10) {
            handle.disconnect();
            disconnected = true;
        }
    };

    auto handle = sig.connectDeferredReflective(evaluator, slot);
    sig.emit(5);  // Should not trigger the slot yet
    evaluator->evaluateDeferredConnections();  // Now it evaluates

    assert(!disconnected);  // Slot not yet disconnected
    sig.emit(15);
    evaluator->evaluateDeferredConnections();  // Evaluate with value 15

    assert(disconnected);  // Slot should now be disconnected

@LeonMatthesKDAB
Copy link
Contributor Author

@phyBrackets Yes, I think we don't need to implement this in this PR already, but that would be great to add later.

Right now the problem is that the ConnectionEvaluator would usually live on another thread, so accessing the ConnectionHandle from that thread is problematic, as ConnectionHandles are not thread-safe...
Having the ability to add a single-shot slot to a ConnectionEvaluator sounds useful though.

On the other hand, maybe it's easier if we just allow you to queue a lambda onto the ConnectionEvaluator directly, without having to go through a signal/slot connection...

So something like this:

// Thread 1
evaluator->queue([](){
    std::cout << "Hello World!" << std::endl;
})

// Thread 2
evaluator->evaluatedDeferredConnections(); // This would print "Hello World!"

Unsure what's needed here exactly, maybe @seanharmer or @MiKom can give some insights into the needs from a user perspective.

@phyBrackets
Copy link
Contributor

phyBrackets commented May 26, 2024

@LeonMatthesKDAB After cudgeling my brain a bit, there is one more idea to allow safe cross-thread operations on connections here if we want to go through a signal/slot connection is that,

Instead of allowing direct access to ConnectionHandle from another thread, provide a way to queue commands (like disconnect or block) that ConnectionEvaluator can execute in a thread-safe manner.

So we need a way to implement a command queue in ConnectionEvaluator where actions on connections can be enqueued, such as disconnecting or blocking a connection. These actions are then processed in a thread-safe manner when the evaluator runs.

// Example
Signal<int> sig;
auto evaluator = std::make_shared<ConnectionEvaluator>();
auto handle = sig.connect([](int x) { std::cout << "Received " << x << std::endl; });

sig.disconnectInEvaluator(evaluator, handle); // this method would be responsible for enqueuing the disconnect command in the evaluator in a thread safe manner

// Process commands on the evaluator thread
evaluator->processCommands(); // finally processing commands in thread safe manner

Not sure as well that what would be best here as per the user perspective, but surely we can give a try to both approaches, what do you think?

@LeonMatthesKDAB
Copy link
Contributor Author

Instead of allowing direct access to ConnectionHandle from another thread, provide a way to queue commands (like disconnect or block) that ConnectionEvaluator can execute in a thread-safe manner.

So we need a way to implement a command queue in ConnectionEvaluator where actions on connections can be enqueued, such as disconnecting or blocking a connection. These actions are then processed in a thread-safe manner when the evaluator runs.

Not sure as well that what would be best here as per the user perspective, but surely we can give a try to both approaches, what do you think?

Hm, sounds interesting.

What would be somewhat different to the way we use this right now is that currently the ConnectionEvaluator is evaluated from the secondary thread, which does not own the signal.
Although, as we've seen with KDUtils, there may well be a ConnectionEvaluator for the primary thread as well, which does own the signal.

I like the idea of making the ConnectionEvaluator a more general "command queue", as that would be quite useful in general to transmit messages/events between threads.

Then the API may just look something like:

Signal<int> sig;
auto evaluator = std::make_shared<ConnectionEvaluator>();
auto handle = sig.connect([](int x) { std::cout << "Received " << x << std::endl; });

auto thread = std::thread::spawn([evaluator,handle](){
    // queue the disconnect from another thread.
    evaluator->queue([handle](){ handle.disconnect(); });
});
thread.join();

// Process commands on the main thread
evaluator->processCommands(); // finally processing commands in thread safe manner

Which could then be used to queue whatever you want on any threads event loop.
I think that sounds useful for KDUtils as well, what do you think @seanharmer @MiKom ?

Then we should just rename the ConnectionEvaluator to something more generic...
Maybe CommandQueue? EventQueue? Suggestions welcome 😊

Suddenly I'm very glad all of this is still behind a "beta" release flag :)

@LeonMatthesKDAB LeonMatthesKDAB force-pushed the connect_reflective branch 2 times, most recently from 93d1e7f to 75e6753 Compare June 14, 2024 07:30
MiKom
MiKom previously approved these changes Jul 3, 2024
Copy link
Member

@MiKom MiKom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from a formatting nit, it's a go

phyBrackets and others added 6 commits July 3, 2024 15:53
@LeonMatthesKDAB LeonMatthesKDAB merged commit 4bb82b0 into KDAB:main Jul 3, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants