core: Fix quitting from networks on shutdown
authorManuel Nickschas <sputnick@quassel-irc.org>
Sat, 29 Sep 2018 17:38:43 +0000 (19:38 +0200)
committerManuel Nickschas <sputnick@quassel-irc.org>
Mon, 1 Oct 2018 17:06:49 +0000 (19:06 +0200)
The previous implementation relied on processEvents() to let the
QUIT command be sent to the IRC server. However, processEvents()
does not guarantee that it processes all events (also in other
threads), and indeed, the previous approach was not reliable.

Fix this by waiting for the networks to emit the disconnected()
signal after requesting the quit. There already is a 10 second
timeout in case the IRC server does not close the socket within
a reasonable time, so rely on that. Also ensure that connect requests
are ignored during shutdown.

With the CoreSession requesting a clean shutdown, we no longer need
a similar handling in CoreNetwork's dtor, so remove this.

See also GH-203 and GH-207, which were previous attempts on getting
this right.

src/core/corenetwork.cpp
src/core/corenetwork.h
src/core/coresession.cpp
src/core/coresession.h

index e0be7e6..2b8f131 100644 (file)
@@ -122,23 +122,12 @@ CoreNetwork::CoreNetwork(const NetworkId &networkid, CoreSession *session)
 
 CoreNetwork::~CoreNetwork()
 {
-    // Request a proper disconnect, but don't count as user-requested disconnect
-    if (socketConnected()) {
-        // Only try if the socket's fully connected (not initializing or disconnecting).
-        // Force an immediate disconnect, jumping the command queue.  Ensures the proper QUIT is
-        // shown even if other messages are queued.
-        disconnectFromIrc(false, QString(), false, true);
-        // Process the putCmd events that trigger the quit.  Without this, shutting down the core
-        // results in abrubtly closing the socket rather than sending the QUIT as expected.
-        QCoreApplication::processEvents();
-        // Wait briefly for each network to disconnect.  Sometimes it takes a little while to send.
-        if (!forceDisconnect()) {
-            qWarning() << "Timed out quitting network" << networkName() <<
-                          "(user ID " << userId() << ")";
-        }
+    // Ensure we don't get any more signals from the socket while shutting down
+    disconnect(&socket, nullptr, this, nullptr);
+    if (!forceDisconnect()) {
+        qWarning() << QString{"Could not disconnect from network %1 (network ID: %2, user ID: %3)"}
+                      .arg(networkName()).arg(networkId().toInt()).arg(userId().toInt());
     }
-    disconnect(&socket, 0, this, 0); // this keeps the socket from triggering events during clean up
-    delete _userInputHandler;
 }
 
 
@@ -150,8 +139,10 @@ bool CoreNetwork::forceDisconnect(int msecs)
     }
     // Request a socket-level disconnect if not already happened
     socket.disconnectFromHost();
-    // Return the result of waiting for disconnect; true if successful, otherwise false
-    return socket.waitForDisconnected(msecs);
+    if (socket.state() != QAbstractSocket::UnconnectedState) {
+        return socket.waitForDisconnected(msecs);
+    }
+    return true;
 }
 
 
@@ -197,6 +188,10 @@ QByteArray CoreNetwork::userEncode(const QString &userNick, const QString &strin
 
 void CoreNetwork::connectToIrc(bool reconnecting)
 {
+    if (_shuttingDown) {
+        return;
+    }
+
     if (Core::instance()->identServer()) {
         _socketId = Core::instance()->identServer()->addWaitingSocket();
     }
@@ -287,8 +282,7 @@ void CoreNetwork::connectToIrc(bool reconnecting)
 }
 
 
-void CoreNetwork::disconnectFromIrc(bool requested, const QString &reason, bool withReconnect,
-                                    bool forceImmediate)
+void CoreNetwork::disconnectFromIrc(bool requested, const QString &reason, bool withReconnect)
 {
     // Disconnecting from the network, should expect a socket close or error
     _disconnectExpected = true;
@@ -316,20 +310,38 @@ void CoreNetwork::disconnectFromIrc(bool requested, const QString &reason, bool
     displayMsg(Message::Server, BufferInfo::StatusBuffer, "", tr("Disconnecting. (%1)").arg((!requested && !withReconnect) ? tr("Core Shutdown") : _quitReason));
     if (socket.state() == QAbstractSocket::UnconnectedState) {
         socketDisconnected();
-    } else {
+    }
+    else {
         if (socket.state() == QAbstractSocket::ConnectedState) {
-            userInputHandler()->issueQuit(_quitReason, forceImmediate);
-        } else {
+            // If shutting down, prioritize the QUIT command
+            userInputHandler()->issueQuit(_quitReason, _shuttingDown);
+        }
+        else {
             socket.close();
         }
-        if (requested || withReconnect) {
-            // the irc server has 10 seconds to close the socket
+        if (socket.state() != QAbstractSocket::UnconnectedState) {
+            // Wait for up to 10 seconds for the socket to close cleanly, then it will be forcefully aborted
             _socketCloseTimer.start(10000);
         }
     }
 }
 
 
+void CoreNetwork::socketCloseTimeout()
+{
+    qWarning() << QString{"Timed out quitting network %1 (network ID: %2, user ID: %3)"}
+                  .arg(networkName()).arg(networkId().toInt()).arg(userId().toInt());
+    socket.abort();
+}
+
+
+void CoreNetwork::shutdown()
+{
+    _shuttingDown = true;
+    disconnectFromIrc(false, {}, false);
+}
+
+
 void CoreNetwork::userInput(BufferInfo buf, QString msg)
 {
     userInputHandler()->handleUserInput(buf, msg);
@@ -1506,6 +1518,9 @@ Network::Server CoreNetwork::usedServer() const
 
 void CoreNetwork::requestConnect() const
 {
+    if (_shuttingDown) {
+        return;
+    }
     if (connectionState() != Disconnected) {
         qWarning() << "Requesting connect while already being connected!";
         return;
@@ -1516,6 +1531,9 @@ void CoreNetwork::requestConnect() const
 
 void CoreNetwork::requestDisconnect() const
 {
+    if (_shuttingDown) {
+        return;
+    }
     if (connectionState() == Disconnected) {
         qWarning() << "Requesting disconnect while not being connected!";
         return;
index 3efcfca..3023044 100644 (file)
@@ -57,7 +57,8 @@ class CoreNetwork : public Network
 
 public:
     CoreNetwork(const NetworkId &networkid, CoreSession *session);
-    ~CoreNetwork();
+    ~CoreNetwork() override;
+
     inline virtual const QMetaObject *syncMetaObject() const { return &Network::staticMetaObject; }
 
     inline CoreIdentity *identityPtr() const { return coreSession()->identity(identity()); }
@@ -229,6 +230,13 @@ public slots:
      */
     void setPongTimestampValid(bool validTimestamp);
 
+    /**
+     * Indicates that the CoreSession is shutting down.
+     *
+     * Disconnects the network if connected, and sets a flag that prevents reconnections.
+     */
+    void shutdown();
+
     void connectToIrc(bool reconnecting = false);
     /**
      * Disconnect from the IRC server.
@@ -238,16 +246,14 @@ public slots:
      * @param requested       If true, user requested this disconnect; don't try to reconnect
      * @param reason          Reason for quitting, defaulting to the user-configured quit reason
      * @param withReconnect   Reconnect to the network after disconnecting (e.g. ping timeout)
-     * @param forceImmediate  Immediately disconnect from network, skipping queue of other commands
      */
-    void disconnectFromIrc(bool requested = true, const QString &reason = QString(),
-                           bool withReconnect = false, bool forceImmediate = false);
+    void disconnectFromIrc(bool requested = true, const QString &reason = QString(), bool withReconnect = false);
 
     /**
      * Forcibly close the IRC server socket, waiting for it to close.
      *
      * Call CoreNetwork::disconnectFromIrc() first, allow the event loop to run, then if you need to
-     * be sure the network's disconencted (e.g. clean-up), call this.
+     * be sure the network's disconnected (e.g. clean-up), call this.
      *
      * @param msecs  Maximum time to wait for socket to close, in milliseconds.
      * @return True if socket closes successfully; false if error occurs or timeout reached
@@ -456,7 +462,7 @@ private slots:
     void socketHasData();
     void socketError(QAbstractSocket::SocketError);
     void socketInitialized();
-    inline void socketCloseTimeout() { socket.abort(); }
+    void socketCloseTimeout();
     void socketDisconnected();
     void socketStateChanged(QAbstractSocket::SocketState);
     void networkInitialized();
@@ -527,6 +533,8 @@ private:
     // This avoids logging a spurious RemoteHostClosedError whenever disconnect is called without
     // specifying a permanent (saved to core session) disconnect.
 
+    bool _shuttingDown{false};  ///< If true, we're shutting down and ignore requests to (dis)connect networks
+
     bool _previousConnectionAttemptFailed;
     int _lastUsedServerIndex;
 
index 8d08d5b..c38c3d6 100644 (file)
@@ -158,53 +158,30 @@ void CoreSession::shutdown()
 {
     saveSessionState();
 
-    /* Why partially duplicate CoreNetwork destructor?  When each CoreNetwork quits in the
-     * destructor, disconnections are processed in sequence for each object.  For many IRC servers
-     * on a slow network, this could significantly delay core shutdown [msecs wait * network count].
-     *
-     * Here, CoreSession first calls disconnect on all networks, letting them all start
-     * disconnecting before beginning to sequentially wait for each network.  Ideally, after the
-     * first network is disconnected, the other networks will have already closed.  Worst-case may
-     * still wait [msecs wait time * num. of networks], but the risk should be much lower.
-     *
-     * CoreNetwork should still do cleanup in its own destructor in case a network is deleted
-     * outside of deleting the whole CoreSession.
-     *
-     * If this proves to be problematic in the future, there's an alternative Qt signal-based system
-     * implemented in another pull request that guarentees a maximum amount of time to disconnect,
-     * but at the cost of more complex code.
-     *
-     * See https://github.com/quassel/quassel/pull/203
-     */
-
-    foreach(CoreNetwork *net, _networks.values()) {
-        // Request each network properly disconnect, but don't count as user-requested disconnect
-        if (net->socketConnected()) {
-            // Only try if the socket's fully connected (not initializing or disconnecting).
-            // Force an immediate disconnect, jumping the command queue.  Ensures the proper QUIT is
-            // shown even if other messages are queued.
-            net->disconnectFromIrc(false, QString(), false, true);
+    // Request disconnect from all connected networks in parallel, and wait until every network
+    // has emitted the disconnected() signal before deleting the session itself
+    for (CoreNetwork *net : _networks.values()) {
+        if (net->socketState() != QAbstractSocket::UnconnectedState) {
+            _networksPendingDisconnect.insert(net->networkId());
+            connect(net, SIGNAL(disconnected(NetworkId)), this, SLOT(onNetworkDisconnected(NetworkId)));
+            net->shutdown();
         }
     }
 
-    // Process the putCmd events that trigger the quit.  Without this, shutting down the core
-    // results in abrubtly closing the socket rather than sending the QUIT as expected.
-    QCoreApplication::processEvents();
-
-    foreach(CoreNetwork *net, _networks.values()) {
-        // Wait briefly for each network to disconnect.  Sometimes it takes a little while to send.
-        if (!net->forceDisconnect()) {
-            qWarning() << "Timed out quitting network" << net->networkName() <<
-                          "(user ID " << net->userId() << ")";
-        }
-        // Delete the network now that it's closed
-        delete net;
+    if (_networksPendingDisconnect.isEmpty()) {
+        // Nothing to do, suicide so the core can shut down
+        deleteLater();
     }
+}
 
-    _networks.clear();
 
-    // Suicide
-    deleteLater();
+void CoreSession::onNetworkDisconnected(NetworkId networkId)
+{
+    _networksPendingDisconnect.remove(networkId);
+    if (_networksPendingDisconnect.isEmpty()) {
+        // We're done, suicide so the core can shut down
+        deleteLater();
+    }
 }
 
 
index 15c9533..b4903ff 100644 (file)
  *   51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.         *
  ***************************************************************************/
 
-#ifndef CORESESSION_H
-#define CORESESSION_H
+#pragma once
 
+#include <QHash>
+#include <QSet>
 #include <QString>
 #include <QVariant>
 
@@ -213,6 +214,8 @@ private slots:
 
     void saveSessionState() const;
 
+    void onNetworkDisconnected(NetworkId networkId);
+
 private:
     void processMessages();
 
@@ -229,10 +232,10 @@ private:
 
     SignalProxy *_signalProxy;
     CoreAliasManager _aliasManager;
-    // QHash<NetworkId, NetworkConnection *> _connections;
-    QHash<NetworkId, CoreNetwork *> _networks;
-    //  QHash<NetworkId, CoreNetwork *> _networksToRemove;
+
     QHash<IdentityId, CoreIdentity *> _identities;
+    QHash<NetworkId, CoreNetwork *> _networks;
+    QSet<NetworkId> _networksPendingDisconnect;
 
     CoreBufferSyncer *_bufferSyncer;
     CoreBacklogManager *_backlogManager;
@@ -290,5 +293,3 @@ struct RawMessage {
     RawMessage(NetworkId networkId, Message::Type type, BufferInfo::Type bufferType, const QString &target, const QString &text, const QString &sender, Message::Flags flags)
         : networkId(networkId), type(type), bufferType(bufferType), target(target), text(text), sender(sender), flags(flags) {}
 };
-
-#endif