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.

diff --git a/src/core/core.cpp b/src/core/core.cpp
index 7f69869..2634dd2 100644 (file)
--- 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 efbefa6..021d9c9 100644 (file)
--- 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 90f3905..9ad6ef0 100644 (file)
--- 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 f157b54..216eda0 100644 (file)
--- a/src/core/coreapplication.h
+++ b/src/core/coreapplication.h
@@ -24,6 +24,7 @@
 
 #include <QCoreApplication>
 
+#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> _core;
 };

diff --git a/src/core/coresession.cpp b/src/core/coresession.cpp
index c81ac89..8d08d5b 100644 (file)
--- 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 fe81a2b..15c9533 100644 (file)
--- 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<BufferInfo> 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 7b8bc10..7df8306 100644 (file)
--- 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 <QPointer>
+#include <QTimer>
+
 #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<RemotePeer*>(peer);
+        if (remotePeer) {
+            _session->addClient(remotePeer);
+            return;
+        }
+        auto internalPeer = qobject_cast<InternalPeer*>(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<CoreSession> _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<RemotePeer *>(peer);
-    if (remote) {
-        addRemoteClientToSession(remote);
-        return;
-    }
-
-    InternalPeer *internal = qobject_cast<InternalPeer *>(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 c215590..63bb2a5 100644 (file)
--- 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 <memory>
 
-#include <QMutex>
 #include <QThread>
 
 #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<QObject *> 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<Peer *> _clientQueue;
 };
-
-
-#endif

diff --git a/src/qtui/monoapplication.cpp b/src/qtui/monoapplication.cpp
index c9695cc..128ab97 100644 (file)
--- 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 6064312..a63dfe0 100644 (file)
--- a/src/qtui/monoapplication.h
+++ b/src/qtui/monoapplication.h
@@ -46,6 +46,7 @@ signals:
 private slots:
     void onConnectionRequest(QPointer<InternalPeer> peer);
     void onClientDestroyed();
+    void onCoreShutdown();
 
 private:
     void startInternalCore();