Guard get_state liveness probe during pooled H2/H3 checkout (#914)

src/hackney.erl

@@ -196,8 +196,15 @@ connect_pool(Transport, Host, Port, Options) ->
           case PoolHandler:checkout_h2(Host, Port, Transport, Options2) of
             {ok, H2Pid} ->
               %% Verify connection is actually in connected state
-              %% (OTP 28 on FreeBSD may have timing issues with SSL connections)
-              case hackney_conn:get_state(H2Pid) of
+              %% (OTP 28 on FreeBSD may have timing issues with SSL connections).
+              %% The probe is a gen_statem:call, which exits if the pooled
+              %% connection is terminating (idle teardown, GOAWAY, keepalive
+              %% close) at checkout time; treat that as unusable and fall
+              %% through to a fresh connection instead of crashing the caller.
+              %% Mirrors maybe_register_h2/maybe_upgrade_ssl.
+              GetState = try hackney_conn:get_state(H2Pid)
+                         catch exit:_ -> {error, terminated} end,
+              case GetState of
                 {ok, connected} ->
                           {ok, H2Pid};
                 _ ->
@@ -247,8 +254,13 @@ try_h3_connection(Host, Port, Transport, Options, PoolHandler) ->
       %% Check if we have an existing HTTP/3 connection
       case PoolHandler:checkout_h3(Host, Port, Transport, Options2) of
         {ok, H3Pid} ->
-          %% Verify connection is actually in connected state
-          case hackney_conn:get_state(H3Pid) of
+          %% Verify connection is actually in connected state. The probe is a
+          %% gen_statem:call, which exits if the pooled connection is
+          %% terminating at checkout time; treat that as unusable and fall
+          %% through to a fresh connection instead of crashing the caller.
+          GetState = try hackney_conn:get_state(H3Pid)
+                     catch exit:_ -> {error, terminated} end,
+          case GetState of
             {ok, connected} ->
               {ok, H3Pid};
             _ ->

test/hackney_connect_race_tests.erl

@@ -0,0 +1,76 @@
+%%% Regression tests for issue #914.
+%%%
+%%% When a pooled HTTP/2 or HTTP/3 connection terminates during the checkout
+%%% get_state liveness probe, the probe's gen_statem:call exits. That exit must
+%%% be caught so checkout falls through to a fresh connection instead of
+%%% crashing the caller of hackney:connect/4.
+-module(hackney_connect_race_tests).
+
+-include_lib("eunit/include/eunit.hrl").
+
+race_test_() ->
+    {setup, fun setup/0, fun teardown/1, fun tests/1}.
+
+setup() ->
+    {ok, _} = application:ensure_all_started(hackney),
+    Prev = application:get_env(hackney, pool_handler),
+    DeadPid = dead_pid(),
+    application:set_env(hackney, race_dead_pid, DeadPid),
+    application:set_env(hackney, pool_handler, hackney_race_pool),
+    {Prev, DeadPid}.
+
+teardown({Prev, _DeadPid}) ->
+    case Prev of
+        {ok, Handler} -> application:set_env(hackney, pool_handler, Handler);
+        undefined -> application:unset_env(hackney, pool_handler)
+    end,
+    application:unset_env(hackney, race_dead_pid),
+    ok.
+
+tests({_Prev, DeadPid}) ->
+    [{"h2 checkout survives a terminating pooled connection",
+      fun() -> h2_no_crash(DeadPid) end},
+     {"h3 checkout get_state probe exit does not escape",
+      fun() -> h3_probe_does_not_escape(DeadPid) end}].
+
+%% A guaranteed-dead pid: spawn, wait for the DOWN, then it is gone. No timing.
+dead_pid() ->
+    {Pid, Ref} = spawn_monitor(fun() -> ok end),
+    receive {'DOWN', Ref, process, Pid, _} -> ok end,
+    Pid.
+
+%% H2 fallback is a plain TCP connect, so a closed local port lets the whole
+%% call return cleanly: the checkout probe exits, is caught, and the fresh
+%% connection to the closed port fails with {error, _} instead of crashing.
+h2_no_crash(DeadPid) ->
+    ?assertNot(is_process_alive(DeadPid)),
+    Port = closed_port(),
+    Result = connect([http2], Port),
+    %% Buggy: EXIT {noproc,{gen_statem,call,[_,get_state|_]}}.
+    %% Fixed:  {error, _} from the fresh-connection fallback.
+    ?assertMatch({error, _}, Result).
+
+%% H3's fresh-connection fallback drives a real QUIC handshake (a separate
+%% path), so here we assert only the #914 property: the get_state probe exit
+%% must not escape hackney:connect/4.
+h3_probe_does_not_escape(DeadPid) ->
+    ?assertNot(is_process_alive(DeadPid)),
+    Port = closed_port(),
+    Result = connect([http3], Port),
+    ?assertNotMatch({exit, {_, {gen_statem, call, [_, get_state | _]}}}, Result).
+
+closed_port() ->
+    {ok, L} = gen_tcp:listen(0, [{ip, {127, 0, 0, 1}}]),
+    {ok, Port} = inet:port(L),
+    ok = gen_tcp:close(L),
+    Port.
+
+connect(Protocols, Port) ->
+    try
+        hackney:connect(hackney_ssl, "127.0.0.1", Port,
+                        [{protocols, Protocols},
+                         {pool, default},
+                         {connect_timeout, 500}])
+    catch
+        Class:Reason -> {Class, Reason}
+    end.

test/hackney_race_pool.erl

@@ -0,0 +1,66 @@
+%%% Test pool handler for issue #914.
+%%%
+%%% checkout_h2/4 and checkout_h3/4 return an already-terminated pid so the
+%%% checkout get_state liveness probe races connection teardown. Every other
+%%% callback delegates to hackney_pool, so the new-connection fallback behaves
+%%% exactly as in production.
+-module(hackney_race_pool).
+
+-export([checkout/4,
+         checkin/2,
+         checkout_ssl/4,
+         checkout_h2/4,
+         register_h2/5,
+         unregister_h2/2,
+         unregister_h2_all/0,
+         checkout_h3/4,
+         register_h3/5,
+         unregister_h3/2,
+         get_h3_session/4,
+         store_h3_session/5,
+         delete_h3_session/4]).
+
+%% The dead pid to hand out is stashed in application env by the test setup.
+dead_pid() ->
+    {ok, Pid} = application:get_env(hackney, race_dead_pid),
+    Pid.
+
+checkout_h2(_Host, _Port, _Transport, _Options) ->
+    {ok, dead_pid()}.
+
+checkout_h3(_Host, _Port, _Transport, _Options) ->
+    {ok, dead_pid()}.
+
+%% Everything else delegates unchanged.
+checkout(Host, Port, Transport, Options) ->
+    hackney_pool:checkout(Host, Port, Transport, Options).
+
+checkin(Ref, Options) ->
+    hackney_pool:checkin(Ref, Options).
+
+checkout_ssl(Host, Port, Transport, Options) ->
+    hackney_pool:checkout_ssl(Host, Port, Transport, Options).
+
+register_h2(Host, Port, Transport, Pid, Options) ->
+    hackney_pool:register_h2(Host, Port, Transport, Pid, Options).
+
+unregister_h2(Pid, Options) ->
+    hackney_pool:unregister_h2(Pid, Options).
+
+unregister_h2_all() ->
+    hackney_pool:unregister_h2_all().
+
+register_h3(Host, Port, Transport, Pid, Options) ->
+    hackney_pool:register_h3(Host, Port, Transport, Pid, Options).
+
+unregister_h3(Pid, Options) ->
+    hackney_pool:unregister_h3(Pid, Options).
+
+get_h3_session(Host, Port, Transport, Options) ->
+    hackney_pool:get_h3_session(Host, Port, Transport, Options).
+
+store_h3_session(Host, Port, Transport, Session, Options) ->
+    hackney_pool:store_h3_session(Host, Port, Transport, Session, Options).
+
+delete_h3_session(Host, Port, Transport, Options) ->
+    hackney_pool:delete_h3_session(Host, Port, Transport, Options).