fix: retry request when error reason is normal

Fixes https://emqx.atlassian.net/browse/EMQX-9656

changelog.md

@@ -1,5 +1,9 @@
 # ehttpc changes
 
+## 0.4.8
+
+- Fix an issue where a race condition would yield an `{error, normal}` return value.  This can be caused when the `gun` process terminates when the remote server closes the connection for whatever reason.  In this case, we simply retry without consuming "retry credits".
+
 ## 0.4.7
 
 - Fix crash when using body headers and no body. When one sent a message with, for example, the content-type header and no body, the underlying gun library would expect a body to come with a gun:data/3 call and would not finish the request. The next request would then crash. See the following issue for more information: https://github.com/ninenines/gun/issues/141

rebar.config

@@ -3,7 +3,7 @@
 {deps, [
     {gun, {git, "https://github.com/emqx/gun", {tag, "1.3.7"}}},
     {gproc, {git, "https://github.com/uwiger/gproc", {tag, "0.9.0"}}},
-    {snabbkaffe, {git, "https://github.com/kafka4beam/snabbkaffe.git", {tag, "1.0.3"}}}
+    {snabbkaffe, {git, "https://github.com/kafka4beam/snabbkaffe.git", {tag, "1.0.7"}}}
 ]}.
 
 {erl_opts, [

src/ehttpc.app.src

@@ -1,6 +1,6 @@
 {application, ehttpc,
  [{description, "HTTP Client for Erlang/OTP"},
-  {vsn, "0.4.7"},
+  {vsn, "0.4.8"},
   {registered, []},
   {applications, [kernel,
                   stdlib,

src/ehttpc.appup.src

@@ -1,6 +1,9 @@
-%% -*-: erlang -*-
-{"0.4.7",
+%% -*- mode: erlang -*-
+{"0.4.8",
    [
+     {"0.4.7", [
+       {load_module, ehttpc, brutal_purge, soft_purge, []}
+     ]},
      {"0.4.6", [
        {load_module, ehttpc, brutal_purge, soft_purge, []}
      ]},
@@ -47,6 +50,9 @@
      ]}
    ],
    [
+    {"0.4.7", [
+       {load_module, ehttpc, brutal_purge, soft_purge, []}
+     ]},
      {"0.4.6", [
        {load_module, ehttpc, brutal_purge, soft_purge, []}
      ]},

src/ehttpc.erl

@@ -141,6 +141,15 @@ request(Worker, Method, Request, Timeout, Retry) when is_pid(Worker) ->
     try gen_server:call(Worker, ?REQ(Method, Request, ExpireAt), Timeout + 500) of
         %% gun will reply {gun_down, _Client, _, normal, _KilledStreams, _} message
         %% when connection closed by keepalive
+
+        %% If `Reason' = `normal', we should just retry without
+        %% consuming a retry credit, as it could be a race condition
+        %% where the gun process is down (e.g.: when the server closes
+        %% the connection), and then requests would be ignored while
+        %% the `gun' process is terminating.
+        {error, normal} ->
+            ?tp(ehttpc_retry_gun_down_normal, #{}),
+            request(Worker, Method, Request, Timeout, Retry);
         {error, Reason} when Retry < 1 ->
             {error, Reason};
         {error, _} ->

test/ehttpc_tests.erl

@@ -759,6 +759,87 @@ no_body_but_headers_indicating_body_test_() ->
         fun() -> ?WITH(ServerOpts, PoolOpts, TestFunction()) end
     ].
 
+gun_down_with_reason_normal_is_retried_test() ->
+    Port = ?PORT,
+    Host = "127.0.0.1",
+    ok = snabbkaffe:start_trace(),
+    with_server(
+        #{
+            port => Port,
+            name => ?FUNCTION_NAME,
+            delay => 1000
+        },
+        fun() ->
+            ?WITH_POOL(
+                pool_opts(Host, Port, _Pipelining = 2),
+                begin
+                    ?force_ordering(
+                        #{?snk_kind := shot},
+                        #{?snk_kind := will_disconnect}
+                    ),
+                    TestPid = self(),
+                    spawn_link(fun() ->
+                        ?tp(will_disconnect, #{}),
+                        [
+                            begin
+                                Ref = monitor(process, P),
+                                sys:terminate(P, normal),
+                                receive
+                                    {'DOWN', Ref, _, _, _} -> ok
+                                after 500 -> ct:fail("gun didn't die")
+                                end,
+                                ok
+                            end
+                         || P <- processes(),
+                            case proc_lib:initial_call(P) of
+                                {gun, proc_lib_hack, _} -> true;
+                                _ -> false
+                            end
+                        ],
+                        ok
+                    end),
+                    spawn_link(fun() ->
+                        Res = ehttpc:request(?POOL, get, req(), 2000, 0),
+                        TestPid ! {result, Res}
+                    end),
+                    Res =
+                        receive
+                            {result, R} -> R
+                        after 2_000 -> ct:fail("no response")
+                        end,
+                    ?assertMatch({ok, 200, _, _}, Res),
+                    ok
+                end
+            )
+        end,
+        fun(Trace0) ->
+            Trace = ?of_kind(
+                [
+                    gun_up,
+                    shot,
+                    handle_client_down,
+                    ehttpc_retry_gun_down_normal
+                ],
+                Trace0
+            ),
+            ?assertMatch(
+                %% first attempt
+                [
+                    gun_up,
+                    shot,
+                    handle_client_down,
+                    %% we retry because of this particularly unusual race condition
+                    ehttpc_retry_gun_down_normal,
+                    gun_up,
+                    shot
+                ],
+                ?projection(?snk_kind, Trace)
+            ),
+            ok
+        end
+    ),
+    ok.
+
 ensure_not_error_response({error, _Reason} = Error) ->
     Error;
 ensure_not_error_response(_) ->