Fix write connection reconnection issues (#2850)

Co-authored-by: Felanbird <41973452+Felanbird@users.noreply.github.com>
Co-authored-by: pajlada <rasmus.karlsson@pajlada.com>

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
 

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<Channel> AbstractIrcServer::getCustomChannel(
     const QString &channelName)
 {

src/providers/irc/AbstractIrcServer.hpp

@@ -70,7 +70,6 @@ protected:
     virtual void onReadConnected(IrcConnection *connection);
     virtual void onWriteConnected(IrcConnection *connection);
     virtual void onDisconnected();
-    virtual void onSocketError();
 
     virtual std::shared_ptr<Channel> getCustomChannel(
         const QString &channelName);

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::milliseconds>(
+                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

src/providers/irc/IrcConnection2.hpp

@@ -4,6 +4,7 @@
 
 #include <IrcConnection>
 #include <QTimer>
+#include <chrono>
 
 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<bool> 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<bool> recentlyReceivedMessage_{true};
+    std::chrono::steady_clock::time_point lastConnected_;
 
-    // waitingForPong_ is set to true when we send a PING message, and back to false when we receive the matching PONG response
+    std::atomic<bool> expectConnectionLoss_{false};
+
+    // waitingForPong_ is set to true when we send a PING message, and back to
+    // false when we receive the matching PONG response
     std::atomic<bool> waitingForPong_{false};
 };