diff --git a/CHANGELOG.md b/CHANGELOG.md index 74cba16da1a..0ffcb8a9f3f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ - Minor: Added a link to accounts page in settings to "You need to be logged in to send messages" message. (#2862) - Minor: Switch to Twitch v2 emote API for animated emote support. (#2863) - Bugfix: Fixed FFZ emote links for global emotes (#2807, #2808) +- Bugfix: Fix reconnecting when IRC write connection is lost (#1831, #2356, #2850) ## 2.3.2 diff --git a/src/providers/irc/AbstractIrcServer.cpp b/src/providers/irc/AbstractIrcServer.cpp index b8396fe260c..be9fc4f88b4 100644 --- a/src/providers/irc/AbstractIrcServer.cpp +++ b/src/providers/irc/AbstractIrcServer.cpp @@ -18,7 +18,7 @@ const int MAX_FALLOFF_COUNTER = 60; AbstractIrcServer::AbstractIrcServer() { // Initialize the connections - // XXX: don't create write connection if there is not separate write connection. + // XXX: don't create write connection if there is no separate write connection. this->writeConnection_.reset(new IrcConnection); this->writeConnection_->moveToThread( QCoreApplication::instance()->thread()); @@ -32,6 +32,11 @@ AbstractIrcServer::AbstractIrcServer() &Communi::IrcConnection::connected, this, [this] { this->onWriteConnected(this->writeConnection_.get()); }); + this->writeConnection_->connectionLost.connect([this](bool timeout) { + qCDebug(chatterinoIrc) + << "Write connection reconnect requested. Timeout:" << timeout; + this->writeConnection_->smartReconnect.invoke(); + }); // Listen to read connection message signals this->readConnection_.reset(new IrcConnection); @@ -55,34 +60,17 @@ AbstractIrcServer::AbstractIrcServer() &Communi::IrcConnection::disconnected, this, [this] { this->onDisconnected(); }); - QObject::connect(this->readConnection_.get(), - &Communi::IrcConnection::socketError, this, [this] { - this->onSocketError(); - }); - - // listen to reconnect request - this->readConnection_->reconnectRequested.connect([this] { - this->addGlobalSystemMessage( - "Server connection timed out, reconnecting"); - this->connect(); - }); - // this->writeConnection->reconnectRequested.connect([this] { - // this->connect(); }); - this->reconnectTimer_.setInterval(RECONNECT_BASE_INTERVAL); - this->reconnectTimer_.setSingleShot(true); - QObject::connect(&this->reconnectTimer_, &QTimer::timeout, [this] { - this->reconnectTimer_.setInterval(RECONNECT_BASE_INTERVAL * - this->falloffCounter_); - - this->falloffCounter_ = - std::min(MAX_FALLOFF_COUNTER, this->falloffCounter_ + 1); - - if (!this->readConnection_->isConnected()) + this->readConnection_->connectionLost.connect([this](bool timeout) { + qCDebug(chatterinoIrc) + << "Read connection reconnect requested. Timeout:" << timeout; + if (timeout) { - qCDebug(chatterinoIrc) - << "Trying to reconnect..." << this->falloffCounter_; - this->connect(); + // Show additional message since this is going to interrupt a + // connection that is still "connected" + this->addGlobalSystemMessage( + "Server connection timed out, reconnecting"); } + this->readConnection_->smartReconnect.invoke(); }); } @@ -370,11 +358,6 @@ void AbstractIrcServer::onDisconnected() } } -void AbstractIrcServer::onSocketError() -{ - this->reconnectTimer_.start(); -} - std::shared_ptr AbstractIrcServer::getCustomChannel( const QString &channelName) { diff --git a/src/providers/irc/AbstractIrcServer.hpp b/src/providers/irc/AbstractIrcServer.hpp index de3a61ff5e1..7cfc38f5ef3 100644 --- a/src/providers/irc/AbstractIrcServer.hpp +++ b/src/providers/irc/AbstractIrcServer.hpp @@ -70,7 +70,6 @@ class AbstractIrcServer : public QObject virtual void onReadConnected(IrcConnection *connection); virtual void onWriteConnected(IrcConnection *connection); virtual void onDisconnected(); - virtual void onSocketError(); virtual std::shared_ptr getCustomChannel( const QString &channelName); diff --git a/src/providers/irc/IrcConnection2.cpp b/src/providers/irc/IrcConnection2.cpp index b46964b98f8..916cf544e19 100644 --- a/src/providers/irc/IrcConnection2.cpp +++ b/src/providers/irc/IrcConnection2.cpp @@ -1,9 +1,13 @@ #include "IrcConnection2.hpp" +#include "common/QLogging.hpp" #include "common/Version.hpp" namespace chatterino { +// The minimum interval between attempting to establish a new connection +const int RECONNECT_MIN_INTERVAL = 15000; + namespace { const auto payload = QString("chatterino/" CHATTERINO_VERSION); @@ -13,40 +17,95 @@ namespace { IrcConnection::IrcConnection(QObject *parent) : Communi::IrcConnection(parent) { - // send ping every x seconds + // Log connection errors for ease-of-debugging + QObject::connect(this, &Communi::IrcConnection::socketError, this, + [this](QAbstractSocket::SocketError error) { + qCDebug(chatterinoIrc) << "Connection error:" << error; + }); + + QObject::connect( + this, &Communi::IrcConnection::socketStateChanged, this, + [this](QAbstractSocket::SocketState state) { + if (state == QAbstractSocket::UnconnectedState) + { + this->pingTimer_.stop(); + + // The socket will enter unconnected state both in case of + // socket error (including failures to connect) and regular + // disconnects. We signal that the connection was lost if this + // was not the result of us calling `close`. + if (!this->expectConnectionLoss_.load()) + { + this->connectionLost.invoke(false); + } + } + }); + + // Schedule a reconnect that won't violate RECONNECT_MIN_INTERVAL + this->smartReconnect.connect([this] { + if (this->reconnectTimer_.isActive()) + { + return; + } + + auto delta = + std::chrono::duration_cast( + std::chrono::steady_clock::now() - this->lastConnected_) + .count(); + delta = delta < RECONNECT_MIN_INTERVAL + ? (RECONNECT_MIN_INTERVAL - delta) + : 10; + qCDebug(chatterinoIrc) << "Reconnecting in" << delta << "ms"; + this->reconnectTimer_.start(delta); + }); + + this->reconnectTimer_.setInterval(RECONNECT_MIN_INTERVAL); + this->reconnectTimer_.setSingleShot(true); + QObject::connect(&this->reconnectTimer_, &QTimer::timeout, [this] { + if (this->isConnected()) + { + // E.g. user manually reconnecting doesn't cancel this path, so + // just ignore + qCDebug(chatterinoIrc) << "Reconnect: already reconnected"; + } + else + { + qCDebug(chatterinoIrc) << "Reconnecting"; + this->open(); + } + }); + + // Send ping every x seconds this->pingTimer_.setInterval(5000); this->pingTimer_.start(); QObject::connect(&this->pingTimer_, &QTimer::timeout, [this] { if (this->isConnected()) { - if (this->waitingForPong_.load()) + if (this->recentlyReceivedMessage_.load()) { - // Not sending another ping as we haven't received the matching pong yet + // If we're still receiving messages, all is well + this->recentlyReceivedMessage_ = false; + this->waitingForPong_ = false; return; } - if (!this->recentlyReceivedMessage_.load()) + if (this->waitingForPong_.load()) + { + // The remote server did not send a PONG fast enough; close the + // connection + this->close(); + this->connectionLost.invoke(true); + } + else { this->sendRaw("PING " + payload); - this->reconnectTimer_.start(); this->waitingForPong_ = true; } - this->recentlyReceivedMessage_ = false; - } - }); - - // reconnect after x seconds without receiving a message - this->reconnectTimer_.setInterval(5000); - this->reconnectTimer_.setSingleShot(true); - QObject::connect(&this->reconnectTimer_, &QTimer::timeout, [this] { - if (this->isConnected()) - { - reconnectRequested.invoke(); } }); QObject::connect(this, &Communi::IrcConnection::connected, this, [this] { - this->waitingForPong_ = false; + this->pingTimer_.start(); }); QObject::connect(this, &Communi::IrcConnection::pongMessageReceived, @@ -57,20 +116,27 @@ IrcConnection::IrcConnection(QObject *parent) } }); - QObject::connect( - this, &Communi::IrcConnection::messageReceived, - [this](Communi::IrcMessage *message) { - this->recentlyReceivedMessage_ = true; + QObject::connect(this, &Communi::IrcConnection::messageReceived, + [this](Communi::IrcMessage *message) { + // This connection is probably still alive + this->recentlyReceivedMessage_ = true; + }); +} - if (this->reconnectTimer_.isActive()) - { - this->reconnectTimer_.stop(); +void IrcConnection::open() +{ + // Accurately track the time a connection was opened + this->lastConnected_ = std::chrono::steady_clock::now(); + this->expectConnectionLoss_ = false; + this->waitingForPong_ = false; + this->recentlyReceivedMessage_ = false; + Communi::IrcConnection::open(); +} - // The reconnect timer had started, that means we were waiting for a pong. - // Since we received a message, this means that any pong response is meaningless, so we can just stop waiting for the pong and send another ping soon:tm: - this->waitingForPong_ = false; - } - }); +void IrcConnection::close() +{ + this->expectConnectionLoss_ = true; + Communi::IrcConnection::close(); } } // namespace chatterino diff --git a/src/providers/irc/IrcConnection2.hpp b/src/providers/irc/IrcConnection2.hpp index 65f1e1ea000..930b5dc4a0b 100644 --- a/src/providers/irc/IrcConnection2.hpp +++ b/src/providers/irc/IrcConnection2.hpp @@ -4,6 +4,7 @@ #include #include +#include namespace chatterino { @@ -12,14 +13,27 @@ class IrcConnection : public Communi::IrcConnection public: IrcConnection(QObject *parent = nullptr); - pajlada::Signals::NoArgSignal reconnectRequested; + // Signal to notify that we're unexpectedly no longer connected, either due + // to a connection error or if we think we've timed out. It's up to the + // receiver to trigger a reconnect, if desired + pajlada::Signals::Signal connectionLost; + + // Request a reconnect with a minimum interval between attempts. + pajlada::Signals::NoArgSignal smartReconnect; + + virtual void open(); + virtual void close(); private: QTimer pingTimer_; QTimer reconnectTimer_; std::atomic recentlyReceivedMessage_{true}; + std::chrono::steady_clock::time_point lastConnected_; + + std::atomic expectConnectionLoss_{false}; - // waitingForPong_ is set to true when we send a PING message, and back to false when we receive the matching PONG response + // waitingForPong_ is set to true when we send a PING message, and back to + // false when we receive the matching PONG response std::atomic waitingForPong_{false}; };