Add Mint.HTTP.recv_response/3

[wojtekmach]

I'd like to propose to add a Mint.HTTP1.recv_response/3 that improve ergonomics of making one off requests:

iex> {:ok, conn} = Mint.HTTP.connect(:https, "httpbin.org", 443, mode: :passive)
iex> {:ok, conn, ref} = Mint.HTTP.request(conn, "GET", "/user-agent", [], nil)
iex> {:ok, conn, response} = Mint.HTTP.recv_response(conn, ref, 5000)
iex> Mint.HTTP1.close(conn)
iex> response
%{
  status: 200,
  headers: [
    {"date", ...},
    ...
  ],
  body: "{\n  \"user-agent\": \"mint/1.6.2\"\n}\n"
}

There is a bunch of libraries which are simply make one off requests, to name a few: tzdata, goth/aws_credentials (they have their own pool, make one off request every ~hour), esbuild/tailwind.

To play the devils advocate, esbuild/tailwind are simply using httpc, copy-pasting these secure ssl defaults from EEF Security WG.

[coveralls]

Pull Request Test Coverage Report for Build 55ab096f5279d6b3e29ddcd2654eda50680b0c15-PR-447

Details

• 18 of 21 (85.71%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.3%) to 87.719%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/mint/core/util.ex	0	1	0.0%
lib/mint/http.ex	18	20	90.0%
<!--	Total:	18	21	85.71%	-->

Totals
Change from base Build 50b11d668b6a240b0d9b20c67fbb41a10a7410b1:	0.3%
Covered Lines:	1300
Relevant Lines:	1482

---

💛 - Coveralls

[wojtekmach]

Another idea is to instead have:

{:ok, conn} = Mint.HTTP.connect(:https, "httpbin.org", 443)

{:ok, conn, _acc = nil} =
  Mint.HTTP.request(
    conn,
    "GET",
    "/user-agent",
    _headers = [],
    _body = nil,
    _timeout = 5000,
    _acc = nil,
    fn conn, entry, acc ->
      IO.inspect(entry)
      {:cont, conn, acc}
    end
  )

which does request/recv_response. Yet another idea is to have a single function that does connect/request/recv_response. I think request/8 above is a nice middle ground though.

[whatyouhide]

@wojtekmach why would this crash?

{:ok, conn} = Mint.HTTP1.connect(:https, "httpbin.org", 443, mode: :passive)
{:ok, conn, ref1} = Mint.HTTP1.request(conn, "GET", "/user-agent", [], nil)
{:ok, conn, ref2} = Mint.HTTP1.request(conn, "GET", "/user-agent", [], nil)
Mint.HTTP1.recv_response(conn, ref1, 5000)
Mint.HTTP1.recv_response(conn, ref2, 5000)

recv_response could just discard messages that are not for the given ref, no?

request/8 is a no-go for me, too complex of an API. recv_response/3 is nice because it separates the APIs, so that you can still use request/5 and in case then you decide you're done with async stuff and want to receive a response, you have the option to do so in a blocking fashion with recv_response/3.

[whatyouhide]

By the way, I’m totally in favor for this. 👍

[wojtekmach]

Yes I can totally disregard unrelated messages and can document it as such. The only problem is they cannot be recovered. My understanding is in active mode we do selective receive on conn, not request, and in passive mode we’d have read from the socket. Again I think it’s fine.

Should we support active mode or passive mode or both?

Should we support HTTP/2, ie this becomes a function on Mint.HTTP?

Fair enough about not supporting /8.

[josevalim]

Should we support HTTP/2, ie this becomes a function on Mint.HTTP?

Yes IMO

[whatyouhide]

Yes HTTP/2 for sure. I’m thinking we might want to enforce this on passive conns actually, because if you think about it there's not much advantage to doing a blocking req and using active mode (yeah can recv bytes while parsing but probably negligible).

[wojtekmach]

Got it, thanks.

Would you prefer to start with just recv_response/3 or recv_response/5 (that additionally has acc and fun) or both? In the PR we have both at the moment.

[whatyouhide]

@wojtekmach can recv_response/3 return a stream of {atom(), term()} instead? Like a stream of {:status, ...} and so on. That way, you can do Map.new() if you expect a single piece of body or just Enum.reduce for custom stuff.

[wojtekmach]

I renamed this to Mint.HTTP.recv_response/3, added basic docs and tests, and updated PR description accordingly.

[wojtekmach]

@whatyouhide interesting! By stream of {atom, term} tuples you mean a list of these right? Or we're talking about lazy streams? I assume it's a list (so this function returns updated conn) so a follow-up question, what'd be a benefit over a map? I don't know if this would happen (or even be possible) in practice but maybe a silly server could return {:data, "foo"} followed by {:data, ""} and this would break passing it to Map.new(). A response with trailers would break Map.new too but I concede it's rather rare. If people are going to end up using Enum.reduce over this list a lot of times then I'd push for a version with acc + fun which will be very similar to Enum.reduce. In other words, in that case I'd have recv_response/3 which returns a map and recv_response_5 (or recv_response_while/5). WDYT?

[wojtekmach]

RE CI failure, sigh, https://github.com/actions/runner-images/issues/9692.

[wojtekmach]

CI is fixed in #448.

[whatyouhide]

Damn I always forget that streams cannot carry an accumulator, and we really want to accumulate over the conn 🙄

[wojtekmach]

We could have a single function that does connect/request/recv_response/close and it returns {:ok, enumerable}, no conn, it would be something like request/9 or, you know, request/1. :)

[whatyouhide]

Since the use case here is to make a request and receive a response in a single operation without having to deal with the intricacies of Mint, then I would go with:

Mint.HTTP.request_and_response(
  conn,
  method,
  path,
  headers,
  body,
  options :: [{:timeout, timeout()}]
)

which returns a response map (usual %{status: ..., ...}). This is a handy compromise for one-shot requests. If you want to have control over accumulation, go with receiving the messages (or passive mode). Thoughts?

[wojtekmach]

@whatyouhide yeah, I think this is a good compromise. The only thing that comes to my mind is with a distinct recv_response/3 we may still support request body streaming before calling it (since it blocks), but with request_and_response/6 we can't. But I think that's probably OK, can always drop down to lower level things. WDYT?

[josevalim]

I think I would keep two separate functions. You get to support request streaming, passive mode, and pipelining by keeping them apart. Sure they are not “deal breakers” but we are not gaining much by unifying anyway, so why do it?

[wojtekmach]

That being said I think what we have right now is pretty decent (if I may say so)

iex> {:ok, conn} = Mint.HTTP.connect(:https, "httpbin.org", 443, mode: :passive)
iex> {:ok, conn, request_ref} = Mint.HTTP.request(conn, "GET", "/user-agent", [], nil)
iex> {:ok, _conn, response} = Mint.HTTP.recv_response(conn, request_ref, 5000)
iex> response
%{
  status: 200,
  headers: [{"date", ...}, ...],
  body: "{\n  \"user-agent\": \"1.6.2\"\n}\n"
}

edit: right, ditto

[whatyouhide]

Yeah I like that but I do not like the fact that it can trip you up if there are multiple in-flight reqs. Can we raise an error if there are?

[josevalim]

Why can you trip up? You can for HTTP1 if you consume the responses out of order but not for HTTP2, right?

[whatyouhide]

You can because as Wojtek said you will still consume the bytes (or msgs) for other requests. Say you have ref1 and ref2 in flight and you get these bytes:

ref1(HEADERS)
ref2(HEADERS)
ref1(BODY)
ref1(END)

in a single packet. If you call recv_response(ref1), then you will consume the headers for ref2 and won't be able to return them in the response.

[wojtekmach]

Yeah see this comment: https://github.com/elixir-mint/mint/pull/447#issuecomment-2258261087. I’m fine with discarding (right now on the branch) or crashing. Lmk!

[whatyouhide]

We can crash if there are multiple reqs in flight, the conn knows that.

[josevalim]

Sorry, to be clear, there is a valid use case for this feature:

1. HTTP1, as long as you consume them in order
2. HTTP2

So I am not sure why we should raise and forbid all cases? Couldn't we just document it instead that HTTP1 itself requires order?

[whatyouhide]

@josevalim okay yes in HTTP/1 reqs are in order so this is not an issue. In HTTP/2, this is an issue, as per https://github.com/elixir-mint/mint/pull/447#issuecomment-2268750627. If frames come out of order, we can absolutely match them back into their corresponding request, but generally Mint emits "responses" as frames. So in the sequence of frames in the comment above, a normal Mint flow would return

[{:headers, ref1, ...}, {:headers, ref2, ...}, {:body, ref1, ...}, {:done, ref1}]

In this, you didn't lose the headers of ref2. If recv_response instead returns %{status, headers, body} for ref1, we lost the headers for ref2 unless we buffer them into the conn (I would rather not).

Considering that this is a nice utility for one-shot requests, and you can still implement blocking recv_response through the normal message-based API anyway, I'd say we don't need HTTP/2 multiplexing?

[josevalim]

@whatyouhide If we receive messages from ref2 when receiving ref1, we should store them on the conn, and replay them only when you receive ref2, no?

[whatyouhide]

@josevalim yeah that's the complexity I want to avoid, as I don't think it's justified for the recv_response utility. We don't store anything now because if you receive a complete frame for ref2 while ref1 is also in flight, we just emit it (as a {:status, ...}, {:headers, ...}, etc response).

[josevalim]

Can you expand on the complexity? Because I was thinking it would mostly be a map with frames per ref?

[whatyouhide]

Complexity as we don't need it 🙃

Also, can you describe a use case where I have ref1 and ref2 in flight, and then I call recv_response(ref1). I never call recv_response(ref2). What happens to the frames we receive for ref2? Do we surface them only when the user finally calls stream/2?

[josevalim]

@whatyouhide I think I finally get what you mean. The functions in Mint.HTTP2 do not receive the request_ref as argument, it is up to me to match on them as I receive responses. So if we use recv_response(ref1) and we store responses for ref2, only recv_response itself would be able to use the ref2 messages, which feels inconsistent.

Alternatively, we could change recv, stream, and so on in Mint.HTTP2 to stream whatever pending messages as soon as they are invoked but that doesn't feel desired either?

So we can definitely do several requests and responses at once but then we should rather build something on top of conn and not reusing it. So with all of this said, I would perhaps revert to:

Mint.HTTP.request_and_response(
  conn,
  method,
  path,
  headers,
  body,
  options :: [{:timeout, timeout()}]
)

Although it would be nice to reduce the number of arguments.

[whatyouhide]

@josevalim consider that this is just one mor argument than Mint.HTTP.request/5, so I think we're okay 🙃 The options will ensure that we won't have any more arguments in the future as we can just add them as options then.

Also, the reason I’m pushing back here on features like body streaming and whatnot is that I want Mint to stay really low level and just an abstraction around the network socket, but with protocol understanding. If we start to add fancy functionality then I’m afraid we will just end up with Finch 😄 A single one-shot req/resp function makes sense for scripting with less deps, and libraries and so on, but I would probably stop there and ask anyone who needs more complex scenarios to just use the normal stream/2-based API.

[josevalim]

Also, the reason I’m pushing back here on features like body streaming and whatnot is that I want Mint to stay really low level and just an abstraction around the network socket, but with protocol understanding.

Yes, I agree with this. At the same time, there can be use cases where you only need to do one HTTP request, and people are resorting to Finch because there is nothing slightly more ergonomic in Mint. Also, if the goal is to one day have Mint in Erlang/OTP, I think such conveniences make sense, which is why I am not opposed for some slight exploration here. :) But yeah, if you need body streaming, it is probably fine to rely on the low-level ones.

[whatyouhide]

@wojtekmach let's go with request_and_response for now then and evolve from there. Body streaming and whatnot might come in later as options after all 🙃

[wojtekmach]

I renamed the function to Mint.HTTP.request_and_response/6. I made it so receiving unrelated responses now crashes and documented it as such. I still need to update HTTP/2 test suite for that scenario.

FWIW, and this is I think mostly because how test helpers were implemented, this functionality was easier to test when we were separately calling request and recv_response. I'm not sure if it matters for how users would end up using this function but I thought I'd share this observation anyway.