From 94461212440b2eaeccdbbbb8a734782e8301280c Mon Sep 17 00:00:00 2001 From: Manuel Nickschas Date: Sat, 29 Sep 2018 19:38:43 +0200 Subject: [PATCH] core: Fix quitting from networks on shutdown 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 | 68 +++++++++++++++++++++++++--------------- src/core/corenetwork.h | 20 ++++++++---- src/core/coresession.cpp | 59 +++++++++++----------------------- src/core/coresession.h | 15 ++++----- 4 files changed, 83 insertions(+), 79 deletions(-) diff --git a/src/core/corenetwork.cpp b/src/core/corenetwork.cpp index e0be7e63..2b8f131a 100644 --- a/src/core/corenetwork.cpp +++ b/src/core/corenetwork.cpp @@ -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; diff --git a/src/core/corenetwork.h b/src/core/corenetwork.h index 3efcfca2..30230445 100644 --- a/src/core/corenetwork.h +++ b/src/core/corenetwork.h @@ -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; diff --git a/src/core/coresession.cpp b/src/core/coresession.cpp index 8d08d5bf..c38c3d6b 100644 --- a/src/core/coresession.cpp +++ b/src/core/coresession.cpp @@ -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(); + } } diff --git a/src/core/coresession.h b/src/core/coresession.h index 15c95339..b4903ff4 100644 --- a/src/core/coresession.h +++ b/src/core/coresession.h @@ -18,9 +18,10 @@ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. * ***************************************************************************/ -#ifndef CORESESSION_H -#define CORESESSION_H +#pragma once +#include +#include #include #include @@ -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 _connections; - QHash _networks; - // QHash _networksToRemove; + QHash _identities; + QHash _networks; + QSet _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 -- 2.20.1