Skip to content

feat: polymorphic helper for request body streaming#477

Open
tank-bohr wants to merge 2 commits intoelixir-mint:mainfrom
tank-bohr:main
Open

feat: polymorphic helper for request body streaming#477
tank-bohr wants to merge 2 commits intoelixir-mint:mainfrom
tank-bohr:main

Conversation

@tank-bohr
Copy link
Copy Markdown
Contributor

@tank-bohr tank-bohr commented Mar 19, 2026

Hey guys. Let me explain the problem I faced with. Below is the typical code for the sreaming body request

defmodule MintDialyzerRepro do
  @default_chunk_size 16_384

  def run(method, %URI{scheme: scheme} = uri, headers, body, opts)
      when scheme in ~w[http https] do
    scheme = String.to_existing_atom(scheme)
    port = uri.port || URI.default_port(uri.scheme)
    path = URI.to_string(%URI{uri | scheme: nil, host: nil, authority: nil})

    with {:ok, conn} <- Mint.HTTP.connect(scheme, uri.host, port, opts) do
      stream(conn, method, path, headers, body)
    end
  end

  defp stream(conn, method, path, headers, body) do
    with {:ok, conn, ref} <- Mint.HTTP.request(conn, method, path, headers, :stream) do
      stream_body(conn, ref, body)
    end
  end

  defp stream_body(conn, ref, "") do
    Mint.HTTP.stream_request_body(conn, ref, :eof)
  end

  defp stream_body(conn, ref, body) do
    chunk_size = min(chunk_size(conn, ref), byte_size(body))
    <<chunk::binary-size(chunk_size), rest::binary>> = body

    with {:ok, conn} <- Mint.HTTP.stream_request_body(conn, ref, chunk) do
      stream_body(conn, ref, rest)
    end
  end

  defp chunk_size(conn, ref) do
    case Mint.HTTP.protocol(conn) do
      :http1 ->
        @default_chunk_size

      :http2 ->
        chunk_size_http2(conn, ref)
    end
  end

  defp chunk_size_http2(conn, ref) do
    min(
      Mint.HTTP2.get_window_size(conn, :connection),
      Mint.HTTP2.get_window_size(conn, {:request, ref})
    )
  end
end

This code generates dialyzer warnings

Total errors: 2, Skipped: 0, Unnecessary Skips: 0
done in 0m1.78s
lib/mint_dialyzer_repro.ex:46:34:call_without_opaque
Function call without opaqueness type mismatch.

Call does not have expected opaque term of type Mint.HTTP2.t() in the 1st position.

Mint.HTTP2.get_window_size(_conn :: Mint.HTTP.t(), :connection)

________________________________________________________________________________
lib/mint_dialyzer_repro.ex:47:34:call_without_opaque
Function call without opaqueness type mismatch.

Call does not have expected opaque term of type Mint.HTTP2.t() in the 1st position.

Mint.HTTP2.get_window_size(_conn :: Mint.HTTP.t(), {:request, reference()})

________________________________________________________________________________
done (warnings were emitted)
Halting VM with exit status 2

It was discussed in the #380 and was left with a dirty workaround.
And people are using the workaround. And on the OTP-28 such trick in the k8s library leads to the infinite hanging during the PLT building phase.

But it's a legit issue. And it's totally worth to be fixed.

Opaqueness

Since the caller have to know the exact type for streaming, the Mint.HTTP.t() cannot be opaque. And the caller cannot know the exact type in advance either, because it's not their decision, but the server's decision.

Missing API for streaming

Another consequence of this code and dialyzer warning is the request body streaming API is not sufficient for using it without knowing the underlying protocol. But it's kinda designed to be such. To close the gap the current PR is introducing the missing piece - an ability to get the chunk size for the streaming.

@tank-bohr
Copy link
Copy Markdown
Contributor Author

@ericmj Could you please take a look

@whatyouhide
Copy link
Copy Markdown
Contributor

What if you make

@type Mint.HTTP.t() :: Mint.HTTP1.t() | Mint.HTTP2.t()

so that the "top-level" type is not opaque, but the inner types are?

@tank-bohr
Copy link
Copy Markdown
Contributor Author

tank-bohr commented Apr 23, 2026

What if you make

@type Mint.HTTP.t() :: Mint.HTTP1.t() | Mint.HTTP2.t()

so that the "top-level" type is not opaque, but the inner types are?

@whatyouhide That's exactly what my PR does. It un-opaques (exposes) the Mint.HTTP.t(). This will help to solve the dialyzer issue in the existing code (like k8s). Because existing code has to know about internals.

And get_send_window callback and Mint.HTTP.next_body_chunk_size/3 potentially can allow to opaque the Mint.HTTP.t() type as well. But it requires changing the callers code. But it's helpful anyhow. It brings the missing piece to the ubiquities API of Mint.HTTP

@tank-bohr
Copy link
Copy Markdown
Contributor Author

tank-bohr commented Apr 23, 2026

the original example with the get_send_window will look like

defmodule MintDialyzerRepro do
  def run(method, %URI{scheme: scheme} = uri, headers, body, opts)
      when scheme in ~w[http https] do
    scheme = String.to_existing_atom(scheme)
    port = uri.port || URI.default_port(uri.scheme)
    path = URI.to_string(%URI{uri | scheme: nil, host: nil, authority: nil})

    with {:ok, conn} <- Mint.HTTP.connect(scheme, uri.host, port, opts) do
      stream(conn, method, path, headers, body)
    end
  end

  defp stream(conn, method, path, headers, body) do
    with {:ok, conn, ref} <- Mint.HTTP.request(conn, method, path, headers, :stream) do
      stream_body(conn, ref, body)
    end
  end

  defp stream_body(conn, ref, "") do
    Mint.HTTP.stream_request_body(conn, ref, :eof)
  end

  defp stream_body(conn, ref, body) do
    chunk_size = Mint.HTTP.next_body_chunk_size(conn, ref, body)
    <<chunk::binary-size(chunk_size), rest::binary>> = body

    with {:ok, conn} <- Mint.HTTP.stream_request_body(conn, ref, chunk) do
      stream_body(conn, ref, rest)
    end
  end
end

As you see no need for using the HTTP2 module directly. No need to detect protocol. The Mint.HTTP module dispatches to the correct implementation itself. With this change we can keep Mint.HTTP.t() opaque. But I still recommend to unopaque it because of the existing code

Comment thread lib/mint/http1.ex
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
Contributor 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
Contributor 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.

Copy link
Copy Markdown
Contributor Author

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.

It's not required. It's just a number. it's part of TCP protocol, yes

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.

I don't propose the contract. I propose a helper for the streaming users. To get the best possible chunk size for streaming (you can think of it as 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.

Yes. And it exists only for the HTTP2. But the streaming is a common concept. H2 uses send windows for streaming. And http1 doesn't have it. So the best possible (the closest) alternative and concept for the chunk size we can offer to the API consumer is a sndbuf.

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.

So the best possible (the closest) alternative and concept for the chunk size we can offer to the API consumer is a sndbuf.

I don't agree with that, it's a different concept.

Comment thread lib/mint/http.ex Outdated
Comment thread lib/mint/http2.ex
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
Contributor 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
Contributor 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

Comment thread lib/mint/http.ex
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.

Co-authored-by: Eric Meadows-Jönsson <eric.meadows.jonsson@gmail.com>
@tank-bohr
Copy link
Copy Markdown
Contributor Author

@ericmj I have created a small PR #483 to fix a dialyzer issue. Because it can bring an immediate value. And we can proceed the discussion around the streaming API within this PR. I encourage you to take a closer look at the code sample in the PR description. It demonstrates the gap in the streaming API I want to cover.

  • send window is an H2 concept
  • http1 doesn't have it
  • h2 uses window to calculate chunk size for streaming
  • http1 also supports streaming
  • http1 as a chunk size can use either arbitrary size or the biggest possible size on the current TCP configuration
  • I believe the second option is better

@ericmj
Copy link
Copy Markdown
Member

ericmj commented Apr 25, 2026

I believe #483 solves the streaming issue for H2 as you can match on the connection type and call Mint.HTTP2 directly for the window size. H1 should already work for streaming.

If you need sndbuf for performance reasons you can access it via:

{:ok, conn} = Mint.HTTP.connect(:http, host, port, transport_opts: [sndbuf: 1_048_576])

socket = Mint.HTTP.get_socket(conn)
:inet.getopts(socket, [:sndbuf])
# or
:ssl.getopts(socket, [:sndbuf])

@tank-bohr
Copy link
Copy Markdown
Contributor Author

tank-bohr commented Apr 25, 2026

@ericmj You can forget about sndbuf. It's not important. Do you agree that streaming API is incomplete? We need a chunk size and the current API doesn't give a protocol-agnostic way to get the chunk size

@ericmj
Copy link
Copy Markdown
Member

ericmj commented Apr 25, 2026

You can't make it protocol agnostic because for H1 the chunk size is arbitrary and for H2 it's <window_size.

The only way I can think that is protocol agnostic would be send_as_much_as_possible(conn, body) :: binary() that for H2 splits the binary on window size, sends what fit inside the window and returns what was outside the window, and for H1 always just sends everything and returns an empty binary.

Mint is not supposed to be fully protocol agnostic. It's a low level library that abstracts away the protocol version when it's possible without adding abstraction leaks.

@tank-bohr
Copy link
Copy Markdown
Contributor Author

You can't make it protocol agnostic because for H1 the chunk size is arbitrary and for H2 it's <window_size.

Correct. But we can give user a hand and cover the most obvious and straightforward usecase

send_as_much_as_possible

for h1 it effectively means "send sndbuf bytes"

Mint is not supposed to be fully protocol agnostic. It's a low level library that abstracts away the protocol version when it's possible without adding abstraction leaks.

Yes! And my proposal is to consider if it's possible to make the streaming protocol-agnostic. Because I believe it's possible. Chunk size is a very low-level thingy. It keep all the flexibility of current API and just helps users not to write the exact same code over and over again

@ericmj
Copy link
Copy Markdown
Member

ericmj commented Apr 25, 2026

for h1 it effectively means "send sndbuf bytes"

No, that's only if you don't want to block on the send function. And even then there is no guarantee you won't block because you don't know if the receiver will have ACKed before you call send again. And there is no guarantee that H2 won't block on send for the same reason.

@tank-bohr
Copy link
Copy Markdown
Contributor Author

No, that's only if you don't want to block on the send function.

But why would you want to block? It's easy to make non-blocking call blocking but not other way around. I think that streaming by default should be non-blocking. And I would consider the non-blocking scenario is the default one on the API level. Especially taking into account that it's the only way to send body. A non-blocking call gives a user all the control

@tank-bohr
Copy link
Copy Markdown
Contributor Author

And even then there is no guarantee you won't block because you don't know if the receiver will have ACKed before you call send again. And there is no guarantee that H2 won't block on send for the same reason.

I can accept it. But I would expect from underlying library to make the best effort to stream even without guarantees

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants