From 0c2488505ca83a6c8cb9b867cf138396b77e76a2 Mon Sep 17 00:00:00 2001 From: pajlada Date: Sun, 20 Jun 2021 15:29:52 +0200 Subject: [PATCH] Use an exponential backoff when deciding how long we need to wait for reconnects (#2892) Co-authored-by: Leon Richardt --- CHANGELOG.md | 2 +- src/providers/irc/IrcConnection2.cpp | 19 ++------- src/providers/irc/IrcConnection2.hpp | 7 +++- src/util/ExponentialBackoff.hpp | 60 ++++++++++++++++++++++++++++ tests/CMakeLists.txt | 1 + tests/src/ExponentialBackoff.cpp | 58 +++++++++++++++++++++++++++ 6 files changed, 129 insertions(+), 18 deletions(-) create mode 100644 src/util/ExponentialBackoff.hpp create mode 100644 tests/src/ExponentialBackoff.cpp diff --git a/CHANGELOG.md b/CHANGELOG.md index 552a03ada..653cb9f49 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/providers/irc/IrcConnection2.cpp b/src/providers/irc/IrcConnection2.cpp index 916cf544e..ff52fa847 100644 --- a/src/providers/irc/IrcConnection2.cpp +++ b/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 = - 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); + auto delay = this->reconnectBackoff_.next(); + qCDebug(chatterinoIrc) << "Reconnecting in" << delay.count() << "ms"; + this->reconnectTimer_.start(delay); }); - 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; diff --git a/src/providers/irc/IrcConnection2.hpp b/src/providers/irc/IrcConnection2.hpp index 930b5dc4a..d3426c029 100644 --- a/src/providers/irc/IrcConnection2.hpp +++ b/src/providers/irc/IrcConnection2.hpp @@ -1,10 +1,11 @@ #pragma once +#include "util/ExponentialBackoff.hpp" + #include #include #include -#include namespace chatterino { @@ -28,7 +29,9 @@ private: QTimer pingTimer_; QTimer reconnectTimer_; std::atomic 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 expectConnectionLoss_{false}; diff --git a/src/util/ExponentialBackoff.hpp b/src/util/ExponentialBackoff.hpp new file mode 100644 index 000000000..44eca67e1 --- /dev/null +++ b/src/util/ExponentialBackoff.hpp @@ -0,0 +1,60 @@ +#pragma once + +#include +#include + +namespace chatterino { + +// Yes, you can't specify the base 😎 deal with it +template +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 diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 8a13c128d..52e8560f7 100644 --- a/tests/CMakeLists.txt +++ b/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}) diff --git a/tests/src/ExponentialBackoff.cpp b/tests/src/ExponentialBackoff.cpp new file mode 100644 index 000000000..2a4259744 --- /dev/null +++ b/tests/src/ExponentialBackoff.cpp @@ -0,0 +1,58 @@ +#include "util/ExponentialBackoff.hpp" + +#include + +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}; +}