Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/mint/core/conn.ex
Original file line number Diff line number Diff line change
Expand Up @@ -62,4 +62,6 @@ defmodule Mint.Core.Conn do
@callback put_proxy_headers(conn(), Mint.Types.headers()) :: conn()

@callback put_log(conn(), boolean()) :: conn()

@callback get_send_window(conn(), term()) :: non_neg_integer()
end
8 changes: 7 additions & 1 deletion lib/mint/http.ex
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ defmodule Mint.HTTP do

@behaviour Mint.Core.Conn

@opaque t() :: Mint.HTTP1.t() | Mint.HTTP2.t()
@type t() :: Mint.HTTP1.t() | Mint.HTTP2.t() | Mint.UnsafeProxy.t()

defguardp is_data_message(message)
when elem(message, 0) in [:ssl, :tcp] and tuple_size(message) == 3
Expand Down Expand Up @@ -1065,6 +1065,12 @@ defmodule Mint.HTTP do
@impl true
def put_proxy_headers(conn, headers), do: conn_apply(conn, :put_proxy_headers, [conn, headers])

@impl true
def get_send_window(conn, ref), do: conn_apply(conn, :get_send_window, [conn, ref])

@spec next_body_chunk_size(t(), term(), binary()) :: non_neg_integer()
def next_body_chunk_size(conn, ref, body), do: min(get_send_window(conn, ref), byte_size(body))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this function is unnecessary.


## Helpers

defp conn_apply(%UnsafeProxy{}, fun, args), do: apply(UnsafeProxy, fun, args)
Expand Down
11 changes: 9 additions & 2 deletions lib/mint/http1.ex
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,8 @@ defmodule Mint.HTTP1 do
proxy_headers: [],
private: %{},
log: false,
optional_responses: []
optional_responses: [],
sndbuf: nil
]

defmacrop log(conn, level, message) do
Expand Down Expand Up @@ -204,6 +205,7 @@ defmodule Mint.HTTP1 do
end

with :ok <- Util.inet_opts(transport, socket),
{:ok, sndbuf_opts} <- transport.getopts(socket, [:sndbuf]),
:ok <- if(mode == :active, do: transport.setopts(socket, active: :once), else: :ok) do
conn = %__MODULE__{
transport: transport,
Expand All @@ -216,7 +218,8 @@ defmodule Mint.HTTP1 do
log: log?,
case_sensitive_headers: Keyword.get(opts, :case_sensitive_headers, false),
skip_target_validation: Keyword.get(opts, :skip_target_validation, false),
optional_responses: validate_optional_response_values(opts)
optional_responses: validate_optional_response_values(opts),
sndbuf: sndbuf_opts[:sndbuf]
}

{:ok, conn}
Expand Down Expand Up @@ -667,6 +670,10 @@ defmodule Mint.HTTP1 do
%{conn | proxy_headers: headers}
end

@impl true
def get_send_window(%__MODULE__{streaming_request: %{ref: ref}, sndbuf: sndbuf}, ref),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is semantically very different to what what the HTTP2 module does. There is no concept of send windows in HTTP/1 so we should not expose it on this module.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open for suggestions about naming. It was hard to come up with a good name which will reflect the meaning of the concept for both http1 and http2. But streaming exists, and it works for both http1 and http2. There is a number that plays the role of the http2 send window in the http1 streaming. Usually the arbitrary number is used. But I thought that the good default could be the biggest number possible in the current TCP configuration. Conceptually it's the same as HTTP2 does, it's just provided by the underlying protocol

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sndbuf serves a different purpose to H2 window size and has different behavior if you exceed it, blocking vs erroring. sndbuf can be relevant for H2 also, the TCP send buffer can be smaller than the window size, but I don't know how likely that is to happen.

If the purpose is to ensure you never block in gen_tcp.send then you should probably check sndbuf for H2 also or set send_timeout on the socket.

If we don't currently support these options then please send a separate PR.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sndbuf in this context is not related to h2 at all. The main goal is to unify the streaming API. For streaming you need the chunk size. For http1.1 you need to get this number somehow. sndbuf is just the biggest reasonable chunk size for http1.1 streaming. At least that's how I understand the protocol. Naming is bad and to be discussed. But the idea of giving a universal API for streaming is still valid I believe

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If sndbuf is required for streaming H1 it is also required for H2. sndbuf is part of gen_tcp and both H1 and H2 use TCP.

The H2 send window is a contract with the receiving server we must obey, which is why Mint errors if you exceed it. sndbuf on the other hand is the size of the senders TCP send buffer, it only relates to the sender, it's not a contract with anyone. You would want to check it or tweak it for performance reasons.

Send windows as a concept does not exist for H1 so it's impossible to unify the APIs. The function should only exist on the HTTP2 module.

do: sndbuf

## Helpers

defp decode(:status, %{request: request} = conn, data, responses) do
Expand Down
4 changes: 4 additions & 0 deletions lib/mint/http2.ex
Original file line number Diff line number Diff line change
Expand Up @@ -1039,6 +1039,10 @@ defmodule Mint.HTTP2 do
%{conn | proxy_headers: headers}
end

@impl true
def get_send_window(%__MODULE__{} = conn, ref),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_window_size/2 is already public, I don't think we need a function that only calls min on top of that.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's simple it doesn't mean it's useless. If the vast majority of the code that uses streaming is written this way IMHO it makes sense to implement it in the library. Like a direction, like a guideline. Like it's most likely what you need. And if you need something special use the lower level API which is also available. And people need chunk size for streaming most of the time

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather add a comment that describes the min pattern or find a better name for get_send_window, it doesn't match the API for get_window_size:

  • Why does one have have _size in the name and not the other?
  • Why does one have "send" in the name but not the other, they are both send windows?
  • Just from the names I can't say what the difference is between get_send_window and get_window_size, I would have to read the docs.

I can't come up with a really good name, maybe "possible_send_size"?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name get_send_window is awful, I agree. possible_send_size sounds better to me. Maybe possible_chunk_size or streaming_chunk_size. Idk either

do: min(get_window_size(conn, :connection), get_window_size(conn, {:request, ref}))

## Helpers

defp handle_closed(conn) do
Expand Down
5 changes: 5 additions & 0 deletions lib/mint/unsafe_proxy.ex
Original file line number Diff line number Diff line change
Expand Up @@ -199,4 +199,9 @@ defmodule Mint.UnsafeProxy do
def put_proxy_headers(%__MODULE__{}, _headers) do
raise "invalid function for proxy unsafe proxy connections"
end

@impl true
def get_send_window(%__MODULE__{module: module, state: state}, ref) do
module.get_send_window(state, ref)
end
end
40 changes: 40 additions & 0 deletions test/http_test.exs
Original file line number Diff line number Diff line change
@@ -1,4 +1,44 @@
defmodule Mint.HTTPTest do
use ExUnit.Case, async: true
doctest Mint.HTTP

alias Mint.{HTTP, HTTP1.TestServer}

setup do
{:ok, port, server_ref} = TestServer.start()
assert {:ok, conn} = HTTP.connect(:http, "localhost", port)
assert_receive {^server_ref, server_socket}

[conn: conn, server_socket: server_socket]
end

describe "get_send_window/2" do
test "returns a positive integer for an active streaming request", %{conn: conn} do
{:ok, conn, ref} = HTTP.request(conn, "GET", "/", [], :stream)

send_window = HTTP.get_send_window(conn, ref)
assert is_integer(send_window)
assert send_window > 0
end
end

describe "next_body_chunk_size/3" do
test "returns body size when body is smaller than send window", %{conn: conn} do
{:ok, conn, ref} = HTTP.request(conn, "GET", "/", [], :stream)
small_body = "hello"
assert HTTP.next_body_chunk_size(conn, ref, small_body) == byte_size(small_body)
end

test "returns send window when body is larger than send window", %{conn: conn} do
{:ok, conn, ref} = HTTP.request(conn, "GET", "/", [], :stream)
send_window = HTTP.get_send_window(conn, ref)
large_body = :binary.copy(<<0>>, send_window + 1000)
assert HTTP.next_body_chunk_size(conn, ref, large_body) == send_window
end

test "returns 0 for empty body", %{conn: conn} do
{:ok, conn, ref} = HTTP.request(conn, "GET", "/", [], :stream)
assert HTTP.next_body_chunk_size(conn, ref, "") == 0
end
end
end
15 changes: 15 additions & 0 deletions test/mint/http1/conn_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -1073,6 +1073,21 @@ defmodule Mint.HTTP1Test do
{:ok, conn, responses}
end

describe "get_send_window/2" do
test "returns a positive integer for an active streaming request", %{conn: conn} do
{:ok, conn, ref} = HTTP1.request(conn, "GET", "/", [], :stream)

send_window = HTTP1.get_send_window(conn, ref)
assert is_integer(send_window)
assert send_window > 0
end

test "returns the cached sndbuf value", %{conn: conn} do
{:ok, conn, ref} = HTTP1.request(conn, "GET", "/", [], :stream)
assert HTTP1.get_send_window(conn, ref) == conn.sndbuf
end
end

@mint_user_agent "mint/#{Mix.Project.config()[:version]}"
defp mint_user_agent, do: @mint_user_agent
end
23 changes: 23 additions & 0 deletions test/mint/http2/conn_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -1772,6 +1772,29 @@ defmodule Mint.HTTP2Test do
HTTP2.get_window_size(conn, {:request, make_ref()})
end
end

test "get_send_window/2 returns the minimum of connection and request window sizes",
%{conn: conn} do
{conn, ref} = open_request(conn, :stream)

send_window = HTTP2.get_send_window(conn, ref)
conn_window = HTTP2.get_window_size(conn, :connection)
request_window = HTTP2.get_window_size(conn, {:request, ref})

assert send_window == min(conn_window, request_window)
end

test "get_send_window/2 decreases after streaming body data", %{conn: conn} do
{conn, ref} = open_request(conn, :stream)

initial_send_window = HTTP2.get_send_window(conn, ref)
assert initial_send_window > 0

body_chunk = "hello"
{:ok, conn} = HTTP2.stream_request_body(conn, ref, body_chunk)

assert HTTP2.get_send_window(conn, ref) == initial_send_window - byte_size(body_chunk)
end
end

describe "settings" do
Expand Down