From 64d577ebc91bef91fece099191f7bc2cb57ccd53 Mon Sep 17 00:00:00 2001 From: Rasmus Karlsson Date: Sun, 13 Jun 2021 15:51:26 +0200 Subject: [PATCH 1/4] Use an exponential backoff when deciding how long we need to wait for timeouts --- src/providers/irc/IrcConnection2.cpp | 18 ++------- src/providers/irc/IrcConnection2.hpp | 7 +++- src/util/ExponentialBackoff.hpp | 60 ++++++++++++++++++++++++++++ tests/CMakeLists.txt | 1 + tests/src/ExponentialBackoff.cpp | 60 ++++++++++++++++++++++++++++ 5 files changed, 130 insertions(+), 16 deletions(-) create mode 100644 src/util/ExponentialBackoff.hpp create mode 100644 tests/src/ExponentialBackoff.cpp diff --git a/src/providers/irc/IrcConnection2.cpp b/src/providers/irc/IrcConnection2.cpp index 916cf544e19..e8aafb5154a 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,13 @@ 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 930b5dc4a0b..d3426c02922 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 @@ class IrcConnection : public Communi::IrcConnection 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 00000000000..44eca67e1bd --- /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 8a13c128d44..52e8560f7b1 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 00000000000..eeb996a6372 --- /dev/null +++ b/tests/src/ExponentialBackoff.cpp @@ -0,0 +1,60 @@ +#include "util/ExponentialBackoff.hpp" + +#include +#include +#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}; +} From 3a0e9aaeee41b1242a9d83583c29dfbc682d354b Mon Sep 17 00:00:00 2001 From: Rasmus Karlsson Date: Sun, 13 Jun 2021 15:55:47 +0200 Subject: [PATCH 2/4] Update changelog entry --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f28d192e9f5..06496b91b85 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,7 +17,7 @@ - Minor: Switch to Twitch v2 emote API for animated emote support. (#2863) - 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 emotes not loading in some rare cases. (#2856) ## 2.3.2 From 5c39c4d99d0a7517657d18394c8b91a49aedc55a Mon Sep 17 00:00:00 2001 From: Rasmus Karlsson Date: Sun, 13 Jun 2021 16:07:01 +0200 Subject: [PATCH 3/4] Remove unused includes --- tests/src/ExponentialBackoff.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/src/ExponentialBackoff.cpp b/tests/src/ExponentialBackoff.cpp index eeb996a6372..2a4259744a1 100644 --- a/tests/src/ExponentialBackoff.cpp +++ b/tests/src/ExponentialBackoff.cpp @@ -1,8 +1,6 @@ #include "util/ExponentialBackoff.hpp" #include -#include -#include using namespace chatterino; From 2c7e50cc98725ca587a8b3f2b563a43055176c0f Mon Sep 17 00:00:00 2001 From: pajlada Date: Thu, 17 Jun 2021 19:30:33 +0200 Subject: [PATCH 4/4] Update src/providers/irc/IrcConnection2.cpp Co-authored-by: Leon Richardt --- src/providers/irc/IrcConnection2.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/providers/irc/IrcConnection2.cpp b/src/providers/irc/IrcConnection2.cpp index e8aafb5154a..ff52fa8479f 100644 --- a/src/providers/irc/IrcConnection2.cpp +++ b/src/providers/irc/IrcConnection2.cpp @@ -116,7 +116,6 @@ IrcConnection::IrcConnection(QObject *parent) void IrcConnection::open() { - // Accurately track the time a connection was opened this->expectConnectionLoss_ = false; this->waitingForPong_ = false; this->recentlyReceivedMessage_ = false;