Use an exponential backoff when deciding how long we need to wait for reconnects (#2892)

Co-authored-by: Leon Richardt <leon.richardt@gmail.com>

CHANGELOG.md

@@ -20,7 +20,7 @@
 - Bugfix: Moderation buttons now show the correct time unit when using units other than seconds. (#1719, #2864)
 - Bugfix: Fixed FFZ emote links for global emotes (#2807, #2808)
 - Bugfix: Fixed pasting text with URLs included (#1688, #2855)
-- Bugfix: Fix reconnecting when IRC write connection is lost (#1831, #2356, #2850)
+- Bugfix: Fix reconnecting when IRC write connection is lost (#1831, #2356, #2850, #2892)
 - Bugfix: Fixed bit and new subscriber emotes not (re)loading in some rare cases. (#2856, #2857)
 
 ## 2.3.2

src/providers/irc/IrcConnection2.cpp

@@ -5,9 +5,6 @@
 
 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);
@@ -48,18 +45,11 @@ IrcConnection::IrcConnection(QObject *parent)
             return;
         }
 
-        auto delta =
+        auto delay = this->reconnectBackoff_.next();
-            std::chrono::duration_cast<std::chrono::milliseconds>(
+        qCDebug(chatterinoIrc) << "Reconnecting in" << delay.count() << "ms";
-                std::chrono::steady_clock::now() - this->lastConnected_)
+        this->reconnectTimer_.start(delay);
-                .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())
@@ -120,13 +110,12 @@ IrcConnection::IrcConnection(QObject *parent)
                      [this](Communi::IrcMessage *message) {
                          // This connection is probably still alive
                          this->recentlyReceivedMessage_ = true;
+                         this->reconnectBackoff_.reset();
                      });
 }
 
 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;

src/providers/irc/IrcConnection2.hpp

@@ -1,10 +1,11 @@
 #pragma once
 
+#include "util/ExponentialBackoff.hpp"
+
 #include <pajlada/signals/signal.hpp>
 
 #include <IrcConnection>
 #include <QTimer>
-#include <chrono>
 
 namespace chatterino {
 
@@ -28,7 +29,9 @@ private:
     QTimer pingTimer_;
     QTimer reconnectTimer_;
     std::atomic<bool> recentlyReceivedMessage_{true};
-    std::chrono::steady_clock::time_point lastConnected_;
+
+    // Reconnect with a base delay of 1 second and max out at 1 second * (2^4) (i.e. 16 seconds)
+    ExponentialBackoff<4> reconnectBackoff_{std::chrono::milliseconds{1000}};
 
     std::atomic<bool> expectConnectionLoss_{false};
 

src/util/ExponentialBackoff.hpp

@@ -0,0 +1,60 @@
+#pragma once
+
+#include <algorithm>
+#include <chrono>
+
+namespace chatterino {
+
+// Yes, you can't specify the base 😎 deal with it
+template <unsigned maxSteps>
+class ExponentialBackoff
+{
+public:
+    /**
+     * Creates an object helping you make exponentially (with base 2) backed off times.
+     *
+     * @param start The start time in milliseconds
+     * @param maxSteps The max number of progressions we will take before stopping
+     *
+     * For example, ExponentialBackoff(10ms, 3) would have the next() function return 10ms, 20ms, 40ms, 40ms, ..., 40ms
+     **/
+    ExponentialBackoff(const std::chrono::milliseconds &start)
+        : start_(start)
+        , step_{1}
+    {
+        static_assert(maxSteps > 1, "maxSteps must be higher than 1");
+    }
+
+    /**
+     * Return the current number in the progression and increment the step until the next one (assuming we're not at the cap)
+     *
+     * @returns current step in milliseconds
+     **/
+    [[nodiscard]] std::chrono::milliseconds next()
+    {
+        auto next = this->start_ * (1 << (this->step_ - 1));
+
+        this->step_ += 1;
+
+        if (this->step_ >= maxSteps)
+        {
+            this->step_ = maxSteps;
+        }
+
+        return next;
+    }
+
+    /**
+     * Reset the progression back to its initial state
+     **/
+    void reset()
+    {
+        this->step_ = 1;
+    }
+
+private:
+    const std::chrono::milliseconds start_;
+    unsigned step_;
+};
+
+}  // namespace chatterino

tests/CMakeLists.txt

@@ -8,6 +8,7 @@ set(test_SOURCES
     ${CMAKE_CURRENT_LIST_DIR}/src/ChatterSet.cpp
     ${CMAKE_CURRENT_LIST_DIR}/src/HighlightPhrase.cpp
     ${CMAKE_CURRENT_LIST_DIR}/src/Emojis.cpp
+    ${CMAKE_CURRENT_LIST_DIR}/src/ExponentialBackoff.cpp
     )
 
 add_executable(${PROJECT_NAME} ${test_SOURCES})

tests/src/ExponentialBackoff.cpp

@@ -0,0 +1,58 @@
+#include "util/ExponentialBackoff.hpp"
+
+#include <gtest/gtest.h>
+
+using namespace chatterino;
+
+TEST(ExponentialBackoff, MaxSteps)
+{
+    using namespace std::literals::chrono_literals;
+
+    ExponentialBackoff<3> foo{10ms};
+
+    // First usage should be the start value
+    EXPECT_EQ(foo.next(), 10ms);
+    EXPECT_EQ(foo.next(), 20ms);
+    EXPECT_EQ(foo.next(), 40ms);
+    // We reached the max steps, so we should continue returning the max value without increasing
+    EXPECT_EQ(foo.next(), 40ms);
+    EXPECT_EQ(foo.next(), 40ms);
+    EXPECT_EQ(foo.next(), 40ms);
+}
+
+TEST(ExponentialBackoff, Reset)
+{
+    using namespace std::literals::chrono_literals;
+
+    ExponentialBackoff<3> foo{10ms};
+
+    // First usage should be the start value
+    EXPECT_EQ(foo.next(), 10ms);
+    EXPECT_EQ(foo.next(), 20ms);
+    EXPECT_EQ(foo.next(), 40ms);
+    // We reached the max steps, so we should continue returning the max value without increasing
+    EXPECT_EQ(foo.next(), 40ms);
+    EXPECT_EQ(foo.next(), 40ms);
+    EXPECT_EQ(foo.next(), 40ms);
+
+    foo.reset();
+
+    // After a reset, we should start at the beginning value again
+    EXPECT_EQ(foo.next(), 10ms);
+    EXPECT_EQ(foo.next(), 20ms);
+    EXPECT_EQ(foo.next(), 40ms);
+    // We reached the max steps, so we should continue returning the max value without increasing
+    EXPECT_EQ(foo.next(), 40ms);
+    EXPECT_EQ(foo.next(), 40ms);
+    EXPECT_EQ(foo.next(), 40ms);
+}
+
+TEST(ExponentialBackoff, BadMaxSteps)
+{
+    using namespace std::literals::chrono_literals;
+
+    // this will not compile
+    // ExponentialBackoff<1> foo{10ms};
+    // ExponentialBackoff<0> foo{10ms};
+    // ExponentialBackoff<-1> foo{10ms};
+}