diff --git a/NEWS.md b/NEWS.md index 8bc7c2aa..fda042bb 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,20 @@ # NEWS +4.8.4 - UNRELEASED +------------------ + +### Fixed + +- `hackney:close/1` on a shared pooled HTTP/2 connection resets only the + caller's own streams. It stopped the connection, failing every other + caller's streams on it. The connection still closes itself once idle with + no stream open. +- A connection opened without a pool honours `connect_timeout` above 8 + seconds, and a dial that outlives it returns `{error, connect_timeout}`. + The wait was capped at 8 seconds and ended as an exit in the caller. A + dial stuck in the transport no longer holds the caller past its deadline + either (#945). + 4.8.3 - 2026-09-27 ------------------ diff --git a/guides/http_guide.md b/guides/http_guide.md index b0a55104..e0d830f7 100644 --- a/guides/http_guide.md +++ b/guides/http_guide.md @@ -354,7 +354,7 @@ stream_loop(ConnPid) -> hackney:close(ConnPid). ``` -A connection from a pool stays yours between requests; it does not go back to the pool after each response. `hackney:close/1` returns it to the pool when it can be reused, and closes it otherwise. +A connection from a pool stays yours between requests; it does not go back to the pool after each response. `hackney:close/1` returns it to the pool when it can be reused, and closes it otherwise. A pooled HTTP/2 connection is shared with other callers: `hackney:close/1` only resets your own streams on it. ### Hand the Connection to Another Process diff --git a/src/hackney.erl b/src/hackney.erl index 2adf6e1d..c3fe472f 100644 --- a/src/hackney.erl +++ b/src/hackney.erl @@ -147,6 +147,7 @@ connect_direct(Transport, Host, Port, Options) -> undefined -> BaseConnectOpts; _ -> [{protocols, Protocols} | BaseConnectOpts] end, + ConnectTimeout = proplists:get_value(connect_timeout, Options, 8000), ConnOpts = #{ %% The caller owns the connection: it is started under hackney_conn_sup, %% so without this the supervisor is the owner and the connection @@ -155,7 +156,7 @@ connect_direct(Transport, Host, Port, Options) -> host => Host, port => Port, transport => Transport, - connect_timeout => proplists:get_value(connect_timeout, Options, 8000), + connect_timeout => ConnectTimeout, recv_timeout => proplists:get_value(recv_timeout, Options, 5000), %% Single-owner connection: seed the conn-level send_timeout so %% hackney:send_request/2 (which has no options channel) honors it. @@ -167,7 +168,7 @@ connect_direct(Transport, Host, Port, Options) -> }, case hackney_conn_sup:start_conn(ConnOpts) of {ok, ConnPid} -> - case hackney_conn:connect(ConnPid) of + case hackney_conn:connect(ConnPid, ConnectTimeout) of ok -> {ok, ConnPid}; {error, Reason} -> @@ -491,7 +492,9 @@ maybe_upgrade_ssl(_, _ConnPid, _FinalSslOpts) -> %% @private Stop a connection, tolerating an already-dead process. stop_conn(ConnPid) -> - try hackney_conn:stop(ConnPid) catch _:_ -> ok end. + %% Bounded: a conn wedged in a dial past its deadline is killed rather + %% than holding the caller. + try hackney_conn:stop(ConnPid, 100) catch _:_ -> ok end. %% @private Signal the websocket process to shut down, ignoring errors. shutdown_ws(WsPid) -> @@ -500,8 +503,9 @@ shutdown_ws(WsPid) -> %% @doc Close a connection. -spec close(conn()) -> ok. close(ConnPid) when is_pid(ConnPid) -> - %% A pooled connection held by a connect/* caller goes back to its pool; - %% any other one is stopped. + %% A pooled connection held by a connect/* caller goes back to its pool, + %% and a shared HTTP/2 one only drops this caller's streams; any other one + %% is stopped. case hackney_conn:release_held(ConnPid) of ok -> ok; {error, _} -> hackney_conn:stop(ConnPid) diff --git a/src/hackney_conn.erl b/src/hackney_conn.erl index c0387df5..b35fb562 100644 --- a/src/hackney_conn.erl +++ b/src/hackney_conn.erl @@ -320,9 +320,17 @@ kill(Pid) -> connect(Pid) -> connect(Pid, ?CONNECT_TIMEOUT). +%% A dial that outlives `Timeout', or a connection that dies while dialing, +%% is an error for the caller, not an exit: the caller stops the conn on any +%% error return. -spec connect(pid(), timeout()) -> ok | {error, term()}. connect(Pid, Timeout) -> - gen_statem:call(Pid, connect, Timeout). + try gen_statem:call(Pid, connect, Timeout) + catch + exit:{timeout, _} -> {error, connect_timeout}; + exit:{Reason, {gen_statem, call, _}} -> {error, Reason}; + exit:Reason -> {error, Reason} + end. %% @doc Get current state name for debugging. -spec get_state(pid()) -> {ok, atom()} | {error, term()}. @@ -621,8 +629,11 @@ hold(Pid) -> catch exit:_ -> {error, closed} end. -%% @doc Check a held connection back into its pool. Returns ok if it went -%% back, an error if it is not a held, idle pooled connection. +%% @doc Give up the caller's hold on a pooled connection. A held HTTP/1.1 +%% connection goes back to its pool. On a shared HTTP/2 connection only the +%% caller's own streams are reset: the connection stays up for the other +%% callers and closes itself once idle with no stream open. Returns an error +%% for any other connection, which the caller then stops. -spec release_held(pid()) -> ok | {error, term()}. release_held(Pid) -> try gen_statem:call(Pid, release_held, 5000) @@ -940,6 +951,17 @@ connected({call, From}, hold, #conn_data{pool_pid = PoolPid, protocol = http1} = connected({call, From}, hold, _Data) -> {keep_state_and_data, [{reply, From, ok}]}; +connected({call, {Caller, _} = From}, release_held, + #conn_data{h2_shared = true, h2_streams = Streams} = Data) -> + Mine = [StreamId || {StreamId, {Owner, _}} <- maps:to_list(Streams), + h2_stream_owner_pid(Owner) =:= Caller], + Data1 = lists:foldl( + fun(StreamId, D) -> + _ = cancel_h2_stream(D#conn_data.h2_conn, StreamId), + drop_h2_stream(StreamId, D) + end, Data, Mine), + h2_stream_result(Data1, [{reply, From, ok}]); + connected({call, From}, release_held, #conn_data{pool_pid = PoolPid, auto_release = false} = Data) when is_pid(PoolPid) -> connected({call, From}, release_to_pool, Data#conn_data{auto_release = true}); diff --git a/test/hackney_direct_connect_timeout_tests.erl b/test/hackney_direct_connect_timeout_tests.erl new file mode 100644 index 00000000..b4b8c369 --- /dev/null +++ b/test/hackney_direct_connect_timeout_tests.erl @@ -0,0 +1,52 @@ +%%% -*- erlang -*- +%%% +%%% This file is part of hackney released under the Apache 2 license. +%%% See the NOTICE for more information. +%%% +%%% @doc connect_timeout on a connection opened without a pool (#945). +%%% +%%% connect_direct/4 waited on the dial with a hardcoded 8000 ms call, so a +%%% larger connect_timeout was cut to 8 s, and a dial outliving the wait +%%% exited the caller instead of returning an error. +-module(hackney_direct_connect_timeout_tests). + +-include_lib("eunit/include/eunit.hrl"). + +direct_connect_timeout_test_() -> + {setup, + fun() -> {ok, _} = application:ensure_all_started(hackney), ok end, + fun(_) -> hackney_fault_transport:clear() end, + [{"connect_timeout above 8 s is honoured", {timeout, 30, fun t_above_default/0}}, + {"a dial past the deadline is an error", {timeout, 30, fun t_past_deadline/0}}]}. + +%% The dial takes 8.3 s, inside a 9 s connect_timeout: it must succeed. +t_above_default() -> + {ok, L} = gen_tcp:listen(0, [binary, {active, false}, {ip, {127, 0, 0, 1}}]), + {ok, Port} = inet:port(L), + try + ok = hackney_fault_transport:set(connect, {sleep, 8300}), + Result = hackney:connect(hackney_fault_transport, "127.0.0.1", Port, + [{pool, false}, {connect_timeout, 9000}]), + ?assertMatch({ok, _}, Result), + {ok, Conn} = Result, + hackney:close(Conn) + after + hackney_fault_transport:clear(), + gen_tcp:close(L) + end. + +%% The transport never answers: the caller gets an error on its own deadline, +%% not an exit, and is not held until the transport gives up. +t_past_deadline() -> + try + ok = hackney_fault_transport:set(connect, {hang, 1500}), + {Elapsed, Result} = + timer:tc(fun() -> + hackney:connect(hackney_fault_transport, "127.0.0.1", 9, + [{pool, false}, {connect_timeout, 50}]) + end), + ?assertEqual({error, connect_timeout}, Result), + ?assert(Elapsed div 1000 < 800) + after + hackney_fault_transport:clear() + end. diff --git a/test/hackney_http2_shared_conn_tests.erl b/test/hackney_http2_shared_conn_tests.erl index ef44801c..54455017 100644 --- a/test/hackney_http2_shared_conn_tests.erl +++ b/test/hackney_http2_shared_conn_tests.erl @@ -28,7 +28,11 @@ shared_conn_test_() -> {"an unregistered connection still releases its slot", fun unregistered_conn_releases_slot/0}, {"an unpooled connection is not shared", - fun unpooled_conn_not_shared/0} + fun unpooled_conn_not_shared/0}, + {"close/1 by one caller keeps the shared connection", + fun close_keeps_shared_conn/0}, + {"close/1 resets only the caller's own stream", + fun close_resets_own_stream/0} ]]. %%==================================================================== @@ -203,6 +207,46 @@ unpooled_conn_not_shared() -> end end). +%% Another caller holding the shared connection through connect/2 closes +%% it while a stream is in flight: the stream still completes. +close_keeps_shared_conn() -> + with_server([], fun(URL, Port, Opts) -> + _ = sync_request(first, <>, Opts), + {Handler, _} = started(<<"/first">>), + Conn = shared_conn(Opts, Port), + {ok, Conn} = hackney:connect(URL, Opts), + ok = hackney:close(Conn), + ?assert(is_process_alive(Conn)), + Handler ! respond, + ?assertMatch({ok, 200, _, <<"ok">>}, result(first)) + end). + +%% A caller with its own stream open closes the connection: that stream is +%% reset and its monitor dropped, the other caller's stream is untouched. +close_resets_own_stream() -> + with_server([], fun(URL, Port, Opts) -> + _ = sync_request(first, <>, Opts), + {FirstHandler, _} = started(<<"/first">>), + Conn = shared_conn(Opts, Port), + Parent = self(), + Closer = spawn(fun() -> + {ok, Conn} = hackney:connect(URL, Opts), + {ok, _} = hackney_conn:request_async(Conn, <<"GET">>, <<"/second">>, + [], <<>>, true), + receive close -> Parent ! {closed, hackney:close(Conn)} end, + receive after infinity -> ok end + end), + _ = started(<<"/second">>), + ?assert(lists:member(Closer, monitored(Conn))), + Closer ! close, + receive {closed, R} -> ?assertEqual(ok, R) after 5000 -> error(no_close) end, + ?assertNot(lists:member(Closer, monitored(Conn))), + ?assert(is_process_alive(Conn)), + FirstHandler ! respond, + ?assertMatch({ok, 200, _, <<"ok">>}, result(first)), + exit(Closer, kill) + end). + %%==================================================================== %% Helpers %%====================================================================