diff --git a/src/hackney.erl b/src/hackney.erl index e5daf1d8..7c5e229b 100644 --- a/src/hackney.erl +++ b/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}; _ -> diff --git a/test/hackney_connect_race_tests.erl b/test/hackney_connect_race_tests.erl new file mode 100644 index 00000000..eafce91d --- /dev/null +++ b/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. diff --git a/test/hackney_race_pool.erl b/test/hackney_race_pool.erl new file mode 100644 index 00000000..517f1117 --- /dev/null +++ b/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).