Fault injection and chaos tests for the pool

DEVELOPMENT.md

@@ -66,6 +66,64 @@ gunicorn -b 127.0.0.1:8000 httpbin:app &
 rebar3 eunit
 ```
 
+### Fault injection and chaos tests
+
+Use these when you change the pool or the connection state machine. Ordinary
+integration tests only exercise servers that answer, so they never see a
+connection that stalls, crashes, or dies at the wrong moment, which is where
+pool failures come from: the pool dials and stops connections from inside its
+own gen_server, and an unguarded call that exits takes the pool down along with
+every caller using it.
+
+Three pieces make up the harness:
+
+| Module | What it does |
+| --- | --- |
+| `hackney_fault_transport` | A transport that behaves like `hackney_tcp` until you arm a fault on one of its callbacks |
+| `hackney_crash_sentinel` | Captures crash reports so a test can assert a process survived, even with `error_logger:tty(false)` |
+| `hackney_pool_safety_tests` | Walks the compiled abstract code and fails on any call into `hackney_conn` that is not inside a `try` |
+
+Arm a fault, drive the code path, assert the pool is untouched:
+
+```erlang
+ok = hackney_crash_sentinel:start(),
+hackney_fault_transport:set(connect, {slow_error, 300}),
+Opts = [{pool, my_pool}, {connect_timeout, 30}],
+{error, connect_timeout} =
+    hackney_pool:checkout("127.0.0.1", 8080, hackney_fault_transport, Opts),
+hackney_crash_sentinel:assert_no_crash_from(hackney_pool:find_pool(my_pool)),
+ok = hackney_fault_transport:clear().
+```
+
+Available faults: `{sleep, Ms}`, `{slow_error, Ms}`, `{hang, Ms}`, `{error, Reason}`,
+`crash`. Any callback can be armed: `connect`, `send`, `recv`, `setopts`,
+`close`, `controlling_process`.
+
+Run the fault matrix, the multiplexed (HTTP/2, HTTP/3) checkout faults, and the
+randomized chaos run:
+
+```bash
+rebar3 eunit --module=hackney_pool_fault_tests
+rebar3 eunit --module=hackney_pool_h2h3_fault_tests
+rebar3 eunit --module=hackney_pool_chaos_tests
+```
+
+The HTTP/2 and HTTP/3 connections are shared rather than checked out, so one
+bad connection is felt by every caller for that host. Those scenarios wedge a
+registered connection with `sys:suspend/1` and require the pool to answer
+`none` within the probe budget instead of waiting on it.
+
+Soak the chaos run harder, for example before a release:
+
+```bash
+HACKNEY_CHAOS_WORKERS=64 HACKNEY_CHAOS_ROUNDS=2000 \
+    rebar3 eunit --module=hackney_pool_chaos_tests
+```
+
+If `hackney_pool_safety_tests` fails, route the new call through a guarded
+helper in `hackney_pool` (`connect_connection/2`, `set_owner/2`, `stop_conn/1`,
+`checkin_info/1`) rather than relaxing the check.
+
 ## Local Docker Testing
 
 A Dockerfile is provided for testing on Linux locally, which mirrors the GitHub CI environment.

src/hackney_conn.erl

@@ -26,9 +26,11 @@
 -export([
     start_link/1,
     stop/1,
+    stop/2,
     connect/1,
     connect/2,
     get_state/1,
+    get_state/2,
     %% Request/Response (sync)
     request/5,
     request/6,
@@ -72,15 +74,18 @@
     release_to_pool/1,
     verify_socket/1,
     is_ready/1,
+    is_ready/2,
     %% SSL upgrade
     upgrade_to_ssl/2,
     upgrade_to_ssl/3,
     is_upgraded_ssl/1,
     %% Reuse control
     is_no_reuse/1,
     checkin_info/1,
+    checkin_info/2,
     %% Owner management
     set_owner/2,
+    set_owner/3,
     set_owner_async/2,
     %% Protocol info
     get_protocol/1
@@ -270,15 +275,31 @@ start_link(Opts) when is_map(Opts) ->
 %% Returns ok even if the process has already terminated.
 -spec stop(pid()) -> ok.
 stop(Pid) ->
+    stop(Pid, infinity).
+
+%% @doc Stop the connection process, waiting at most `Timeout' for it.
+%% A connection wedged inside a transport call (a dial that outlived its
+%% timeout, a socket that never answers) cannot handle a stop request; it is
+%% killed rather than left holding the caller, which for pool checkouts is the
+%% pool gen_server itself.
+%% Returns ok even if the process has already terminated.
+-spec stop(pid(), timeout()) -> ok.
+stop(Pid, Timeout) ->
     try
-        gen_statem:stop(Pid)
+        gen_statem:stop(Pid, normal, Timeout)
     catch
         exit:noproc -> ok;
         exit:{noproc, _} -> ok;
         exit:normal -> ok;
-        exit:{normal, _} -> ok
+        exit:{normal, _} -> ok;
+        exit:timeout -> kill(Pid);
+        exit:{timeout, _} -> kill(Pid)
     end.
 
+kill(Pid) ->
+    exit(Pid, kill),
+    ok.
+
 %% @doc Connect to the target host. Blocks until connected or timeout.
 -spec connect(pid()) -> ok | {error, term()}.
 connect(Pid) ->
@@ -291,7 +312,15 @@ connect(Pid, Timeout) ->
 %% @doc Get current state name for debugging.
 -spec get_state(pid()) -> {ok, atom()} | {error, term()}.
 get_state(Pid) ->
-    gen_statem:call(Pid, get_state).
+    get_state(Pid, 5000).
+
+%% @doc Get current state name, waiting at most `Timeout'. Callers that probe
+%% a connection they do not own (the pool, deciding whether to hand it out)
+%% pass a short timeout: a connection that cannot answer promptly is unusable
+%% to them, and waiting on it blocks everything behind them.
+-spec get_state(pid(), timeout()) -> {ok, atom()} | {error, term()}.
+get_state(Pid, Timeout) ->
+    gen_statem:call(Pid, get_state, Timeout).
 
 %% @doc Send an HTTP request and wait for the response status and headers.
 %% Returns {ok, Status, Headers} for HTTP/1.1 or {ok, Status, Headers, Body} for HTTP/2.
@@ -571,7 +600,12 @@ release_to_pool(Pid) ->
 %% a connection to a new requester.
 -spec set_owner(pid(), pid()) -> ok | {error, invalid_state}.
 set_owner(Pid, NewOwner) ->
-    gen_statem:call(Pid, {set_owner, NewOwner}, 5000).
+    set_owner(Pid, NewOwner, 5000).
+
+%% @doc Set a new owner, waiting at most `Timeout'. @see get_state/2
+-spec set_owner(pid(), pid(), timeout()) -> ok | {error, invalid_state}.
+set_owner(Pid, NewOwner, Timeout) ->
+    gen_statem:call(Pid, {set_owner, NewOwner}, Timeout).
 
 %% @doc Set a new owner for this connection (async).
 %% Same as set_owner/2 but non-blocking. Used when the caller cannot
@@ -591,7 +625,12 @@ verify_socket(Pid) ->
 %% This combines state check and socket verification in one call.
 -spec is_ready(pid()) -> {ok, connected} | {ok, closed} | {error, term()}.
 is_ready(Pid) ->
-    gen_statem:call(Pid, is_ready).
+    is_ready(Pid, 5000).
+
+%% @doc Check socket health, waiting at most `Timeout'. @see get_state/2
+-spec is_ready(pid(), timeout()) -> {ok, connected} | {ok, closed} | {error, term()}.
+is_ready(Pid, Timeout) ->
+    gen_statem:call(Pid, is_ready, Timeout).
 
 %% @doc Upgrade a TCP connection to SSL.
 %% This performs an SSL handshake on the existing TCP socket.
@@ -632,7 +671,12 @@ is_no_reuse(Pid) ->
                                pool_ssl := boolean(), protocol := atom(),
                                should_close := boolean(), ready := boolean()}.
 checkin_info(Pid) ->
-    gen_statem:call(Pid, checkin_info).
+    checkin_info(Pid, 5000).
+
+%% @doc Checkin flags, waiting at most `Timeout'. @see get_state/2
+-spec checkin_info(pid(), timeout()) -> map().
+checkin_info(Pid, Timeout) ->
+    gen_statem:call(Pid, checkin_info, Timeout).
 
 %% @doc Get the negotiated protocol for this connection.
 %% Returns http1, http2, or http3 based on ALPN negotiation (SSL connections),

src/hackney_conn_sup.erl

@@ -41,9 +41,11 @@ start_conn(Opts) ->
     supervisor:start_child(?SERVER, [Opts]).
 
 %% @doc Stop a connection process gracefully.
+%% Tolerates a connection that is already gone or that dies while stopping:
+%% callers are only asking for it to be off.
 -spec stop_conn(pid()) -> ok.
 stop_conn(Pid) ->
-    hackney_conn:stop(Pid).
+    try hackney_conn:stop(Pid) catch _:_ -> ok end.
 
 %% @doc Stop all connection processes gracefully.
 %% Useful for test cleanup.

src/hackney_pool.erl

@@ -92,6 +92,13 @@
 -define(DEFAULT_MAX_CONNECTIONS, 50).
 -define(DEFAULT_KEEPALIVE_TIMEOUT, 2000).  % 2 seconds max idle
 -define(DEFAULT_PREWARM_COUNT, 4).         % Connections to maintain per host
+-define(STOP_CONN_TIMEOUT, 100).           % Max wait for a conn to stop
+-define(PREWARM_CONNECT_TIMEOUT, 5000).    % Dial budget for a prewarm conn
+%% Every question the pool asks a conn about its own health is answered from
+%% the conn's state, so a healthy conn answers at once. A conn that does not
+%% is wedged, and waiting on it from inside the pool gen_server blocks every
+%% caller of the pool, not just the one that asked: treat slow as unusable.
+-define(PROBE_TIMEOUT, 250).
 
 start() ->
     %% Create ETS table to store pool pid by name
@@ -544,7 +551,7 @@ handle_call({checkout, Key, Requester, Opts}, _From, State) ->
         {ok, Pid, Available2} ->
             %% Found an available connection - update owner to new requester
             ?report_debug("pool: reusing connection", [{pool, PoolName}, {pid, Pid}]),
-            case hackney_conn:set_owner(Pid, Requester) of
+            case set_owner(Pid, Requester) of
                 ok ->
                     InUse2 = maps:put(Pid, Key, InUse),
                     {reply, {ok, Pid}, State#state{available=Available2, in_use=InUse2}};
@@ -589,7 +596,7 @@ handle_call({checkout_ssl, SslKey, Requester, Opts}, _From, State) ->
         {ok, Pid, Available2} ->
             %% Found a pooled SSL connection with the same TLS options hash
             ?report_debug("pool: reusing ssl connection", [{pool, PoolName}, {pid, Pid}]),
-            case hackney_conn:set_owner(Pid, Requester) of
+            case set_owner(Pid, Requester) of
                 ok ->
                     InUse2 = maps:put(Pid, SslKey, InUse),
                     {reply, {ok, Pid, ready},
@@ -889,9 +896,13 @@ h3_connection_key(Host0, Port, Transport, Options) ->
     Host = string:lowercase(Host0),
     {Host, Port, Transport, proplists:get_value(h3_tls_key, Options, default)}.
 
-%% @private Stop a connection, tolerating an already-dead process.
+%% @private Stop a connection, tolerating an already-dead process. Bounded on
+%% purpose: this runs inside the pool gen_server, and a conn wedged in a
+%% transport call (the dial that just outlived its timeout, typically) would
+%% otherwise hold every caller of the pool for as long as the transport takes
+%% to return. Past the deadline the conn is killed.
 stop_conn(Pid) ->
-    try hackney_conn:stop(Pid) catch _:_ -> ok end.
+    try hackney_conn:stop(Pid, ?STOP_CONN_TIMEOUT) catch _:_ -> ok end.
 
 %% @private Find a reusable idle connection for `Key', discarding any that are
 %% no longer keepalive-ready. Only a conn that is_ready reports `{ok, connected}'
@@ -912,7 +923,7 @@ find_available(Key, Available) ->
                     %% The connection can die between is_process_alive/1 above
                     %% and this gen_statem call (flaky network); the resulting
                     %% noproc exit must not crash the pool, so skip and move on.
-                    try hackney_conn:is_ready(Pid) of
+                    try hackney_conn:is_ready(Pid, ?PROBE_TIMEOUT) of
                         {ok, connected} ->
                             {ok, Pid, Available2};
                         _ ->
@@ -952,7 +963,7 @@ checkout_ssl_fallback(SslKey, Requester, Opts, State) ->
 
     case find_available(TcpKey, Available) of
         {ok, Pid, Available2} ->
-            case hackney_conn:set_owner(Pid, Requester) of
+            case set_owner(Pid, Requester) of
                 ok ->
                     InUse2 = maps:put(Pid, SslKey, InUse),
                     {reply, {ok, Pid, needs_upgrade},
@@ -1014,7 +1025,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 +1039,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
@@ -1088,11 +1116,21 @@ checkin_poolable(_TcpKey, Info) ->
         maps:get(upgraded_ssl, Info, false) =:= false andalso
         keepalive_ready(Info).
 
+%% @private Hand a pooled conn to its new owner. Called from inside the pool
+%% gen_server, so it must not raise: the conn answered `is_ready' a moment ago
+%% but can be gone or wedged by now, and either would take the pool down. Any
+%% failure means the conn is unusable; every caller already dials a fresh one
+%% on `{error, _}'.
+set_owner(Pid, Owner) ->
+    try hackney_conn:set_owner(Pid, Owner, ?PROBE_TIMEOUT)
+    catch _:_ -> {error, set_owner_failed}
+    end.
+
 %% @private Fetch the conn's checkin flags, or `error' if the call fails (the
 %% conn died between is_process_alive/1 and here). Caller treats `error' as
 %% not poolable.
 checkin_info(Pid) ->
-    try {ok, hackney_conn:checkin_info(Pid)}
+    try {ok, hackney_conn:checkin_info(Pid, ?PROBE_TIMEOUT)}
     catch _:_ -> error
     end.
 
@@ -1205,7 +1243,7 @@ h2_conn_usable(Pid) ->
     case erlang:is_process_alive(Pid) of
         false -> false;
         true ->
-            try hackney_conn:get_state(Pid) of
+            try hackney_conn:get_state(Pid, ?PROBE_TIMEOUT) of
                 {ok, connected} -> true;
                 _ -> false
             catch
@@ -1333,7 +1371,7 @@ prewarm_connections(PoolPid, Host, Port, Count, IdleTimeout) ->
     },
     case hackney_conn_sup:start_conn(ConnOpts) of
         {ok, Pid} ->
-            case hackney_conn:connect(Pid) of
+            case connect_connection(Pid, ?PREWARM_CONNECT_TIMEOUT) of
                 ok ->
                     %% Checkin the new connection to the pool
                     gen_server:cast(PoolPid, {prewarm_checkin, Pid,