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

Deferred connection evaluation #41

Closed
wants to merge 9 commits into from
2 changes: 2 additions & 0 deletions src/kdbindings/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ set(HEADERS
property.h
property_updater.h
signal.h
connection_evaluator.h
connection_handle.h
utils.h
)

Expand Down
81 changes: 81 additions & 0 deletions src/kdbindings/connection_evaluator.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
#pragma once

#include <functional>
#include <list>
#include <mutex>

#include <kdbindings/connection_handle.h>

namespace KDBindings {

/**
* @brief Manages and evaluates deferred Signal connections.
*
* The ConnectionEvaluator class is responsible for managing and evaluating connections
* to Signals. It provides mechanisms to delay and control the evaluation of connections.
* It therefore allows controlling when and on which thread slots connected to a Signal are executed.
*
* @see Signal::connectDeferred()
*/
class ConnectionEvaluator
phyBrackets marked this conversation as resolved.
Show resolved Hide resolved
{

public:
ConnectionEvaluator() = default;

// ConnectionEvaluator is not copyable, as it is designed to manage connections,
// and copying it could lead to unexpected behavior, including duplication of connections and issues
// related to connection lifetimes. Therefore, it is intentionally made non-copyable.
ConnectionEvaluator(const ConnectionEvaluator &) noexcept = delete;

ConnectionEvaluator &operator=(const ConnectionEvaluator &) noexcept = delete;

// ConnectionEvaluators are not moveable, as they are captures by-reference
// by the Signal, so moving them would lead to a dangling reference.
ConnectionEvaluator(ConnectionEvaluator &&other) noexcept = delete;
phyBrackets marked this conversation as resolved.
Show resolved Hide resolved

ConnectionEvaluator &operator=(ConnectionEvaluator &&other) noexcept = delete;

/**
* @brief Evaluate the deferred connections.
*
* This function is responsible for evaluating and executing deferred connections.
* And this function ensures thread safety
*/
void evaluateDeferredConnections()
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

{
std::lock_guard<std::mutex> lock(m_connectionsMutex);

for (auto &pair : m_deferredConnections) {
pair.second();
}
m_deferredConnections.clear();
}

private:
template<typename...>
friend class Signal;

void enqueueSlotInvocation(const ConnectionHandle &handle, const std::function<void()> &connection)
phyBrackets marked this conversation as resolved.
Show resolved Hide resolved
{
m_deferredConnections.push_back({ handle, std::move(connection) });
}

void dequeueSlotInvocation(const ConnectionHandle &handle)
{
std::lock_guard<std::mutex> lock(m_connectionsMutex);

auto handleMatches = [&handle](const auto &invocationPair) {
return invocationPair.first == handle;
};

// Remove all invocations that match the handle
m_deferredConnections.erase(
std::remove_if(m_deferredConnections.begin(), m_deferredConnections.end(), handleMatches),
m_deferredConnections.end());
}

std::vector<std::pair<ConnectionHandle, std::function<void()>>> m_deferredConnections;
std::mutex m_connectionsMutex;
};
} // namespace KDBindings
213 changes: 213 additions & 0 deletions src/kdbindings/connection_handle.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,213 @@
/*
This file is part of KDBindings.

SPDX-FileCopyrightText: 2021-2023 Klarälvdalens Datakonsult AB, a KDAB Group company <[email protected]>
Author: Sean Harmer <[email protected]>

SPDX-License-Identifier: MIT

Contact KDAB at <[email protected]> for commercial licensing options.
*/

#include <kdbindings/genindex_array.h>
#include <kdbindings/utils.h>

namespace KDBindings {

template<typename... Args>
class Signal;

class ConnectionHandle;

namespace Private {
//
// This class defines a virtual interface, that the Signal this ConnectionHandle refers
// to must implement.
// It allows ConnectionHandle to refer to this non-template class, which then dispatches
// to the template implementation using virtual function calls.
// It allows ConnectionHandle to be a non-template class.
class SignalImplBase : public std::enable_shared_from_this<SignalImplBase>
{
public:
SignalImplBase() = default;

virtual ~SignalImplBase() = default;

virtual void disconnect(const GenerationalIndex &id, const ConnectionHandle &handle) = 0;
virtual bool blockConnection(const GenerationalIndex &id, bool blocked) = 0;
virtual bool isConnectionActive(const GenerationalIndex &id) const = 0;
virtual bool isConnectionBlocked(const GenerationalIndex &id) const = 0;
};

} // namespace Private

/**
* @brief A ConnectionHandle represents the connection of a Signal
* to a slot (i.e. a function that is called when the Signal is emitted).
*
* It is returned from a Signal when a connection is created and used to
* manage the connection by disconnecting, (un)blocking it and checking its state.
**/
class ConnectionHandle
{
public:
/**
* A ConnectionHandle can be default constructed.
* In this case the ConnectionHandle will not reference any active connection (i.e. isActive() will return false),
* and not belong to any Signal.
**/
ConnectionHandle() = default;

/**
* A ConnectionHandle can be copied.
**/
ConnectionHandle(const ConnectionHandle &) = default;
ConnectionHandle &operator=(const ConnectionHandle &) = default;

/**
* A ConnectionHandle can be moved.
**/
ConnectionHandle(ConnectionHandle &&) = default;
ConnectionHandle &operator=(ConnectionHandle &&) = default;

/**
* Disconnect the slot.
*
* When this function is called, the function that was passed to Signal::connect
* to create this ConnectionHandle will no longer be called when the Signal is emitted.
*
* If the ConnectionHandle is not active or the connection has already been disconnected,
* nothing happens.
*
* After this call, the ConnectionHandle will be inactive (i.e. isActive() returns false)
* and will no longer belong to any Signal (i.e. belongsTo returns false).
**/
void disconnect()
{
if (m_id.has_value()) {
if (auto shared_impl = checkedLock()) {
shared_impl->disconnect(*m_id, *this);
}
}

// ConnectionHandle is no longer active;
m_signalImpl.reset();
}

/**
* Check whether the connection of this ConnectionHandle is active.
*
* @return true if the ConnectionHandle refers to an active Signal
* and the connection was not disconnected previously, false otherwise.
**/
bool isActive() const
{
return static_cast<bool>(checkedLock());
}

/**
* Sets the block state of the connection.
* If a connection is blocked, emitting the Signal will no longer call this
* connections slot, until the connection is unblocked.
*
* Behaves the same as calling Signal::blockConnection with this
* ConnectionHandle as argument.
*
* To temporarily block a connection, consider using an instance of ConnectionBlocker,
* which offers a RAII-style implementation that makes sure the connection is always
* returned to its original state.
*
* @param blocked The new blocked state of the connection.
* @return whether the connection was previously blocked.
* @throw std::out_of_range Throws if the connection is not active (i.e. isActive() returns false).
**/
bool block(bool blocked)
{
if (m_id.has_value()) {
if (auto shared_impl = checkedLock()) {
return shared_impl->blockConnection(*m_id, blocked);
}
}
throw std::out_of_range("Cannot block a non-active connection!");
}

/**
* Checks whether the connection is currently blocked.
*
* To change the blocked state of a connection, call ConnectionHandle::block.
*
* @return whether the connection is currently blocked.
**/
bool isBlocked() const
{
if (m_id.has_value())
if (auto shared_impl = checkedLock()) {
return shared_impl->isConnectionBlocked(*m_id);
}
throw std::out_of_range("Cannot check whether a non-active connection is blocked!");
}

/**
* Check whether this ConnectionHandle belongs to the given Signal.
*
* @return true if this ConnectionHandle refers to a connection within the given Signal
**/
template<typename... Args>
bool belongsTo(const Signal<Args...> &signal) const
{
auto shared_impl = m_signalImpl.lock();
return shared_impl && shared_impl == std::static_pointer_cast<Private::SignalImplBase>(signal.m_impl);
}

// Define an operator== function to compare ConnectionHandle objects.
bool operator==(const ConnectionHandle &other) const
{
auto thisSignalImpl = m_signalImpl.lock();
auto otherSignalImpl = other.m_signalImpl.lock();

// If both signalImpl pointers are valid, compare them along with the IDs.
if (thisSignalImpl && otherSignalImpl) {
return (thisSignalImpl == otherSignalImpl) && (m_id == other.m_id);
}

// If neither instance has an ID, and both signalImpl pointers are invalid, consider them equal.
if (!m_id.has_value() && !other.m_id.has_value() && !thisSignalImpl && !otherSignalImpl) {
return true;
}

// In all other cases, they are not equal.
return false;
}

private:
template<typename...>
friend class Signal;

std::weak_ptr<Private::SignalImplBase> m_signalImpl;
std::optional<Private::GenerationalIndex> m_id;

// private, so it is only available from Signal
ConnectionHandle(std::weak_ptr<Private::SignalImplBase> signalImpl, std::optional<Private::GenerationalIndex> id)
: m_signalImpl{ std::move(signalImpl) }, m_id{ std::move(id) }
{
}
void setId(const Private::GenerationalIndex &id)
{
m_id = id;
}

// Checks that the weak_ptr can be locked and that the connection is
// still active
std::shared_ptr<Private::SignalImplBase> checkedLock() const
{
auto shared_impl = m_signalImpl.lock();
if (m_id.has_value()) {
if (shared_impl && shared_impl->isConnectionActive(*m_id)) {
return shared_impl;
}
}
return nullptr;
}
};

} // namespace KDBindings
5 changes: 5 additions & 0 deletions src/kdbindings/genindex_array.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@ namespace Private {
struct GenerationalIndex {
uint32_t index = 0;
uint32_t generation = 0;

bool operator==(const GenerationalIndex& rhs) const {
// Include both index and generation in the equality check
return index == rhs.index && generation == rhs.generation;
}
};

class GenerationalIndexAllocator
Expand Down
Loading