Add Mint.HTTP.recv_response/3 #447

wojtekmach · 2024-07-26T08:39:27Z

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 · 2024-07-26T08:43:28Z

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%

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

💛 - Coveralls

wojtekmach · 2024-07-26T08:51:30Z

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 · 2024-07-30T12:43:30Z

@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 · 2024-07-30T12:43:40Z

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

wojtekmach · 2024-07-30T13:20:29Z

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 · 2024-07-30T13:29:45Z

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

Yes IMO

whatyouhide · 2024-07-30T15:00:10Z

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 · 2024-07-31T10:10:21Z

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 · 2024-07-31T12:24:21Z

@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 · 2024-07-31T16:19:08Z

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

wojtekmach · 2024-07-31T16:27:51Z

@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 · 2024-07-31T16:31:21Z

lib/mint/http.ex

+  end
+
+  defp recv_response([{:data, ref, data} | rest], acc, conn, ref, timeout) do
+    acc = update_in(acc.body, &(&1 <> data))


This could be:

Suggested change

acc = update_in(acc.body, &(&1 <> data))

acc = update_in(acc.body, &[&1, data])

and then on :done we would IO.iodata_to_binary it. I decided to keep it simple for now, Finch is like that (using <>) at the moment too.

We shouldn't use <> and we should probably change Finch to not do that either. That will most likely copy the left-side on every concatenation. The IO version should be fairly more efficient. I would also keep the acc as a triplet.

Keep as triplet and convert to map at the end right?

josevalim

One nit and looks good to me!

wojtekmach · 2024-08-03T20:19:54Z

RE CI failure, sigh, actions/runner-images#9692.

wojtekmach · 2024-08-03T20:34:21Z

CI is fixed in #448.

whatyouhide · 2024-08-04T12:40:34Z

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

wojtekmach · 2024-08-04T13:10:17Z

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 · 2024-08-05T06:53:09Z

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 · 2024-08-05T07:16:59Z

@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 · 2024-08-05T07:22:13Z

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 · 2024-08-05T07:22:34Z

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 · 2024-08-05T10:22:14Z

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 · 2024-08-05T10:27:02Z

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

whatyouhide · 2024-08-05T10:36:01Z

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 · 2024-08-05T10:42:30Z

Yeah see this comment: #447 (comment). I’m fine with discarding (right now on the branch) or crashing. Lmk!

whatyouhide · 2024-08-05T11:06:06Z

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

josevalim · 2024-08-05T11:20:29Z

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

HTTP1, as long as you consume them in order
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 · 2024-08-05T15:08:50Z

@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 #447 (comment). 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 · 2024-08-05T15:41:34Z

@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 · 2024-08-05T15:52:08Z

@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 · 2024-08-05T16:05:39Z

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

whatyouhide · 2024-08-05T16:32:03Z

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 · 2024-08-05T19:35:52Z

@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 · 2024-08-06T07:08:34Z

@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 · 2024-08-06T07:36:46Z

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 · 2024-08-06T09:43:00Z

@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 · 2024-08-14T14:38:01Z

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.

whatyouhide

Thanks @wojtekmach this is looking great. Left a few comments 🙃

whatyouhide · 2024-08-15T06:06:53Z

lib/mint/http.ex

+  @doc """
+  Sends a request and receives a response from the socket in a blocking way.
+
+  This function is a convenience for sending a request with `request/5` and repeatedly calling `recv/3`.


Suggested change

This function is a convenience for sending a request with `request/5` and repeatedly calling `recv/3`.

This function is a convenience for sending a request with `request/5` and calling `recv/3`

repeatedly until a full response is received.

whatyouhide · 2024-08-15T06:07:11Z

lib/mint/http.ex

+
+        * `:status` - HTTP response status, an integer.
+
+        * `:headers` - HTTP response headers, a list of tuples `{header_name, header_value}`.


Also trailing headers?

whatyouhide · 2024-08-15T06:08:27Z

lib/mint/http.ex

+    * `:timeout` - the maximum amount of time in milliseconds waiting to receive the response.
+      Setting to `:infinity`, disables the timeout. Defaults to `:infinity`.


Mh, are we ok with a default that potentially blocks forever? Maybe 30s is a better default?

lib/mint/http.ex

whatyouhide · 2024-08-15T06:11:18Z

lib/mint/http.ex

+    recv_response(rest, {status, headers, [body, data]}, conn, ref, timeout)
+  end
+
+  defp recv_response([{:done, ref} | _rest], {status, headers, body}, conn, ref, _timeout) do


Shouldn't we error out here if there is trailing response data? That is, this should always be [{:done, ref}] no?

lib/mint/http.ex

whatyouhide · 2024-08-15T06:12:15Z

lib/mint/http.ex

+      Contrary to `recv/3`, this function does not return partial responses on errors. Use
+      `recv/3` for full control.
+
+  > #### Error {: .error}


We said we'd also raise if the mode is not :passive, right?

Co-authored-by: Andrea Leopardi <an.leopardi@gmail.com>

wojtekmach changed the title ~~Add Mint.HTTP1.recv_response/3,5~~ Add Mint.HTTP.recv_response/3 Jul 31, 2024

wojtekmach commented Jul 31, 2024

View reviewed changes

josevalim approved these changes Aug 1, 2024

View reviewed changes

wojtekmach added 7 commits August 4, 2024 19:46

Add Mint.HTTP1.recv_response/3,5

2549e1f

Simplify

b499e0d

Update

852d90e

Use IO.iodata_to_binary/1

347d7e8

Add more tests

e14016a

Improve docs

2c450a7

Improve docs and tests

2c10fca

wojtekmach force-pushed the wm-recv-response branch 2 times, most recently from e094168 to 2c10fca Compare August 4, 2024 17:49

wojtekmach added 2 commits August 14, 2024 15:41

Rename to Mint.HTTP.request_and_response/6

90142af

Update docs & test

7dde3bb

Fix tests

f307e26

wojtekmach force-pushed the wm-recv-response branch from d1db80f to f307e26 Compare August 14, 2024 14:40

whatyouhide reviewed Aug 15, 2024

View reviewed changes

Apply suggestions from code review

55ab096

Co-authored-by: Andrea Leopardi <an.leopardi@gmail.com>

	acc = update_in(acc.body, &(&1 <> data))
	acc = update_in(acc.body, &[&1, data])

	This function is a convenience for sending a request with `request/5` and repeatedly calling `recv/3`.
	This function is a convenience for sending a request with `request/5` and calling `recv/3`
	repeatedly until a full response is received.


		* `:status` - HTTP response status, an integer.

		* `:headers` - HTTP response headers, a list of tuples `{header_name, header_value}`.

		* `:timeout` - the maximum amount of time in milliseconds waiting to receive the response.
		Setting to `:infinity`, disables the timeout. Defaults to `:infinity`.

Add Mint.HTTP.recv_response/3 #447

Are you sure you want to change the base?

Add Mint.HTTP.recv_response/3 #447

Conversation

wojtekmach commented Jul 26, 2024 • edited Loading

coveralls commented Jul 26, 2024 • edited Loading

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

Details

💛 - Coveralls

wojtekmach commented Jul 26, 2024 • edited Loading

whatyouhide commented Jul 30, 2024

whatyouhide commented Jul 30, 2024

wojtekmach commented Jul 30, 2024

josevalim commented Jul 30, 2024

whatyouhide commented Jul 30, 2024

wojtekmach commented Jul 31, 2024

whatyouhide commented Jul 31, 2024

wojtekmach commented Jul 31, 2024

wojtekmach commented Jul 31, 2024 • edited Loading

wojtekmach Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josevalim left a comment

Choose a reason for hiding this comment

wojtekmach commented Aug 3, 2024

wojtekmach commented Aug 3, 2024

whatyouhide commented Aug 4, 2024

wojtekmach commented Aug 4, 2024

whatyouhide commented Aug 5, 2024

wojtekmach commented Aug 5, 2024

josevalim commented Aug 5, 2024

wojtekmach commented Aug 5, 2024 • edited Loading

whatyouhide commented Aug 5, 2024

josevalim commented Aug 5, 2024

whatyouhide commented Aug 5, 2024

wojtekmach commented Aug 5, 2024

whatyouhide commented Aug 5, 2024

josevalim commented Aug 5, 2024

whatyouhide commented Aug 5, 2024

josevalim commented Aug 5, 2024

whatyouhide commented Aug 5, 2024

josevalim commented Aug 5, 2024

whatyouhide commented Aug 5, 2024

josevalim commented Aug 5, 2024

whatyouhide commented Aug 6, 2024

josevalim commented Aug 6, 2024

whatyouhide commented Aug 6, 2024

wojtekmach commented Aug 14, 2024

whatyouhide left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtekmach commented Jul 26, 2024 •

edited

Loading

coveralls commented Jul 26, 2024 •

edited

Loading

wojtekmach commented Jul 26, 2024 •

edited

Loading

wojtekmach commented Jul 31, 2024 •

edited

Loading

wojtekmach Jul 31, 2024 •

edited

Loading

wojtekmach commented Aug 5, 2024 •

edited

Loading