diff --git a/libs/server-sent-events/src/CMakeLists.txt b/libs/server-sent-events/src/CMakeLists.txt index be6d9d304..87106db90 100644 --- a/libs/server-sent-events/src/CMakeLists.txt +++ b/libs/server-sent-events/src/CMakeLists.txt @@ -11,6 +11,7 @@ add_library(${LIBNAME} OBJECT parser.cpp event.cpp error.cpp + backoff_detail.cpp backoff.cpp) target_link_libraries(${LIBNAME} diff --git a/libs/server-sent-events/src/backoff.cpp b/libs/server-sent-events/src/backoff.cpp index c3f2a0091..92afd979a 100644 --- a/libs/server-sent-events/src/backoff.cpp +++ b/libs/server-sent-events/src/backoff.cpp @@ -1,22 +1,14 @@ #include "backoff.hpp" +#include "backoff_detail.hpp" -static double const kDefaultJitterRatio = 0.5; -static const std::chrono::milliseconds kDefaultResetInterval = - std::chrono::milliseconds{60'000}; +static constexpr double kDefaultJitterRatio = 0.5; +static constexpr auto kDefaultResetInterval = std::chrono::milliseconds{60'000}; namespace launchdarkly::sse { -std::chrono::milliseconds Backoff::calculate_backoff() { - return initial_ * static_cast(std::pow(2, attempt_ - 1)); -} - -std::chrono::milliseconds Backoff::jitter(std::chrono::milliseconds base) { - return std::chrono::milliseconds(static_cast( - base.count() - (random_(jitter_ratio_) * base.count()))); -} - -std::chrono::milliseconds Backoff::delay() { - return jitter(std::min(calculate_backoff(), max_)); +std::chrono::milliseconds Backoff::delay() const { + return detail::delay(initial_, max_, attempt_, max_exponent_, jitter_ratio_, + random_); } void Backoff::fail() { @@ -50,12 +42,20 @@ Backoff::Backoff(std::chrono::milliseconds initial, double jitter_ratio, std::chrono::milliseconds reset_interval, std::function random) - : attempt_(1), - initial_(initial), + : initial_(initial), max_(max), + // This is necessary to constrain the exponent that is passed eventually + // into std::pow. Otherwise, we'll be operating with a number that is too + // large to be represented as a chrono::milliseconds and (depending on the + // platform) may result in a value of 0, negative, or some other + // unexpected value. + max_exponent_(std::ceil( + std::log2(max.count() / std::max(std::chrono::milliseconds{1}.count(), + initial.count())))), + attempt_(1), jitter_ratio_(jitter_ratio), reset_interval_(reset_interval), - random_(random) {} + random_(std::move(random)) {} Backoff::Backoff(std::chrono::milliseconds initial, std::chrono::milliseconds max) @@ -68,5 +68,4 @@ Backoff::Backoff(std::chrono::milliseconds initial, 0.0, kDefaultJitterRatio); return distribution(this->random_gen_); }) {} - } // namespace launchdarkly::sse diff --git a/libs/server-sent-events/src/backoff.hpp b/libs/server-sent-events/src/backoff.hpp index 9e8f8b75b..1f2d206a5 100644 --- a/libs/server-sent-events/src/backoff.hpp +++ b/libs/server-sent-events/src/backoff.hpp @@ -6,7 +6,6 @@ #include namespace launchdarkly::sse { - /** * Implements an algorithm for calculating the delay between connection * attempts. @@ -52,35 +51,18 @@ class Backoff { /** * Get the current reconnect delay. */ - std::chrono::milliseconds delay(); + [[nodiscard]] std::chrono::milliseconds delay() const; private: - /** - * Calculate an exponential backoff based on the initial retry time and - * the current attempt. - * - * @param attempt The current attempt count. - * @param initial The initial retry delay. - * @return The current backoff based on the number of attempts. - */ - std::chrono::milliseconds calculate_backoff(); - - /** - * Produce a jittered version of the base value. This value will be adjusted - * randomly to be between 50% and 100% of the input duration. - * - * @param base The base duration to jitter. - * @return The jittered duration. - */ - std::chrono::milliseconds jitter(std::chrono::milliseconds base); - std::chrono::milliseconds initial_; std::chrono::milliseconds max_; - uint64_t attempt_; + + std::uint64_t max_exponent_; + std::uint64_t attempt_; std::optional> active_since_; double const jitter_ratio_; - const std::chrono::milliseconds reset_interval_; + std::chrono::milliseconds const reset_interval_; // Default random generator. Used when random method not specified. std::function random_; std::default_random_engine random_gen_; diff --git a/libs/server-sent-events/src/backoff_detail.cpp b/libs/server-sent-events/src/backoff_detail.cpp new file mode 100644 index 000000000..4e27bb359 --- /dev/null +++ b/libs/server-sent-events/src/backoff_detail.cpp @@ -0,0 +1,35 @@ +#include "backoff_detail.hpp" + +#include +#include + +namespace launchdarkly::sse::detail { +std::chrono::milliseconds calculate_backoff( + std::chrono::milliseconds const initial, + std::uint64_t const attempt, + std::uint64_t const max_exponent) { + std::uint64_t const exponent = std::min(attempt - 1, max_exponent); + double const exponentiated = std::pow(2, exponent); + return initial * static_cast(exponentiated); +} + +std::chrono::milliseconds jitter(std::chrono::milliseconds const base, + double const jitter_ratio, + std::function const& random) { + double const jitter = random(jitter_ratio); + auto const base_as_double = static_cast(base.count()); + double const jittered_base = base_as_double - jitter * base_as_double; + return std::chrono::milliseconds(static_cast(jittered_base)); +} + +std::chrono::milliseconds delay(std::chrono::milliseconds const initial, + std::chrono::milliseconds const max, + std::uint64_t const attempt, + std::uint64_t const max_exponent, + double const jitter_ratio, + std::function const& random) { + auto const backoff = calculate_backoff(initial, attempt, max_exponent); + auto const constrained_backoff = std::min(backoff, max); + return jitter(constrained_backoff, jitter_ratio, random); +} +} // namespace launchdarkly::sse::detail diff --git a/libs/server-sent-events/src/backoff_detail.hpp b/libs/server-sent-events/src/backoff_detail.hpp new file mode 100644 index 000000000..5ce448282 --- /dev/null +++ b/libs/server-sent-events/src/backoff_detail.hpp @@ -0,0 +1,52 @@ +#pragma once + +#include +#include + +namespace launchdarkly::sse::detail { +/** + * Calculate an exponential backoff based on the initial retry time and + * the current attempt. + * + * @param initial The initial retry delay. + * @param attempt The current attempt count. + * @param max_exponent The maximum exponent to use in the calculation. + * + * @return The current backoff based on the number of attempts. + */ + +std::chrono::milliseconds calculate_backoff(std::chrono::milliseconds initial, + std::uint64_t attempt, + std::uint64_t max_exponent); +/** + * Produce a jittered version of the base value. This value will be adjusted + * randomly to be between 50% and 100% of the input duration. + * + * @param base The base duration to jitter. + * @param jitter_ratio The ratio to use when jittering. + * @param random A random method which produces a value between 0 and + * jitter_ratio. + * @return The jittered duration. + */ +std::chrono::milliseconds jitter(std::chrono::milliseconds base, + double jitter_ratio, + std::function const& random); + +/** + * + * @param initial The initial retry delay. + * @param max The maximum delay between retries. + * @param attempt The current attempt. + * @param max_exponent The maximum exponent to use in the calculation. + * @param jitter_ratio The ratio to use when jittering. + * @param random A random method which produces a value between 0 and + * jitter_ratio. + * @return The delay between connection attempts. + */ +std::chrono::milliseconds delay(std::chrono::milliseconds initial, + std::chrono::milliseconds max, + std::uint64_t attempt, + std::uint64_t max_exponent, + double jitter_ratio, + std::function const& random); +} // namespace launchdarkly::sse::detail diff --git a/libs/server-sent-events/tests/backoff_test.cpp b/libs/server-sent-events/tests/backoff_test.cpp index 71f1ecd24..e9fc0f450 100644 --- a/libs/server-sent-events/tests/backoff_test.cpp +++ b/libs/server-sent-events/tests/backoff_test.cpp @@ -1,8 +1,10 @@ #include +#include #include #include "backoff.hpp" +#include "backoff_detail.hpp" using launchdarkly::sse::Backoff; @@ -94,3 +96,103 @@ TEST(BackoffTests, BackoffDoesNotResetActiveConnectionWasTooShort) { EXPECT_EQ(8000, backoff.delay().count()); } + +// This test specifically attempts to reproduce an overflow bug reported by a +// customer that occurred using default backoff parameters after 55 attempts. +TEST(BackoffTests, BackoffDoesNotOverflowAt55Attempts) { + Backoff backoff( + std::chrono::milliseconds{1000}, std::chrono::milliseconds{30000}, 0, + std::chrono::milliseconds{6000}, [](auto ratio) { return 0; }); + + constexpr std::size_t kAttemptsToReachMax = 5; + constexpr std::size_t kAttemptWhereOverflowOccurs = 54; + + for (int i = 0; i < kAttemptsToReachMax; i++) { + backoff.fail(); + } + // At 5 attempts, we've already reached maximum backoff, given the initial + // parameters. + EXPECT_EQ(std::chrono::milliseconds{30000}, backoff.delay()); + + // This should succeed even if the code was unprotected. + for (int i = 0; i < kAttemptWhereOverflowOccurs - kAttemptsToReachMax - 1; + i++) { + backoff.fail(); + EXPECT_EQ(std::chrono::milliseconds{30000}, backoff.delay()); + } + + // This should trigger overflow in unprotected code. + backoff.fail(); + EXPECT_EQ(std::chrono::milliseconds{30000}, backoff.delay()); +} + +class BackoffOverflowEndToEndTest + : public ::testing::TestWithParam {}; + +// The purpose of this test is to simulate the SDK calling backoff.fail() +// over and over again. +// +// Given a large amount of failed attempts, the calculation should remain at +// the max backoff value and stay there. +// +// Since we can't simulate very large numbers of attempts (simply takes too +// long), the free functions that Backoff delegates to are tested separately. +TEST_P(BackoffOverflowEndToEndTest, BackoffDoesNotOverflowWithVariousAttempts) { + Backoff backoff( + std::chrono::milliseconds{1000}, std::chrono::milliseconds{30000}, 0, + std::chrono::milliseconds{60000}, [](auto ratio) { return 0; }); + + std::uint64_t const attempts = GetParam(); + + for (int i = 0; i < attempts; i++) { + backoff.fail(); + } + + auto const val = backoff.delay(); + EXPECT_EQ(val, std::chrono::milliseconds{30000}); +} + +INSTANTIATE_TEST_SUITE_P( + VariousBackoffAttempts, + BackoffOverflowEndToEndTest, + ::testing::Values(10, 54, 55, 63, 64, 65, 500, 1000)); + +class BackoffOverflowUnitTest : public ::testing::TestWithParam { +}; + +// This test performs a straight calculation of the backoff given +// a range of large values, including the maximum uint64_t which might be +// reached at the heat death of the universe (or earlier, if there's a bug.) +// It should not overflow, but remain at the max value. +TEST_P(BackoffOverflowUnitTest, + BackoffDoesNotOverflowWithVariousAttemptsInCalculateBackoff) { + std::uint64_t const attempts = GetParam(); + + constexpr std::uint64_t max_exponent = 5; + + // Given an exponent of 5, and initial delay of 1ms, the max should be + // 2^5 = 32 for any amount of attempts > 5. + + constexpr auto initial = std::chrono::milliseconds{1}; + constexpr auto max = std::chrono::milliseconds{32}; + + auto const millisecondsA = launchdarkly::sse::detail::calculate_backoff( + initial, attempts, max_exponent); + + ASSERT_EQ(millisecondsA, max); + + // Given a random function that always returns 0, this should be identical + // to calling calculate_backoff. + auto const millisecondsB = launchdarkly::sse::detail::delay( + initial, max, attempts, max_exponent, 0, [](auto ratio) { return 0; }); + + ASSERT_EQ(millisecondsB, max); +} + +INSTANTIATE_TEST_SUITE_P(VariousBackoffAttempts, + BackoffOverflowUnitTest, + ::testing::Values( + 1000, + 10000, + 100000, + std::numeric_limits::max()));