ninenines / gun

HTTP/1.1, HTTP/2, Websocket client (and more) for Erlang/OTP.
ISC License
891 stars 232 forks source link

Support a function as retry_timeout #200

Closed tony612 closed 4 years ago

tony612 commented 5 years ago

Support pass a function as retry_timeout so that we can support more flexible timeout like exponential backoff.

essen commented 4 years ago

Probably worth returning both timeout and number of retries left instead, maybe wrapped in a map.

essen commented 4 years ago

I also would rather pass the options to the fun as well, and use a separate option for the function.

essen commented 4 years ago

Something like this maybe:

diff --git a/src/gun.erl b/src/gun.erl
index bb275f5..cf6e2ef 100644
--- a/src/gun.erl
+++ b/src/gun.erl
@@ -116,6 +116,7 @@
    http2_opts => http2_opts(),
    protocols => [http | http2],
    retry => non_neg_integer(),
+   retry_fun => fun((non_neg_integer(), opts()) -> #{retries => non_neg_integer(), timeout => pos_integer()}),
    retry_timeout => pos_integer(),
    supervise => boolean(),
    tcp_opts => [gen_tcp:connect_option()],
@@ -770,15 +771,25 @@ default_transport(_) -> tcp.
 %% @todo This is where we would implement the backoff mechanism presumably.
 not_connected(_, {retries, 0, Reason}, State) ->
    {stop, {shutdown, Reason}, State};
-not_connected(_, {retries, Retries, _}, State=#state{opts=Opts}) ->
-   Timeout = maps:get(retry_timeout, Opts, 5000),
+not_connected(_, {retries, Retries0, _}, State=#state{opts=Opts}) ->
+   Fun = maps:get(retry_fun, Opts, fun default_retry_fun/2),
+   #{
+       timeout := Timeout,
+       retries := Retries
+   } = Fun(Retries0, Opts),
    {next_state, domain_lookup, State,
-       {state_timeout, Timeout, {retries, Retries - 1, not_connected}}};
+       {state_timeout, Timeout, {retries, Retries, not_connected}}};
 not_connected({call, From}, {stream_info, _}, _) ->
    {keep_state_and_data, {reply, From, {error, not_connected}}};
 not_connected(Type, Event, State) ->
    handle_common(Type, Event, ?FUNCTION_NAME, State).

+default_retry_fun(Retries, Opts) ->
+   #{
+       timeout => maps:get(retry_timeout, Opts, 5000),
+       retries => Retries - 1
+   }.
+
tony612 commented 4 years ago

@essen I solved the conflicts and updated the code as you said. Tests will be added later.

Updates: Just noticed your example code. I'll think about it later.

essen commented 4 years ago

If my snippet is good for you I'll continue on my own with this.

tony612 commented 4 years ago

@essen How about adding a field for current retry times(or total retries)? I'm OK with your snippet.

essen commented 4 years ago

The fun receives the current number of retries and the options (including retry/retry_timeout) and returns the next number of retries and the timeout to use. I think this includes everything you need?

essen commented 4 years ago

It also allows you to have infinite retries, just never return 0.

tony612 commented 4 years ago

@essen

I think this includes everything you need?

Yes. Thank you!

essen commented 4 years ago

Pushed. Thanks!