Reply with {error, closed} instead of {error, illegal_state} on calls to closed connection

This fixes #932

Author: kpy3

src/hackney_conn.erl

@@ -674,7 +674,7 @@ checkin_info(Pid) ->
     checkin_info(Pid, 5000).
 
 %% @doc Checkin flags, waiting at most `Timeout'. @see get_state/2
--spec checkin_info(pid(), timeout()) -> map().
+-spec checkin_info(pid(), timeout()) -> map() | {error, term()}.
 checkin_info(Pid, Timeout) ->
     gen_statem:call(Pid, checkin_info, 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 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).
 

src/hackney_pool.erl

@@ -1127,10 +1127,12 @@ 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 hackney_conn:checkin_info(Pid, ?PROBE_TIMEOUT) of
+        Info when is_map(Info) -> {ok, Info};
+        _ -> error
     catch _:_ -> error
     end.
 

test/hackney_conn_tests.erl

@@ -256,15 +256,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
+%% {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).