From 8ab7c14597f4bf1963b2da9ab8e416d3c6a1dd0b Mon Sep 17 00:00:00 2001 From: Serhii Snozyk Date: Thu, 3 Sep 2026 19:19:23 +0200 Subject: [PATCH] fix: report the close frame as the disconnect reason The close handshake is handled internally, so the code and reason the server sent were dropped: `handle_disconnect/3` only ever saw the transport error from the socket shutting down afterwards, leaving application close codes such as 1008 or the 4000 range unreachable. Remember the frame when the handshake starts and report it instead, falling back to the Mint error when there was no handshake. The frame is cleared on close so a code cannot leak into the next connection's disconnect. `handle_disconnect/3` now receives `{:close, code, reason}` where it previously received `%Mint.TransportError{reason: :closed}` for a connection closed through the handshake by either side. fix: cancel the previous close timer when starting a handshake `send_close/2` overwrote `close_timer` without cancelling it, and `close/1` only cancels the timer it can see. Two handshakes inside the timeout window left the first timer armed, so it fired after the connection had been closed and re-established, and tore down the healthy connection. feat: add the `:close_timeout` option How long to wait for the server to close the connection once a close handshake has started, in milliseconds, defaulting to the 5000 that used to be hardcoded. Also makes the timeout testable: the new test suspends the server so the handshake is never answered. --- lib/minch.ex | 11 +++++++++- lib/minch/conn.ex | 42 +++++++++++++++++++++++++++----------- test/minch/client_test.exs | 18 ++++++++++++---- 3 files changed, 54 insertions(+), 17 deletions(-) diff --git a/lib/minch.ex b/lib/minch.ex index 4663734..6d27a5d 100644 --- a/lib/minch.ex +++ b/lib/minch.ex @@ -8,6 +8,7 @@ defmodule Minch do @type state :: term() @type response :: %{status: Mint.Types.status(), headers: Mint.Types.headers()} @type frame :: Mint.WebSocket.frame() | Mint.WebSocket.shorthand_frame() + @type option :: {:close_timeout, non_neg_integer()} | GenServer.option() @type callback_result :: {:ok, state()} @@ -44,6 +45,9 @@ defmodule Minch do @doc """ Invoked to handle a disconnect from the server or a failed connection attempt. + The reason is the `{:close, code, reason}` frame whenever a close handshake was started by + either side, and a `t:Mint.WebSocket.error/0` otherwise. + Returning `{:reconnect, backoff, state}` will schedule a reconnect after `backoff` milliseconds. """ @callback handle_disconnect(reason :: term(), attempt :: pos_integer(), state()) :: @@ -68,8 +72,13 @@ defmodule Minch do @doc """ Starts a `Minch` client process linked to the current process. + + Accepts `GenServer` options and: + + * `:close_timeout` - how long to wait, in milliseconds, for the server to close the + connection after a close handshake has started. Defaults to `5000`. """ - @spec start_link(module(), term(), GenServer.options()) :: GenServer.on_start() + @spec start_link(module(), term(), [option()]) :: GenServer.on_start() def start_link(module, init_arg, opts \\ []) do Minch.Conn.start_link(module, init_arg, opts) end diff --git a/lib/minch/conn.ex b/lib/minch/conn.ex index 3e67371..dd5d5c6 100644 --- a/lib/minch/conn.ex +++ b/lib/minch/conn.ex @@ -13,19 +13,24 @@ defmodule Minch.Conn do :callback, :callback_state, :reconnect_timer, - :close_timer + :close_timer, + :close_frame, + :close_timeout ] + @options [:close_timeout] @internal :"$minch" - @spec start_link(module(), term(), GenServer.options()) :: GenServer.on_start() + @spec start_link(module(), term(), [Minch.option()]) :: GenServer.on_start() def start_link(module, init_arg, opts \\ []) do - GenServer.start_link(__MODULE__, {module, init_arg}, opts) + {opts, gen_opts} = Keyword.split(opts, @options) + GenServer.start_link(__MODULE__, {module, init_arg, opts}, gen_opts) end - @spec start(module(), term(), GenServer.options()) :: GenServer.on_start() + @spec start(module(), term(), [Minch.option()]) :: GenServer.on_start() def start(module, init_arg, opts \\ []) do - GenServer.start(__MODULE__, {module, init_arg}, opts) + {opts, gen_opts} = Keyword.split(opts, @options) + GenServer.start(__MODULE__, {module, init_arg, opts}, gen_opts) end @spec stop(GenServer.server()) :: :ok @@ -34,10 +39,16 @@ defmodule Minch.Conn do end @impl true - def init({callback, init_arg}) do + def init({callback, init_arg, opts}) do case callback.init(init_arg) do {:ok, callback_state} -> - state = %State{callback: callback, callback_state: callback_state, conn_attempt: 0} + state = %State{ + callback: callback, + callback_state: callback_state, + close_timeout: Keyword.get(opts, :close_timeout, 5000), + conn_attempt: 0 + } + Process.flag(:trap_exit, true) {:ok, state, {:continue, :connect}} @@ -97,8 +108,12 @@ defmodule Minch.Conn do {:noreply, %{state | reconnect_timer: nil}, {:continue, :connect}} end - def handle_info({@internal, {:close_timeout, reason}}, state) do - handle_disconnect(reason, state) + def handle_info({@internal, :close_timeout}, %State{close_timer: nil} = state) do + {:noreply, state} + end + + def handle_info({@internal, :close_timeout}, state) do + handle_disconnect(state.close_frame, state) end def handle_info(message, %State{conn: nil} = state) do @@ -179,9 +194,10 @@ defmodule Minch.Conn do end defp handle_disconnect(error, %State{} = state) do + reason = state.close_frame || error state = close(%{state | conn_attempt: state.conn_attempt + 1}) - case state.callback.handle_disconnect(error, state.conn_attempt, state.callback_state) do + case state.callback.handle_disconnect(reason, state.conn_attempt, state.callback_state) do {:reconnect, backoff, callback_state} -> cancel_timer(state.reconnect_timer) reconnect_timer = internal_event(:reconnect, backoff) @@ -273,13 +289,15 @@ defmodule Minch.Conn do defp send_close(%State{} = state, frame) do send_frame(state, frame) - %{state | websocket: nil, close_timer: internal_event({:close_timeout, frame}, 5000)} + cancel_timer(state.close_timer) + close_timer = internal_event(:close_timeout, state.close_timeout) + %{state | websocket: nil, close_timer: close_timer, close_frame: frame} end defp close(%State{conn: conn} = state) do if conn, do: Mint.HTTP.close(conn) cancel_timer(state.close_timer) - %{state | conn: nil, websocket: nil, request_ref: nil, close_timer: nil} + %{state | conn: nil, websocket: nil, request_ref: nil, close_timer: nil, close_frame: nil} end defp cancel_timer(nil), do: :ok diff --git a/test/minch/client_test.exs b/test/minch/client_test.exs index 3cd0caa..5a60abf 100644 --- a/test/minch/client_test.exs +++ b/test/minch/client_test.exs @@ -5,7 +5,7 @@ defmodule Minch.ClientTest do use Minch, restart: :transient def start_link(state) do - Minch.start_link(__MODULE__, state) + Minch.start_link(__MODULE__, state, state[:opts] || []) end def connect(state) do @@ -147,8 +147,8 @@ defmodule Minch.ClientTest do @tag client_state: %{reconnect: 10} test "handle_disconnect/2 is called when received a :close frame from server", ctx do assert_receive {:client, :handle_connect, _} - Server.send_frame(ctx.server, :close) - assert_receive {:client, :handle_disconnect, [_reason, 1, _state]} + Server.send_frame(ctx.server, {:close, 4001, "policy"}) + assert_receive {:client, :handle_disconnect, [{:close, 4001, "policy"}, 1, _state]} assert_receive {:server, :init, server} assert_receive {:client, :handle_connect, _} @@ -156,6 +156,16 @@ defmodule Minch.ClientTest do assert_receive {:client, :handle_disconnect, [_reason, 1, _state]} end + @tag client_state: %{opts: [close_timeout: 50]} + test "handle_disconnect/2 is called when the close handshake is left unanswered", ctx do + assert_receive {:client, :handle_connect, _} + # suspended so the server never reads our close frame, and never closes the socket + :sys.suspend(ctx.server) + send(ctx.client, {:close, 1000, "bye"}) + assert_receive {:client, :handle_disconnect, [{:close, 1000, "bye"}, 1, _state]} + :sys.resume(ctx.server) + end + test "connection is properly closed and terminate/2 is called", ctx do assert_receive {:client, :handle_connect, _} Minch.close(ctx.client) @@ -172,7 +182,7 @@ defmodule Minch.ClientTest do assert_receive {:client, :handle_connect, _} send(ctx.client, {:close, 1000, "bye"}) assert_receive {:server, :terminate, {:remote, 1000, "bye"}} - assert_receive {:client, :handle_disconnect, [%Mint.TransportError{reason: :closed}, 1, _]} + assert_receive {:client, :handle_disconnect, [{:close, 1000, "bye"}, 1, _]} end test "stops the client process by returning a :stop tuple from a callback", ctx do