-
Notifications
You must be signed in to change notification settings - Fork 120
feat: polymorphic helper for request body streaming #477
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
|
@@ -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, | ||
|
|
@@ -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} | ||
|
|
@@ -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), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1039,6 +1039,10 @@ defmodule Mint.HTTP2 do | |
| %{conn | proxy_headers: headers} | ||
| end | ||
|
|
||
| @impl true | ||
| def get_send_window(%__MODULE__{} = conn, ref), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
I can't come up with a really good name, maybe "possible_send_size"?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Name |
||
| do: min(get_window_size(conn, :connection), get_window_size(conn, {:request, ref})) | ||
|
|
||
| ## Helpers | ||
|
|
||
| defp handle_closed(conn) do | ||
|
|
||
| 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 |
There was a problem hiding this comment.
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.