From 2d61269bdb3de1eaf2383c9971b5418646c8cc46 Mon Sep 17 00:00:00 2001 From: Benoit Chesneau Date: Tue, 21 Jul 2026 07:45:06 +0200 Subject: [PATCH] Guard get_state liveness probe during pooled H2/H3 checkout (#914) Reusing a pooled HTTP/2 or HTTP/3 connection probes it with hackney_conn:get_state/1, a bare gen_statem:call. If the connection is terminating at that instant (idle teardown, GOAWAY, keepalive close) the call exits, and the surrounding case only handled return values, so the exit crashed the caller of hackney:connect/4 instead of falling through to a fresh connection. Wrap both checkout probes in try/catch exit, mirroring the existing race handling in maybe_register_h2 and maybe_upgrade_ssl. A terminating connection is now treated as unusable and the checkout opens a fresh one. --- src/hackney.erl | 20 ++++++-- test/hackney_connect_race_tests.erl | 76 +++++++++++++++++++++++++++++ test/hackney_race_pool.erl | 66 +++++++++++++++++++++++++ 3 files changed, 158 insertions(+), 4 deletions(-) create mode 100644 test/hackney_connect_race_tests.erl create mode 100644 test/hackney_race_pool.erl 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).