diff --git a/src/hackney_pool.erl b/src/hackney_pool.erl index d96e07a0..5575104f 100644 --- a/src/hackney_pool.erl +++ b/src/hackney_pool.erl @@ -1014,7 +1014,7 @@ start_connection(Host, Port, Transport, Owner, Opts, State) -> case hackney_conn_sup:start_conn(ConnOpts) of {ok, Pid} -> %% Connect the connection - case hackney_conn:connect(Pid) of + case connect_connection(Pid, ConnectTimeout) of ok -> %% Monitor the process MonRef = erlang:monitor(process, Pid), @@ -1028,6 +1028,23 @@ start_connection(Host, Port, Transport, Owner, Opts, State) -> {error, Reason} end. +%% @private Convert a failed connection call into a checkout error. The pool +%% must not terminate because a DNS/TCP/TLS attempt outlives its timeout, nor +%% because the connection process dies while dialing (a transport raising, or +%% the conn being killed). The caller stops the conn on any error return. +connect_connection(Pid, Timeout) -> + try hackney_conn:connect(Pid, Timeout) of + Result -> + Result + catch + exit:{timeout, _} -> + {error, connect_timeout}; + exit:{Reason, {gen_statem, call, _}} -> + {error, Reason}; + exit:Reason -> + {error, Reason} + end. + %% @private Process a checkin - return connection to pool. %% Plain TCP connections are stored under their TCP key. An SSL upgraded %% connection is stored only when it was checked out through checkout_ssl diff --git a/test/hackney_pool_tests.erl b/test/hackney_pool_tests.erl index 0b0ac6cf..c999f370 100644 --- a/test/hackney_pool_tests.erl +++ b/test/hackney_pool_tests.erl @@ -9,6 +9,8 @@ -module(hackney_pool_tests). +-export([connect/4]). + -include_lib("eunit/include/eunit.hrl"). -include("hackney.hrl"). @@ -56,6 +58,10 @@ hackney_pool_integration_test_() -> {"owner crash kills connection", fun test_owner_crash/0}, {"checkin resets owner to pool", fun test_checkin_resets_owner/0}, {"prewarm creates connections", fun test_prewarm/0}, + {"connect timeout does not crash the pool", + fun test_connect_timeout_does_not_crash_pool/0}, + {"connect crash does not crash the pool", + fun test_connect_crash_does_not_crash_pool/0}, {"queue timeout", {timeout, 120, fun test_queue_timeout/0}}, {"checkout timeout", {timeout, 120, fun test_checkout_timeout/0}}, {"stop_pool releases in_use load_regulation slots", @@ -130,6 +136,14 @@ teardown_integration(_) -> error_logger:tty(true), ok. +%% Stub transport: "slow.example" outlives the connect timeout, "crash.example" +%% takes the connection process down while dialing. +connect("slow.example", _Port, _Opts, _Timeout) -> + timer:sleep(100), + {error, simulated_timeout}; +connect("crash.example", _Port, _Opts, _Timeout) -> + erlang:error(simulated_crash). + setup_ssl() -> error_logger:tty(false), {ok, _} = application:ensure_all_started(cowboy), @@ -742,6 +756,24 @@ test_prewarm() -> ok = hackney_pool:stop_pool(test_pool_prewarm). +test_connect_timeout_does_not_crash_pool() -> + PoolName = test_pool_connect_timeout, + ok = hackney_pool:start_pool(PoolName, [{pool_size, 1}, {prewarm_count, 0}]), + Opts = [{pool, PoolName}, {connect_timeout, 10}, {checkout_timeout, 1000}], + ?assertEqual({error, connect_timeout}, + hackney_pool:checkout("slow.example", 443, ?MODULE, Opts)), + ?assert(is_process_alive(hackney_pool:find_pool(PoolName))), + ok = hackney_pool:stop_pool(PoolName). + +test_connect_crash_does_not_crash_pool() -> + PoolName = test_pool_connect_crash, + ok = hackney_pool:start_pool(PoolName, [{pool_size, 1}, {prewarm_count, 0}]), + Opts = [{pool, PoolName}, {connect_timeout, 1000}, {checkout_timeout, 2000}], + ?assertMatch({error, {simulated_crash, _}}, + hackney_pool:checkout("crash.example", 443, ?MODULE, Opts)), + ?assert(is_process_alive(hackney_pool:find_pool(PoolName))), + ok = hackney_pool:stop_pool(PoolName). + %%==================================================================== %% Timeout Tests %%====================================================================