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 73ec3243..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). @@ -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 handle_common's closed-state fallback, then stop. case PoolPid of undefined -> {keep_state, Data#conn_data{socket = undefined}}; @@ -2002,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/test/hackney_conn_tests.erl b/test/hackney_conn_tests.erl index 0f9abb73..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,15 +260,15 @@ 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 -%% 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)), 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). @@ -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},