From 1ee1952316110c3dfd705e33bd393fbd550b05cb Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Mon, 25 Sep 2023 16:02:07 -0700 Subject: [PATCH 1/2] fix: Fix arithmetic error in stream reconnection logic. --- src/ldclient_backoff.erl | 23 ++++++++++---- test/ldclient_backoff_SUITE.erl | 56 +++++++++++++++++++++++++++++++-- 2 files changed, 70 insertions(+), 9 deletions(-) diff --git a/src/ldclient_backoff.erl b/src/ldclient_backoff.erl index efbfd02..26fae60 100644 --- a/src/ldclient_backoff.erl +++ b/src/ldclient_backoff.erl @@ -18,7 +18,8 @@ attempt => non_neg_integer(), active_since => integer() | undefined, destination => pid(), - value => term() + value => term(), + max_exp => non_neg_integer() }. -define(JITTER_RATIO, 0.5). @@ -35,14 +36,21 @@ -spec init(Initial :: non_neg_integer(), Max :: non_neg_integer(), Destination :: pid(), Value :: term()) -> backoff(). init(Initial, Max, Destination, Value) -> + SafeInitial = lists:max([Initial, 1]), #{ - initial => Initial, - current => Initial, + initial => SafeInitial, %% Do not allow initial delay to be 0 or negative. + current => SafeInitial, max => Max, attempt => 0, active_since => undefined, destination => Destination, - value => Value + value => Value, + %% The exponent at which the backoff delay will exceed the maximum. + %% Beyond this limit the backoff can be set to the max. + max_exp => math:ceil(math:log2(Max/SafeInitial)) + %% For reasonable values this should ensure we never overflow. + %% Note that while integers can be arbitrarily large the math library uses C functions + %% that are implemented with floats. }. %% @doc Get an updated backoff with updated delay. Does not start a timer automatically. @@ -90,8 +98,11 @@ update_backoff(#{attempt := Attempt} = Backoff, _ActiveDuration) -> Backoff#{current => delay(NewAttempt, Backoff), attempt => NewAttempt, active_since => undefined}. -spec delay(Attempt :: non_neg_integer(), Backoff :: backoff()) -> non_neg_integer(). -delay(Attempt, #{initial := Initial, max := Max} = _Backoff) -> - jitter(min(backoff(Initial, Attempt), Max)). +delay(Attempt, #{initial := Initial, max := Max, max_exp := MaxExp} = _Backoff) + when Attempt - 1 < MaxExp -> + jitter(min(backoff(Initial, Attempt), Max)); +delay(_Attempt, #{max := Max} = _Backoff) -> + jitter(Max). -spec backoff(Initial :: non_neg_integer(), Attempt :: non_neg_integer()) -> non_neg_integer(). backoff(Initial, Attempt) -> diff --git a/test/ldclient_backoff_SUITE.erl b/test/ldclient_backoff_SUITE.erl index a50fbf4..2b9a5da 100644 --- a/test/ldclient_backoff_SUITE.erl +++ b/test/ldclient_backoff_SUITE.erl @@ -17,7 +17,11 @@ jitters_backoff_value/1, resets_after_60_second_connection/1, does_not_reset_after_less_than_60_connection/1, - creates_timer_when_fired/1 + creates_timer_when_fired/1, + handles_max_exponent_correctly/1, + handles_initial_greater_than_max/1, + handles_initial_equal_to_max/1, + handles_bad_initial_retry/1 ]). %%==================================================================== @@ -32,7 +36,11 @@ all() -> jitters_backoff_value, resets_after_60_second_connection, does_not_reset_after_less_than_60_connection, - creates_timer_when_fired + creates_timer_when_fired, + handles_max_exponent_correctly, + handles_initial_greater_than_max, + handles_initial_equal_to_max, + handles_bad_initial_retry ]. init_per_suite(Config) -> @@ -79,12 +87,20 @@ delay_starts_at_initial(_) -> delay_doubles_consecutive_failures(_) -> Backoff = backoff_client(), + %% This is the full cycle for the defaults of initial 1000 and max 30000. FirstUpdate = ldclient_backoff:fail(Backoff), #{current := 1000} = FirstUpdate, SecondUpdate = ldclient_backoff:fail(FirstUpdate), #{current := 2000} = SecondUpdate, ThirdUpdate = ldclient_backoff:fail(SecondUpdate), - #{current := 4000} = ThirdUpdate. + #{current := 4000} = ThirdUpdate, + FourthUpdate = ldclient_backoff:fail(ThirdUpdate), + #{current := 8000} = FourthUpdate, + FifthUpdate = ldclient_backoff:fail(FourthUpdate), + #{current := 16000} = FifthUpdate, + SixthUpdate = ldclient_backoff:fail(FifthUpdate), + #{current := 30000} = SixthUpdate. + backoff_respects_max(_) -> Backoff = @@ -130,3 +146,37 @@ creates_timer_when_fired(_) -> Backoff = ldclient_backoff:fail(backoff_client()), ldclient_backoff:fire(Backoff), true = meck:validate(ldclient_time). + +handles_max_exponent_correctly(_) -> + Backoff = backoff_client(), + %% 2^4 should be the step before we hit the max + 16000 = ldclient_backoff:delay(5, Backoff), + %% 2^5 and higher should just use the max. + 30000 = ldclient_backoff:delay(6, Backoff), + %% trunc(1000 * :math.pow(2, 1015)) would have an arithmetic error + 30000 = ldclient_backoff:delay(1016, Backoff). + +handles_initial_equal_to_max(_) -> + Backoff = backoff_client(30000), + 30000 = ldclient_backoff:delay(1, Backoff), + 30000 = ldclient_backoff:delay(2, Backoff). + +handles_initial_greater_than_max(_) -> + Backoff = backoff_client(60000), + 30000 = ldclient_backoff:delay(1, Backoff), + 30000 = ldclient_backoff:delay(2, Backoff). + +handles_bad_initial_retry(_) -> + Backoff = backoff_client(0), + 1 = ldclient_backoff:delay(1, Backoff), + 2 = ldclient_backoff:delay(2, Backoff), + 4 = ldclient_backoff:delay(3, Backoff), + 16384 = ldclient_backoff:delay(15, Backoff), + 30000 = ldclient_backoff:delay(16, Backoff), + %% Second backoff we do not use backoff_client as meck is already setup. + Backoff2 = ldclient_backoff:init(-100, 30000, 0, listen), + 1 = ldclient_backoff:delay(1, Backoff2), + 2 = ldclient_backoff:delay(2, Backoff2), + 4 = ldclient_backoff:delay(3, Backoff2), + 16384 = ldclient_backoff:delay(15, Backoff2), + 30000 = ldclient_backoff:delay(16, Backoff2). From f114cda4a5c44463bc5654802ae88e055627ce3c Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Mon, 25 Sep 2023 16:08:47 -0700 Subject: [PATCH 2/2] fix dialyzer error --- src/ldclient_backoff.erl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ldclient_backoff.erl b/src/ldclient_backoff.erl index 26fae60..4927b3e 100644 --- a/src/ldclient_backoff.erl +++ b/src/ldclient_backoff.erl @@ -19,7 +19,7 @@ active_since => integer() | undefined, destination => pid(), value => term(), - max_exp => non_neg_integer() + max_exp => float() }. -define(JITTER_RATIO, 0.5).