Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Fix arithmetic error in stream re-connection logic. #104

Merged
merged 3 commits into from
Sep 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 17 additions & 6 deletions src/ldclient_backoff.erl
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
attempt => non_neg_integer(),
active_since => integer() | undefined,
destination => pid(),
value => term()
value => term(),
max_exp => float()
}.

-define(JITTER_RATIO, 0.5).
Expand All @@ -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.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to make this a bit more robust, and added some additional tests.

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.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does mean that you can set some initial reconnect that is vastly too large, but I don't think attempting to fix that has much value.

%% 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.
Expand Down Expand Up @@ -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) ->
Expand Down
56 changes: 53 additions & 3 deletions test/ldclient_backoff_SUITE.erl
Original file line number Diff line number Diff line change
Expand Up @@ -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
]).

%%====================================================================
Expand All @@ -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) ->
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -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).