From: Manuel Nickschas Date: Wed, 30 May 2018 20:01:50 +0000 (+0200) Subject: common: Make InternalPeer (more) thread-safe X-Git-Tag: travis-deploy-test~68 X-Git-Url: https://git.quassel-irc.org/?p=quassel.git;a=commitdiff_plain;h=047f7a5c2429d44adf0102ee25c7fb254f80ea69 common: Make InternalPeer (more) thread-safe The two internal peers used by a mono client live in different threads. Previously, they each stored a pointer to their counterpart to be able to send protocol messages wrapped in an event. While this has never been observed in the wild, this may cause subtle lifetime issues if one of the sides goes away before the other. While currently, as the two peers are at least created in the same thread (and thus, setting the mutual pointer should not cause issues), this will change in the near future, since the mono core is slated to move into its own thread. Rather than trying to deal with this ourselves, let Qt do the work. Instead of storing a pointer and sending events, use signal/slot connections to transfer the messages between the peers. Qt will ensure that this happens in a thread-safe manner, and that connections are killed if one of the peers is destroyed. --- diff --git a/src/common/internalpeer.cpp b/src/common/internalpeer.cpp index 103db47b..4180b76b 100644 --- a/src/common/internalpeer.cpp +++ b/src/common/internalpeer.cpp @@ -18,40 +18,31 @@ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. * ***************************************************************************/ -#include -#include - #include "internalpeer.h" using namespace Protocol; -template -class PeerMessageEvent : public QEvent -{ -public: - PeerMessageEvent(InternalPeer *sender, InternalPeer::EventType eventType, const T &message) - : QEvent(QEvent::Type(eventType)), sender(sender), message(message) - {} - - InternalPeer *sender; - T message; -}; - - InternalPeer::InternalPeer(QObject *parent) - : Peer(0, parent), - _proxy(0), - _peer(0), - _isOpen(true) + : Peer(nullptr, parent) { + static bool registered = []() { + qRegisterMetaType(); + qRegisterMetaType(); + qRegisterMetaType(); + qRegisterMetaType(); + return true; + }(); + Q_UNUSED(registered) + setFeatures(Quassel::Features{}); } InternalPeer::~InternalPeer() { - if (_isOpen) + if (_isOpen) { emit disconnected(); + } } @@ -60,16 +51,19 @@ QString InternalPeer::description() const return tr("internal connection"); } + QString InternalPeer::address() const { return tr("internal connection"); } + quint16 InternalPeer::port() const { return 0; } + bool InternalPeer::isOpen() const { return true; @@ -111,7 +105,7 @@ int InternalPeer::lag() const void InternalPeer::setSignalProxy(::SignalProxy *proxy) { if (!proxy && _proxy) { - _proxy = 0; + _proxy = nullptr; if (_isOpen) { _isOpen = false; emit disconnected(); @@ -130,19 +124,20 @@ void InternalPeer::setSignalProxy(::SignalProxy *proxy) void InternalPeer::setPeer(InternalPeer *peer) { - if (_peer) { - qWarning() << Q_FUNC_INFO << "Peer already set, ignoring!"; - return; - } - _peer = peer; + connect(peer, SIGNAL(dispatchMessage(Protocol::SyncMessage)), SLOT(handleMessage(Protocol::SyncMessage))); + connect(peer, SIGNAL(dispatchMessage(Protocol::RpcCall)) , SLOT(handleMessage(Protocol::RpcCall))); + connect(peer, SIGNAL(dispatchMessage(Protocol::InitRequest)), SLOT(handleMessage(Protocol::InitRequest))); + connect(peer, SIGNAL(dispatchMessage(Protocol::InitData)) , SLOT(handleMessage(Protocol::InitData))); + connect(peer, SIGNAL(disconnected()), SLOT(peerDisconnected())); + + _isOpen = true; } void InternalPeer::peerDisconnected() { - disconnect(_peer, 0, this, 0); - _peer = 0; + disconnect(sender(), nullptr, this, nullptr); if (_isOpen) { _isOpen = false; emit disconnected(); @@ -152,81 +147,63 @@ void InternalPeer::peerDisconnected() void InternalPeer::dispatch(const SyncMessage &msg) { - dispatch(SyncMessageEvent, msg); + emit dispatchMessage(msg); } void InternalPeer::dispatch(const RpcCall &msg) { - dispatch(RpcCallEvent, msg); + emit dispatchMessage(msg); } void InternalPeer::dispatch(const InitRequest &msg) { - dispatch(InitRequestEvent, msg); + emit dispatchMessage(msg); } void InternalPeer::dispatch(const InitData &msg) { - dispatch(InitDataEvent, msg); + emit dispatchMessage(msg); } -namespace { - -void setSourcePeer(Peer* peer) +void InternalPeer::handleMessage(const Protocol::SyncMessage &msg) { - auto p = SignalProxy::current(); - if (p) - p->setSourcePeer(peer); + handle(msg); } -} // anon - -template -void InternalPeer::dispatch(EventType eventType, const T &msg) +void InternalPeer::handleMessage(const Protocol::RpcCall &msg) { - if (!_peer) { - qWarning() << Q_FUNC_INFO << "Cannot dispatch a message without a peer!"; - return; - } + handle(msg); +} + - // The peers always live in different threads, so use an event for thread-safety - QCoreApplication::postEvent(_peer, new PeerMessageEvent(this, eventType, msg)); +void InternalPeer::handleMessage(const Protocol::InitRequest &msg) +{ + handle(msg); } -void InternalPeer::customEvent(QEvent *event) +void InternalPeer::handleMessage(const Protocol::InitData &msg) { - setSourcePeer(this); + handle(msg); +} - switch ((int)event->type()) { - case SyncMessageEvent: { - handle(static_cast *>(event)->message); - break; - } - case RpcCallEvent: { - handle(static_cast *>(event)->message); - break; - } - case InitRequestEvent: { - handle(static_cast *>(event)->message); - break; - } - case InitDataEvent: { - handle(static_cast *>(event)->message); - break; - } - default: - qWarning() << Q_FUNC_INFO << "Received unknown custom event:" << event->type(); - setSourcePeer(nullptr); - return; - } +template +void InternalPeer::handle(const T &msg) +{ + static auto setSourcePeer = [](Peer *peer) { + auto p = SignalProxy::current(); + if (p) { + p->setSourcePeer(peer); + } + }; + setSourcePeer(this); + Peer::handle(msg); setSourcePeer(nullptr); - event->accept(); } diff --git a/src/common/internalpeer.h b/src/common/internalpeer.h index 55cfeae6..c00319b7 100644 --- a/src/common/internalpeer.h +++ b/src/common/internalpeer.h @@ -18,83 +18,79 @@ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. * ***************************************************************************/ -#ifndef INTERNALPEER_H -#define INTERNALPEER_H +#pragma once + +#include #include "peer.h" #include "protocol.h" #include "signalproxy.h" -class QEvent; - class InternalPeer : public Peer { Q_OBJECT public: - enum EventType { - SyncMessageEvent = QEvent::User, - RpcCallEvent, - InitRequestEvent, - InitDataEvent - }; - - InternalPeer(QObject *parent = 0); - virtual ~InternalPeer(); + InternalPeer(QObject *parent = nullptr); + ~InternalPeer() override; - Protocol::Type protocol() const { return Protocol::InternalProtocol; } - QString description() const; + Protocol::Type protocol() const override { return Protocol::InternalProtocol; } + QString description() const override; - virtual QString address() const; - virtual quint16 port() const; + QString address() const override; + quint16 port() const override; - SignalProxy *signalProxy() const; - void setSignalProxy(SignalProxy *proxy); + SignalProxy *signalProxy() const override; + void setSignalProxy(SignalProxy *proxy) override; InternalPeer *peer() const; void setPeer(InternalPeer *peer); - bool isOpen() const; - bool isSecure() const; - bool isLocal() const; + bool isOpen() const override; + bool isSecure() const override; + bool isLocal() const override; - int lag() const; + int lag() const override; - void dispatch(const Protocol::SyncMessage &msg); - void dispatch(const Protocol::RpcCall &msg); - void dispatch(const Protocol::InitRequest &msg); - void dispatch(const Protocol::InitData &msg); + void dispatch(const Protocol::SyncMessage &msg) override; + void dispatch(const Protocol::RpcCall &msg) override; + void dispatch(const Protocol::InitRequest &msg) override; + void dispatch(const Protocol::InitData &msg) override; /* These are not needed for InternalPeer */ - void dispatch(const Protocol::RegisterClient &) {} - void dispatch(const Protocol::ClientDenied &) {} - void dispatch(const Protocol::ClientRegistered &) {} - void dispatch(const Protocol::SetupData &) {} - void dispatch(const Protocol::SetupFailed &) {} - void dispatch(const Protocol::SetupDone &) {} - void dispatch(const Protocol::Login &) {} - void dispatch(const Protocol::LoginFailed &) {} - void dispatch(const Protocol::LoginSuccess &) {} - void dispatch(const Protocol::SessionState &) {} + void dispatch(const Protocol::RegisterClient &) override {} + void dispatch(const Protocol::ClientDenied &) override {} + void dispatch(const Protocol::ClientRegistered &) override {} + void dispatch(const Protocol::SetupData &) override {} + void dispatch(const Protocol::SetupFailed &) override {} + void dispatch(const Protocol::SetupDone &) override {} + void dispatch(const Protocol::Login &) override {} + void dispatch(const Protocol::LoginFailed &) override {} + void dispatch(const Protocol::LoginSuccess &) override {} + void dispatch(const Protocol::SessionState &) override {} public slots: - void close(const QString &reason = QString()); + void close(const QString &reason = QString()) override; -protected: - void customEvent(QEvent *event); +signals: + void dispatchMessage(const Protocol::SyncMessage &msg); + void dispatchMessage(const Protocol::RpcCall &msg); + void dispatchMessage(const Protocol::InitRequest &msg); + void dispatchMessage(const Protocol::InitData &msg); private slots: void peerDisconnected(); + void handleMessage(const Protocol::SyncMessage &msg); + void handleMessage(const Protocol::RpcCall &msg); + void handleMessage(const Protocol::InitRequest &msg); + void handleMessage(const Protocol::InitData &msg); + private: - template - void dispatch(EventType eventType, const T &msg); + template + void handle(const T &msg); private: - SignalProxy *_proxy; - InternalPeer *_peer; - bool _isOpen; + SignalProxy *_proxy{nullptr}; + bool _isOpen{false}; }; - - -#endif diff --git a/src/common/protocol.h b/src/common/protocol.h index 499d36e3..2453a520 100644 --- a/src/common/protocol.h +++ b/src/common/protocol.h @@ -189,8 +189,9 @@ struct SignalProxyMessage struct SyncMessage : public SignalProxyMessage { - inline SyncMessage(const QByteArray &className, const QString &objectName, const QByteArray &slotName, const QVariantList ¶ms) - : className(className), objectName(objectName), slotName(slotName), params(params) {} + SyncMessage() = default; + SyncMessage(const QByteArray &className, const QString &objectName, const QByteArray &slotName, const QVariantList ¶ms) + : className(className), objectName(objectName), slotName(slotName), params(params) {} QByteArray className; QString objectName; @@ -201,8 +202,9 @@ struct SyncMessage : public SignalProxyMessage struct RpcCall : public SignalProxyMessage { - inline RpcCall(const QByteArray &slotName, const QVariantList ¶ms) - : slotName(slotName), params(params) {} + RpcCall() = default; + RpcCall(const QByteArray &slotName, const QVariantList ¶ms) + : slotName(slotName), params(params) {} QByteArray slotName; QVariantList params; @@ -211,8 +213,9 @@ struct RpcCall : public SignalProxyMessage struct InitRequest : public SignalProxyMessage { - inline InitRequest(const QByteArray &className, const QString &objectName) - : className(className), objectName(objectName) {} + InitRequest() = default; + InitRequest(const QByteArray &className, const QString &objectName) + : className(className), objectName(objectName) {} QByteArray className; QString objectName; @@ -221,8 +224,9 @@ struct InitRequest : public SignalProxyMessage struct InitData : public SignalProxyMessage { - inline InitData(const QByteArray &className, const QString &objectName, const QVariantMap &initData) - : className(className), objectName(objectName), initData(initData) {} + InitData() = default; + InitData(const QByteArray &className, const QString &objectName, const QVariantMap &initData) + : className(className), objectName(objectName), initData(initData) {} QByteArray className; QString objectName; @@ -249,3 +253,9 @@ struct HeartBeatReply }; + +// Required for InternalPeer +Q_DECLARE_METATYPE(Protocol::SyncMessage) +Q_DECLARE_METATYPE(Protocol::RpcCall) +Q_DECLARE_METATYPE(Protocol::InitRequest) +Q_DECLARE_METATYPE(Protocol::InitData)