From b77df10467aca2c7a0c8a641941b2f34c9815538 Mon Sep 17 00:00:00 2001 From: v0idpwn Date: Mon, 29 Apr 2024 23:19:26 -0300 Subject: [PATCH 1/2] Don't send RST_STREAM when not needed When the stream state is local_half_closed and an end_stream flag is received, the stream can be terminated normally without RST_STREAM. --- lib/mint/http2.ex | 35 ++++++++++++++++++++++------------- test/mint/http2/conn_test.exs | 7 ++----- 2 files changed, 24 insertions(+), 18 deletions(-) diff --git a/lib/mint/http2.ex b/lib/mint/http2.ex index df54869f..a5d75627 100644 --- a/lib/mint/http2.ex +++ b/lib/mint/http2.ex @@ -1566,7 +1566,7 @@ defmodule Mint.HTTP2 do responses = [{:data, stream.ref, data} | responses] if flag_set?(flags, :data, :end_stream) do - conn = close_stream!(conn, stream.id, :no_error) + conn = close_stream!(conn, stream.id, :remote_end_stream) {conn, [{:done, stream.ref} | responses]} else {conn, responses} @@ -1675,7 +1675,7 @@ defmodule Mint.HTTP2 do {conn, responses} end_stream? -> - conn = close_stream!(conn, stream.id, :no_error) + conn = close_stream!(conn, stream.id, :remote_end_stream) {conn, [{:done, ref} | new_responses]} true -> @@ -1685,7 +1685,7 @@ defmodule Mint.HTTP2 do end end_stream? -> - conn = close_stream!(conn, stream.id, :no_error) + conn = close_stream!(conn, stream.id, :remote_end_stream) {conn, [{:done, ref} | new_responses]} true -> @@ -1695,7 +1695,7 @@ defmodule Mint.HTTP2 do # Trailer headers. We don't care about the :status header here. headers when received_first_headers? -> if end_stream? do - conn = close_stream!(conn, stream.id, :no_error) + conn = close_stream!(conn, stream.id, :remote_end_stream) headers = headers |> Headers.remove_unallowed_trailer() |> join_cookie_headers() {conn, [{:done, ref}, {:headers, ref, headers} | responses]} else @@ -2094,18 +2094,27 @@ defmodule Mint.HTTP2 do throw({:mint, %{conn | state: :closed}, wrap_error({error_code, debug_data})}) end - defp close_stream!(conn, stream_id, error_code) do + # Reason is either an error code or `remote_end_stream` + defp close_stream!(conn, stream_id, reason) do stream = Map.fetch!(conn.streams, stream_id) - # First of all we send a RST_STREAM with the given error code so that we - # move the stream to the :closed state (that is, we remove it). - rst_stream_frame = rst_stream(stream_id: stream_id, error_code: error_code) - conn = - if open?(conn) do - send!(conn, Frame.encode(rst_stream_frame)) - else - conn + cond do + # If the stream is ended on both sides, it is already deemed closed and + # there's no need to send a RST_STREAM frame + reason == :remote_end_stream and stream.state == :half_closed_local -> + conn + + # We send a RST_STREAM with the given error code so that we move the + # stream to the :closed state (that is, we remove it). + open?(conn) -> + error_code = if reason == :remote_end_stream, do: :no_error, else: reason + rst_stream_frame = rst_stream(stream_id: stream_id, error_code: error_code) + send!(conn, Frame.encode(rst_stream_frame)) + + # If the connection is already closed, no-op + true -> + conn end delete_stream(conn, stream) diff --git a/test/mint/http2/conn_test.exs b/test/mint/http2/conn_test.exs index 429d3000..6692de93 100644 --- a/test/mint/http2/conn_test.exs +++ b/test/mint/http2/conn_test.exs @@ -325,7 +325,7 @@ defmodule Mint.HTTP2Test do assert [{:status, ^ref, 200}, {:headers, ^ref, []}, {:done, ^ref}] = responses - assert_recv_frames [rst_stream(stream_id: ^stream_id)] + assert Enum.empty?(conn.streams) assert {:ok, %HTTP2{} = conn, []} = stream_frames(conn, [ @@ -763,8 +763,7 @@ defmodule Mint.HTTP2Test do assert [{:status, ^ref, 200}, {:headers, ^ref, _headers}, {:done, ^ref}] = responses - assert_recv_frames [rst_stream(error_code: :no_error)] - + assert Enum.empty?(conn.streams) assert HTTP2.open?(conn) end @@ -1466,8 +1465,6 @@ defmodule Mint.HTTP2Test do {:done, ^ref} ] = responses - assert_recv_frames [rst_stream(stream_id: ^stream_id, error_code: :no_error)] - # Here we send headers for the two promised streams. Note that neither of the # header frames have the END_STREAM flag set otherwise we close the streams and # they don't count towards the open stream count. From e6a3aac052f01725352df867c357cb80818e034f Mon Sep 17 00:00:00 2001 From: v0idpwn Date: Thu, 9 May 2024 18:44:26 -0300 Subject: [PATCH 2/2] Add test --- lib/mint/http2.ex | 2 +- test/mint/http2/conn_test.exs | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/lib/mint/http2.ex b/lib/mint/http2.ex index a5d75627..6642f25b 100644 --- a/lib/mint/http2.ex +++ b/lib/mint/http2.ex @@ -2105,7 +2105,7 @@ defmodule Mint.HTTP2 do reason == :remote_end_stream and stream.state == :half_closed_local -> conn - # We send a RST_STREAM with the given error code so that we move the + # We send a RST_STREAM with the given error code so that we move the # stream to the :closed state (that is, we remove it). open?(conn) -> error_code = if reason == :remote_end_stream, do: :no_error, else: reason diff --git a/test/mint/http2/conn_test.exs b/test/mint/http2/conn_test.exs index 6692de93..41bcd4d3 100644 --- a/test/mint/http2/conn_test.exs +++ b/test/mint/http2/conn_test.exs @@ -335,6 +335,25 @@ defmodule Mint.HTTP2Test do assert HTTP2.open?(conn) end + + test "doesn't send RST_STREAM when stream is declared ended in both sides", %{conn: conn} do + {conn, ref} = open_request(conn) + + assert_recv_frames [headers(stream_id: stream_id)] + + assert conn.streams[stream_id].state == :half_closed_local + + assert {:ok, %HTTP2{} = conn, responses} = + stream_frames(conn, [ + {:headers, stream_id, [{":status", "200"}], [:end_headers, :end_stream]} + ]) + + assert [{:status, ^ref, 200}, {:headers, ^ref, []}, {:done, ^ref}] = responses + + assert_recv_frames([]) + assert is_nil(conn.streams[stream_id]) + assert HTTP2.open?(conn) + end end describe "stream state transitions" do