Merge pull request #928 from aboroska/fix-slow-connect-crashing-pool

Fix slow connect crashing the connection pool

Author: benoitc

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

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
 %%====================================================================