From ddfb1d2574c4bffd180361a80df9b1cd584bb040 Mon Sep 17 00:00:00 2001 From: Manuel Nickschas Date: Sat, 29 Sep 2018 17:31:42 +0200 Subject: [PATCH 1/1] core: Allow clean shutdown of the core Relying on things to happen while the core is being destroyed is fragile (and does not work in practice), because events are no longer delivered to objects that are scheduled for deletion. Add an explicit call for shutting down the core, its sessions and their networks. Wait for everything to be cleaned up properly before quitting the session threads, and ultimately shutting down the main event loop. While we're at it, refactor SessionThread to follow the worker pattern, rather than inheriting from QThread. This avoids accidentally calling slots in the wrong thread. Ensure that all cross-thread communication happens through queued signals. Simplify the API, too. --- src/core/core.cpp | 39 ++++++-- src/core/core.h | 11 +++ src/core/coreapplication.cpp | 19 ++-- src/core/coreapplication.h | 6 +- src/core/coresession.cpp | 7 +- src/core/coresession.h | 6 +- src/core/sessionthread.cpp | 170 ++++++++++++++++++++--------------- src/core/sessionthread.h | 45 ++++------ src/qtui/monoapplication.cpp | 12 +++ src/qtui/monoapplication.h | 1 + 10 files changed, 197 insertions(+), 119 deletions(-) diff --git a/src/core/core.cpp b/src/core/core.cpp index 7f69869a..2634dd28 100644 --- a/src/core/core.cpp +++ b/src/core/core.cpp @@ -81,7 +81,6 @@ Core::Core() Core::~Core() { - saveState(); qDeleteAll(_connectingClients); qDeleteAll(_sessions); syncStorage(); @@ -260,6 +259,39 @@ void Core::initAsync() } +void Core::shutdown() +{ + quInfo() << "Core shutting down..."; + + saveState(); + + for (auto &&client : _connectingClients) { + client->deleteLater(); + } + _connectingClients.clear(); + + if (_sessions.isEmpty()) { + emit shutdownComplete(); + return; + } + + for (auto &&session : _sessions) { + connect(session, SIGNAL(shutdownComplete(SessionThread*)), this, SLOT(onSessionShutdown(SessionThread*))); + session->shutdown(); + } +} + + +void Core::onSessionShutdown(SessionThread *session) +{ + _sessions.take(_sessions.key(session))->deleteLater(); + if (_sessions.isEmpty()) { + quInfo() << "Core shutdown complete!"; + emit shutdownComplete(); + } +} + + /*** Session Restore ***/ void Core::saveState() @@ -837,10 +869,7 @@ SessionThread *Core::sessionForUser(UserId uid, bool restore) if (_sessions.contains(uid)) return _sessions[uid]; - SessionThread *session = new SessionThread(uid, restore, strictIdentEnabled(), this); - _sessions[uid] = session; - session->start(); - return session; + return (_sessions[uid] = new SessionThread(uid, restore, strictIdentEnabled(), this)); } diff --git a/src/core/core.h b/src/core/core.h index efbefa69..021d9c94 100644 --- a/src/core/core.h +++ b/src/core/core.h @@ -69,6 +69,12 @@ public: void init(); + /** + * Shuts down active core sessions, saves state and emits the shutdownComplete() signal afterwards. + */ + void shutdown(); + + /*** Storage access ***/ // These methods are threadsafe. @@ -704,6 +710,9 @@ signals: //! Emitted when a fatal error was encountered during async initialization void exitRequested(int exitCode, const QString &reason); + //! Emitted once core shutdown is complete + void shutdownComplete(); + public slots: void initAsync(); @@ -747,6 +756,8 @@ private slots: bool changeUserPass(const QString &username); + void onSessionShutdown(SessionThread *session); + private: SessionThread *sessionForUser(UserId userId, bool restoreState = false); void addClientHelper(RemotePeer *peer, UserId uid); diff --git a/src/core/coreapplication.cpp b/src/core/coreapplication.cpp index 90f39055..9ad6ef09 100644 --- a/src/core/coreapplication.cpp +++ b/src/core/coreapplication.cpp @@ -18,10 +18,8 @@ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. * ***************************************************************************/ -#include "core.h" #include "coreapplication.h" - CoreApplication::CoreApplication(int &argc, char **argv) : QCoreApplication(argc, argv) { @@ -30,12 +28,10 @@ CoreApplication::CoreApplication(int &argc, char **argv) #endif /* Q_OS_MAC */ Quassel::setRunMode(Quassel::CoreOnly); -} - - -CoreApplication::~CoreApplication() -{ - _core.reset(); + Quassel::registerQuitHandler([this]() { + connect(_core.get(), SIGNAL(shutdownComplete()), this, SLOT(onShutdownComplete())); + _core->shutdown(); + }); } @@ -48,3 +44,10 @@ void CoreApplication::init() _core.reset(new Core{}); // FIXME C++14: std::make_unique _core->init(); } + + +void CoreApplication::onShutdownComplete() +{ + connect(_core.get(), SIGNAL(destroyed()), QCoreApplication::instance(), SLOT(quit())); + _core.release()->deleteLater(); +} diff --git a/src/core/coreapplication.h b/src/core/coreapplication.h index f157b548..216eda0f 100644 --- a/src/core/coreapplication.h +++ b/src/core/coreapplication.h @@ -24,6 +24,7 @@ #include +#include "core.h" #include "quassel.h" class Core; @@ -31,12 +32,15 @@ class Core; class CoreApplication : public QCoreApplication { Q_OBJECT + public: CoreApplication(int &argc, char **argv); - ~CoreApplication() override; void init(); +private slots: + void onShutdownComplete(); + private: std::unique_ptr _core; }; diff --git a/src/core/coresession.cpp b/src/core/coresession.cpp index c81ac894..8d08d5bf 100644 --- a/src/core/coresession.cpp +++ b/src/core/coresession.cpp @@ -154,7 +154,7 @@ CoreSession::CoreSession(UserId uid, bool restoreState, bool strictIdentEnabled, } -CoreSession::~CoreSession() +void CoreSession::shutdown() { saveSessionState(); @@ -200,6 +200,11 @@ CoreSession::~CoreSession() // Delete the network now that it's closed delete net; } + + _networks.clear(); + + // Suicide + deleteLater(); } diff --git a/src/core/coresession.h b/src/core/coresession.h index fe81a2b4..15c95339 100644 --- a/src/core/coresession.h +++ b/src/core/coresession.h @@ -63,7 +63,6 @@ class CoreSession : public QObject public: CoreSession(UserId, bool restoreState, bool strictIdentEnabled, QObject *parent = 0); - ~CoreSession(); QList buffers() const; inline UserId user() const { return _user; } @@ -112,6 +111,11 @@ public slots: void addClient(RemotePeer *peer); void addClient(InternalPeer *peer); + /** + * Shuts down the session and deletes itself afterwards. + */ + void shutdown(); + void msgFromClient(BufferInfo, QString message); //! Create an identity and propagate the changes to the clients. diff --git a/src/core/sessionthread.cpp b/src/core/sessionthread.cpp index 7b8bc105..7df83068 100644 --- a/src/core/sessionthread.cpp +++ b/src/core/sessionthread.cpp @@ -18,6 +18,9 @@ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. * ***************************************************************************/ +#include +#include + #include "core.h" #include "coresession.h" #include "internalpeer.h" @@ -25,107 +28,128 @@ #include "sessionthread.h" #include "signalproxy.h" -SessionThread::SessionThread(UserId uid, bool restoreState, bool strictIdentEnabled, QObject *parent) - : QThread(parent), - _session(0), - _user(uid), - _sessionInitialized(false), - _restoreState(restoreState), - _strictIdentEnabled(strictIdentEnabled) -{ - connect(this, SIGNAL(initialized()), this, SLOT(setSessionInitialized())); -} - +namespace { -SessionThread::~SessionThread() +class Worker : public QObject { - // shut down thread gracefully - quit(); - wait(); -} + Q_OBJECT + +public: + Worker(UserId userId, bool restoreState, bool strictIdentEnabled) + : _userId{userId} + , _restoreState{restoreState} + , _strictIdentEnabled{strictIdentEnabled} + { + } +public slots: + void initialize() + { + _session = new CoreSession{_userId, _restoreState, _strictIdentEnabled, this}; + connect(_session, SIGNAL(destroyed()), QThread::currentThread(), SLOT(quit())); + connect(_session, SIGNAL(sessionState(Protocol::SessionState)), Core::instance(), SIGNAL(sessionState(Protocol::SessionState))); + emit initialized(); + } -CoreSession *SessionThread::session() -{ - return _session; -} + void shutdown() + { + if (_session) { + _session->shutdown(); + } + } + void addClient(Peer *peer) + { + if (!_session) { + qWarning() << "Session not initialized!"; + return; + } + + auto remotePeer = qobject_cast(peer); + if (remotePeer) { + _session->addClient(remotePeer); + return; + } + auto internalPeer = qobject_cast(peer); + if (internalPeer) { + _session->addClient(internalPeer); + return; + } + + qWarning() << "SessionThread::addClient() received invalid peer!" << peer; + } -UserId SessionThread::user() -{ - return _user; -} +signals: + void initialized(); +private: + UserId _userId; + bool _restoreState; + bool _strictIdentEnabled; ///< Whether or not strict ident mode is enabled, locking users' idents to Quassel username + QPointer _session; +}; -bool SessionThread::isSessionInitialized() +} // anon + +SessionThread::SessionThread(UserId uid, bool restoreState, bool strictIdentEnabled, QObject *parent) + : QObject(parent) { - return _sessionInitialized; + auto worker = new Worker(uid, restoreState, strictIdentEnabled); + worker->moveToThread(&_sessionThread); + connect(&_sessionThread, SIGNAL(started()), worker, SLOT(initialize())); + connect(&_sessionThread, SIGNAL(finished()), worker, SLOT(deleteLater())); + connect(worker, SIGNAL(initialized()), this, SLOT(onSessionInitialized())); + connect(worker, SIGNAL(destroyed()), this, SLOT(onSessionDestroyed())); + + connect(this, SIGNAL(addClientToWorker(Peer*)), worker, SLOT(addClient(Peer*))); + connect(this, SIGNAL(shutdownSession()), worker, SLOT(shutdown())); + + // Defer thread start through the event loop, so the SessionThread instance is fully constructed before + QTimer::singleShot(0, &_sessionThread, SLOT(start())); } -void SessionThread::setSessionInitialized() +SessionThread::~SessionThread() { - _sessionInitialized = true; - foreach(QObject *peer, clientQueue) { - addClientToSession(peer); - } - clientQueue.clear(); + // shut down thread gracefully + _sessionThread.quit(); + _sessionThread.wait(30000); } -// this and the following related methods are executed in the Core thread! -void SessionThread::addClient(QObject *peer) +void SessionThread::shutdown() { - if (isSessionInitialized()) { - addClientToSession(peer); - } - else { - clientQueue.append(peer); - } + emit shutdownSession(); } -void SessionThread::addClientToSession(QObject *peer) +void SessionThread::onSessionInitialized() { - RemotePeer *remote = qobject_cast(peer); - if (remote) { - addRemoteClientToSession(remote); - return; - } - - InternalPeer *internal = qobject_cast(peer); - if (internal) { - addInternalClientToSession(internal); - return; + _sessionInitialized = true; + for (auto &&peer : _clientQueue) { + peer->setParent(nullptr); + peer->moveToThread(&_sessionThread); + emit addClientToWorker(peer); } - - qWarning() << "SessionThread::addClient() received invalid peer!" << peer; + _clientQueue.clear(); } -void SessionThread::addRemoteClientToSession(RemotePeer *remotePeer) +void SessionThread::onSessionDestroyed() { - remotePeer->setParent(0); - remotePeer->moveToThread(session()->thread()); - emit addRemoteClient(remotePeer); + emit shutdownComplete(this); } - -void SessionThread::addInternalClientToSession(InternalPeer *internalPeer) +void SessionThread::addClient(Peer *peer) { - internalPeer->setParent(0); - internalPeer->moveToThread(session()->thread()); - emit addInternalClient(internalPeer); + if (_sessionInitialized) { + peer->setParent(nullptr); + peer->moveToThread(&_sessionThread); + emit addClientToWorker(peer); + } + else { + _clientQueue.push_back(peer); + } } - -void SessionThread::run() -{ - _session = new CoreSession(user(), _restoreState, _strictIdentEnabled); - connect(this, SIGNAL(addRemoteClient(RemotePeer*)), _session, SLOT(addClient(RemotePeer*))); - connect(this, SIGNAL(addInternalClient(InternalPeer*)), _session, SLOT(addClient(InternalPeer*))); - connect(_session, SIGNAL(sessionState(Protocol::SessionState)), Core::instance(), SIGNAL(sessionState(Protocol::SessionState))); - emit initialized(); - exec(); - delete _session; -} +#include "sessionthread.moc" diff --git a/src/core/sessionthread.h b/src/core/sessionthread.h index c215590f..63bb2a59 100644 --- a/src/core/sessionthread.h +++ b/src/core/sessionthread.h @@ -18,20 +18,20 @@ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. * ***************************************************************************/ -#ifndef SESSIONTHREAD_H -#define SESSIONTHREAD_H +#pragma once + +#include -#include #include #include "types.h" class CoreSession; +class Peer; class InternalPeer; class RemotePeer; -class QIODevice; -class SessionThread : public QThread +class SessionThread : public QObject { Q_OBJECT @@ -39,39 +39,24 @@ public: SessionThread(UserId user, bool restoreState, bool strictIdentEnabled, QObject *parent = 0); ~SessionThread(); - void run(); - - CoreSession *session(); - UserId user(); - public slots: - void addClient(QObject *peer); + void addClient(Peer *peer); + void shutdown(); private slots: - void setSessionInitialized(); + void onSessionInitialized(); + void onSessionDestroyed(); signals: void initialized(); - void shutdown(); + void shutdownSession(); + void shutdownComplete(SessionThread *); - void addRemoteClient(RemotePeer *peer); - void addInternalClient(InternalPeer *peer); + void addClientToWorker(Peer *peer); private: - CoreSession *_session; - UserId _user; - QList clientQueue; - bool _sessionInitialized; - bool _restoreState; - - /// Whether or not strict ident mode is enabled, locking users' idents to Quassel username - bool _strictIdentEnabled; + QThread _sessionThread; + bool _sessionInitialized{false}; - bool isSessionInitialized(); - void addClientToSession(QObject *peer); - void addRemoteClientToSession(RemotePeer *remotePeer); - void addInternalClientToSession(InternalPeer *internalPeer); + std::vector _clientQueue; }; - - -#endif diff --git a/src/qtui/monoapplication.cpp b/src/qtui/monoapplication.cpp index c9695cc8..128ab979 100644 --- a/src/qtui/monoapplication.cpp +++ b/src/qtui/monoapplication.cpp @@ -63,6 +63,18 @@ Quassel::QuitHandler MonolithicApplication::quitHandler() void MonolithicApplication::onClientDestroyed() +{ + if (_core) { + connect(_core, SIGNAL(shutdownComplete()), this, SLOT(onCoreShutdown())); + _core->shutdown(); + } + else { + QCoreApplication::quit(); + } +} + + +void MonolithicApplication::onCoreShutdown() { if (_core) { connect(_core, SIGNAL(destroyed()), QCoreApplication::instance(), SLOT(quit())); diff --git a/src/qtui/monoapplication.h b/src/qtui/monoapplication.h index 6064312d..a63dfe07 100644 --- a/src/qtui/monoapplication.h +++ b/src/qtui/monoapplication.h @@ -46,6 +46,7 @@ signals: private slots: void onConnectionRequest(QPointer peer); void onClientDestroyed(); + void onCoreShutdown(); private: void startInternalCore(); -- 2.20.1