From 2907b431bba57beb43086d789954281c69c0fd66 Mon Sep 17 00:00:00 2001 From: Sergey Yelin Date: Mon, 17 Aug 2026 16:11:10 +0300 Subject: [PATCH 1/2] Reply with {error, closed} instead of {error, illegal_state} on calls to closed connection This fixes #932 --- src/hackney_conn.erl | 12 ++++++++++-- src/hackney_pool.erl | 6 +++--- test/hackney_conn_tests.erl | 4 ++-- 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/hackney_conn.erl b/src/hackney_conn.erl index 73ec3243..29e72b19 100644 --- a/src/hackney_conn.erl +++ b/src/hackney_conn.erl @@ -1824,8 +1824,8 @@ closed(enter, _OldState, #conn_data{socket = Socket, transport = Transport, pool %% late-arriving {call, From, {request, _}} messages from workers that %% raced the pool checkout race a terminating gen_statem — which %% surfaces as `exit:{normal, _}` in the caller (issue #836). Stay - %% alive briefly so those late calls get a proper `{error, {closed, _}}` - %% reply via handle_common's invalid_state fallback, then stop. + %% alive briefly so those late calls get a proper `{error, closed}` + %% reply via the dedicated closed/3 catch-all clause below, then stop. case PoolPid of undefined -> {keep_state, Data#conn_data{socket = undefined}}; @@ -1879,6 +1879,14 @@ closed(cast, {set_owner, _NewOwner}, #conn_data{pool_pid = PoolPid} = Data) %% of lingering through the grace window and being handed out again. {stop, normal, Data}; +closed({call, From}, _Msg, _Data) -> + %% Any other synchronous call arriving during the grace window (request, + %% request_async, send_headers, body, stream_body, etc.) gets a proper + %% `{error, closed}` instead of the generic `{error, invalid_state}` from + %% handle_common. This lets callers distinguish a peer-closed connection + %% from a misuse of the API. + {keep_state_and_data, [{reply, From, {error, closed}}]}; + closed(EventType, Event, Data) -> handle_common(EventType, Event, closed, Data). diff --git a/src/hackney_pool.erl b/src/hackney_pool.erl index b48ce0bc..2655c346 100644 --- a/src/hackney_pool.erl +++ b/src/hackney_pool.erl @@ -1127,10 +1127,10 @@ set_owner(Pid, Owner) -> 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. +%% conn died between is_process_alive/1 and here) or in closed state (grace window). +%% Caller treats `error' as not poolable. checkin_info(Pid) -> - try {ok, hackney_conn:checkin_info(Pid, ?PROBE_TIMEOUT)} + try {ok, #{} = hackney_conn:checkin_info(Pid, ?PROBE_TIMEOUT)} catch _:_ -> error end. diff --git a/test/hackney_conn_tests.erl b/test/hackney_conn_tests.erl index 0f9abb73..d454c7f3 100644 --- a/test/hackney_conn_tests.erl +++ b/test/hackney_conn_tests.erl @@ -256,7 +256,7 @@ test_owner_death() -> %% #850: when a checkout races a server-side close, the pool calls set_owner on %% a connection that has just transitioned to `closed`. It must get -%% {error, invalid_state} back (so the pool can fall through to a fresh +%% {error, closed} back (so the pool can fall through to a fresh %% connection) rather than crash. A non-pooled connection has no grace timer, %% so it stays in `closed` to answer. test_set_owner_closed_returns_error() -> @@ -264,7 +264,7 @@ test_set_owner_closed_returns_error() -> ?assertEqual({ok, connected}, hackney_conn:get_state(Pid)), ok = hackney_conn:close(Pid), ?assertEqual({ok, closed}, hackney_conn:get_state(Pid)), - ?assertEqual({error, invalid_state}, hackney_conn:set_owner(Pid, self())), + ?assertEqual({error, closed}, hackney_conn:set_owner(Pid, self())), hackney_conn:stop(Pid), gen_tcp:close(ListenSock). From 6534921bc8bea9429e7b869e68166cba00aff26e Mon Sep 17 00:00:00 2001 From: Benoit Chesneau Date: Mon, 31 Aug 2026 12:28:01 +0200 Subject: [PATCH 2/2] Scope the closed-state error to calls that had no handler The catch-all added in the previous commit sits ahead of handle_common, so in `closed` it also answered response_headers, get_location, get_protocol, is_upgraded_ssl, is_no_reuse, checkin_info and close_socket with {error, closed}. That breaks hackney:location/1, which feeds response_headers straight to hackney_headers, and made the pool match a map out of a try to keep checkin working. Reply from handle_common's fallback keyed on the closed state instead: only calls with no handler change, and the accessors the pool and hackney read after a response keep answering. Reverts the hackney_pool workaround, widens the set_owner specs, and adds tests for the request path #932 reports and for the accessors. --- NEWS.md | 13 +++++++++ src/hackney_conn.erl | 22 +++++++-------- src/hackney_pool.erl | 6 ++--- test/hackney_conn_tests.erl | 54 +++++++++++++++++++++++++++++++++---- 4 files changed, 76 insertions(+), 19 deletions(-) diff --git a/NEWS.md b/NEWS.md index 9187c4d8..575033f7 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,18 @@ # NEWS +unreleased +---------- + +### Fixed + +- A request that races a peer-initiated close now returns `{error, closed}` + instead of `{error, invalid_state}`. A connection that sees the peer close + stays alive briefly so late calls get an answer, and during that window every + call without a handler answered with the generic `invalid_state`. The same + race therefore had two answers: `{error, closed}` once the connection process + was gone, `{error, invalid_state}` while it lingered. Callers can now tell a + closed connection from a misuse of the API (#932, #933, thanks @kpy3). + 4.7.4 - 2026-08-12 ------------------ diff --git a/src/hackney_conn.erl b/src/hackney_conn.erl index 29e72b19..ca54fb99 100644 --- a/src/hackney_conn.erl +++ b/src/hackney_conn.erl @@ -598,12 +598,12 @@ release_to_pool(Pid) -> %% This updates the process being monitored - if the new owner crashes, %% the connection will terminate. Used by the pool when checking out %% a connection to a new requester. --spec set_owner(pid(), pid()) -> ok | {error, invalid_state}. +-spec set_owner(pid(), pid()) -> ok | {error, closed | invalid_state}. set_owner(Pid, NewOwner) -> 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}. +-spec set_owner(pid(), pid(), timeout()) -> ok | {error, closed | invalid_state}. set_owner(Pid, NewOwner, Timeout) -> gen_statem:call(Pid, {set_owner, NewOwner}, Timeout). @@ -1825,7 +1825,7 @@ closed(enter, _OldState, #conn_data{socket = Socket, transport = Transport, pool %% raced the pool checkout race a terminating gen_statem — which %% surfaces as `exit:{normal, _}` in the caller (issue #836). Stay %% alive briefly so those late calls get a proper `{error, closed}` - %% reply via the dedicated closed/3 catch-all clause below, then stop. + %% reply via handle_common's closed-state fallback, then stop. case PoolPid of undefined -> {keep_state, Data#conn_data{socket = undefined}}; @@ -1879,14 +1879,6 @@ closed(cast, {set_owner, _NewOwner}, #conn_data{pool_pid = PoolPid} = Data) %% of lingering through the grace window and being handed out again. {stop, normal, Data}; -closed({call, From}, _Msg, _Data) -> - %% Any other synchronous call arriving during the grace window (request, - %% request_async, send_headers, body, stream_body, etc.) gets a proper - %% `{error, closed}` instead of the generic `{error, invalid_state}` from - %% handle_common. This lets callers distinguish a peer-closed connection - %% from a misuse of the API. - {keep_state_and_data, [{reply, From, {error, closed}}]}; - closed(EventType, Event, Data) -> handle_common(EventType, Event, closed, Data). @@ -2010,6 +2002,14 @@ handle_common({call, From}, checkin_info, _State, Data) -> handle_common({call, From}, get_protocol, _State, #conn_data{protocol = Protocol}) -> {keep_state_and_data, [{reply, From, Protocol}]}; +handle_common({call, From}, _, closed, _Data) -> + %% #932: a call that reaches a connection already in `closed' (a request + %% that raced a peer-initiated close after checkout) is a closed + %% connection, not a misuse of the API. Reply with the same + %% `{error, closed}' safe_call/3 gives when the process is already gone, + %% so the race has one answer instead of two. + {keep_state_and_data, [{reply, From, {error, closed}}]}; + handle_common({call, From}, _, _State, _Data) -> {keep_state_and_data, [{reply, From, {error, invalid_state}}]}; diff --git a/src/hackney_pool.erl b/src/hackney_pool.erl index 2655c346..b48ce0bc 100644 --- a/src/hackney_pool.erl +++ b/src/hackney_pool.erl @@ -1127,10 +1127,10 @@ set_owner(Pid, Owner) -> end. %% @private Fetch the conn's checkin flags, or `error' if the call fails (the -%% conn died between is_process_alive/1 and here) or in closed state (grace window). -%% Caller treats `error' as not poolable. +%% 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, ?PROBE_TIMEOUT)} + try {ok, hackney_conn:checkin_info(Pid, ?PROBE_TIMEOUT)} catch _:_ -> error end. diff --git a/test/hackney_conn_tests.erl b/test/hackney_conn_tests.erl index d454c7f3..026d79f7 100644 --- a/test/hackney_conn_tests.erl +++ b/test/hackney_conn_tests.erl @@ -29,14 +29,18 @@ hackney_conn_test_() -> {"connect timeout", fun test_connect_timeout/0}, {"connect to invalid host", fun test_connect_invalid/0}, {"owner death stops connection", fun test_owner_death/0}, - {"set_owner on closed connection returns invalid_state (#850)", + {"set_owner on closed connection returns closed (#850, #932)", fun test_set_owner_closed_returns_error/0}, {"set_owner_async stops a closed pooled connection (#850)", fun test_set_owner_async_closed_pooled_stops/0}, {"request on a dead connection returns {error, closed} (#861)", fun test_request_dead_conn_returns_error/0}, {"body on a dead connection returns {error, closed} (#861)", - fun test_body_dead_conn_returns_error/0} + fun test_body_dead_conn_returns_error/0}, + {"request on a connection in closed returns {error, closed} (#932)", + fun test_request_closed_conn_returns_closed/0}, + {"accessors still answer in closed (#932)", + fun test_closed_conn_accessors_still_answer/0} ]}. %% Integration tests - use embedded Cowboy server @@ -256,9 +260,9 @@ test_owner_death() -> %% #850: when a checkout races a server-side close, the pool calls set_owner on %% a connection that has just transitioned to `closed`. It must get -%% {error, closed} back (so the pool can fall through to a fresh -%% connection) rather than crash. A non-pooled connection has no grace timer, -%% so it stays in `closed` to answer. +%% {error, closed} back (so the pool can fall through to a fresh connection) +%% rather than crash. A non-pooled connection has no grace timer, so it stays +%% in `closed` to answer. test_set_owner_closed_returns_error() -> {Pid, ListenSock} = connected_conn(#{}), ?assertEqual({ok, connected}, hackney_conn:get_state(Pid)), @@ -285,6 +289,46 @@ test_set_owner_async_closed_pooled_stops() -> %% #861: a pooled connection can stop between checkout and the call, so a %% request to an already-dead connection must return {error, closed} rather %% than letting exit:{normal,_}/noproc crash the caller. +%% #932: a request that races a peer-initiated close lands on a connection +%% that is still alive in `closed' (the #836 grace window). It used to get the +%% generic {error, invalid_state}, so the same race answered {error, closed} +%% when the process had already stopped and {error, invalid_state} when it had +%% not. Both answers are now {error, closed}. +test_request_closed_conn_returns_closed() -> + {Pid, ListenSock} = connected_conn(#{}), + ok = hackney_conn:close(Pid), + ?assertEqual({ok, closed}, hackney_conn:get_state(Pid)), + ?assertEqual({error, closed}, + hackney_conn:request(Pid, <<"GET">>, <<"/">>, [], <<>>)), + ?assertEqual({error, closed}, hackney_conn:body(Pid)), + hackney_conn:stop(Pid), + gen_tcp:close(ListenSock). + +%% #932: only calls with no handler answer {error, closed}. The accessors the +%% pool and hackney read after a response, which is when the peer's close +%% typically lands, must keep answering. `hackney:location/1' feeds +%% response_headers straight to hackney_headers, so an error tuple there +%% crashes it. +test_closed_conn_accessors_still_answer() -> + {Pid, ListenSock} = connected_conn(#{}), + ok = hackney_conn:close(Pid), + ?assertEqual({ok, closed}, hackney_conn:get_state(Pid)), + ?assertEqual(undefined, hackney_conn:response_headers(Pid)), + %% reads response_headers when no location is stored + ?assertEqual(undefined, hackney:location(Pid)), + ?assertEqual(ok, hackney_conn:set_location(Pid, <<"http://127.0.0.1/x">>)), + ?assertEqual(<<"http://127.0.0.1/x">>, hackney_conn:get_location(Pid)), + ?assertEqual(<<"http://127.0.0.1/x">>, hackney:location(Pid)), + ?assertEqual(http1, hackney_conn:get_protocol(Pid)), + ?assertEqual(false, hackney_conn:is_upgraded_ssl(Pid)), + ?assertEqual(false, hackney_conn:is_no_reuse(Pid)), + Info = hackney_conn:checkin_info(Pid), + ?assert(is_map(Info)), + ?assertEqual(false, maps:get(ready, Info)), + ?assertEqual(ok, hackney_conn:close(Pid)), + hackney_conn:stop(Pid), + gen_tcp:close(ListenSock). + test_request_dead_conn_returns_error() -> Pid = dead_conn_pid(), ?assertEqual({error, closed},