Merge pull request #922 from benoitc/fix/pooled-tls-upgrade-timeout

Bound the pooled TLS upgrade handshake with connect_timeout

Author: benoitc

src/hackney_conn.erl

@@ -898,7 +898,7 @@ connected({call, From}, is_ready, #conn_data{transport = Transport, socket = Soc
 connected({call, From}, {upgrade_to_ssl, _SslOpts, _UpgradeOpts}, #conn_data{transport = hackney_ssl} = _Data) ->
     %% Already SSL - no upgrade needed
     {keep_state_and_data, [{reply, From, ok}]};
-connected({call, From}, {upgrade_to_ssl, SslOpts, UpgradeOpts}, #conn_data{socket = Socket, host = Host, connect_options = ConnectOpts} = Data) ->
+connected({call, From}, {upgrade_to_ssl, SslOpts, UpgradeOpts}, #conn_data{socket = Socket, host = Host, connect_options = ConnectOpts, connect_timeout = HandshakeTimeout} = Data) ->
     %% Upgrade TCP socket to SSL (e.g., after CONNECT proxy tunnel)
     FinalSslOpts = case maps:get(final, UpgradeOpts, false) of
         true ->
@@ -926,7 +926,11 @@ connected({call, From}, {upgrade_to_ssl, SslOpts, UpgradeOpts}, #conn_data{socke
     Resumable = hackney_ssl:auto_tickets(FinalSslOpts),
     Cached = hackney_ssl:recall_alpn(Host, AlpnProtos),
     GatedSslOpts = gate_resumption(FinalSslOpts, Cached),
-    case ssl:connect(Socket, GatedSslOpts) of
+    %% Bound the pooled TLS upgrade handshake with the connection's
+    %% connect_timeout. ssl:connect/2 has no handshake deadline, so a server
+    %% that stalls after TCP accept would pin this process and its pool slot
+    %% indefinitely, past connect_timeout/recv_timeout.
+    case ssl:connect(Socket, GatedSslOpts, HandshakeTimeout) of
         {ok, SslSocket} ->
             %% Detect negotiated protocol, carrying ALPN across resumption
             Protocol = hackney_ssl:negotiated_protocol(SslSocket, Host, AlpnProtos, Cached, Resumable),

test/hackney_conn_upgrade_timeout_tests.erl

@@ -0,0 +1,35 @@
+%%% Pooled TLS upgrade must not hang on a stalled handshake.
+%%%
+%%% ssl:connect/2 has no handshake deadline, so a peer that accepts TCP but
+%%% never completes the TLS handshake would pin the connection process (and its
+%%% pool slot) forever. The upgrade must be bounded by connect_timeout.
+-module(hackney_conn_upgrade_timeout_tests).
+
+-include_lib("eunit/include/eunit.hrl").
+
+pooled_tls_upgrade_times_out_test_() ->
+    %% Without the bound the upgrade never returns and this test times out.
+    {timeout, 10, fun pooled_tls_upgrade_times_out/0}.
+
+pooled_tls_upgrade_times_out() ->
+    {ok, _} = application:ensure_all_started(hackney),
+    %% Listener that accepts the TCP connection but never speaks TLS.
+    {ok, LSock} = gen_tcp:listen(0, [binary, {active, false}, {ip, {127, 0, 0, 1}}]),
+    {ok, Port} = inet:port(LSock),
+    %% Let the conn open (and thus own) its own TCP socket to the stalled peer.
+    Opts = #{host => "127.0.0.1", port => Port, transport => hackney_tcp,
+             connect_timeout => 500},
+    {ok, Pid} = hackney_conn:start_link(Opts),
+    ok = hackney_conn:connect(Pid, 1000),
+    {ok, _ServerSock} = gen_tcp:accept(LSock, 1000),
+    ?assertEqual({ok, connected}, hackney_conn:get_state(Pid)),
+    %% verify_none so the handshake proceeds and then stalls waiting for the
+    %% ServerHello that never arrives; only the timeout can end it.
+    T0 = erlang:monotonic_time(millisecond),
+    Result = hackney_conn:upgrade_to_ssl(Pid, [{verify, verify_none}], #{final => true}),
+    Elapsed = erlang:monotonic_time(millisecond) - T0,
+    ?assertMatch({error, _}, Result),
+    %% 500ms bound with generous slack; a regression hangs instead.
+    ?assert(Elapsed < 4000),
+    catch hackney_conn:stop(Pid),
+    gen_tcp:close(LSock).