All praise sensible defaults! I got bitten once by one of the clients not verifying SSL. Now it’s the first thing I check.

Req.get!("https://wrong.host.badssl.com/")
10:19:47.183 [info] TLS :client: In state :certify at ssl_handshake.erl:1990 generated CLIENT ALERT: Fatal - Handshake Failure
 - {:bad_cert, :hostname_check_failed}
10:19:47.184 [error] retry: got exception, will retry in 1000ms, 3 attempts left
10:19:47.184 [error] ** (Mint.TransportError) TLS client: In state certify at ssl_handshake.erl:1990 generated CLIENT ALERT: Fatal - Handshake Failure
 {bad_cert,hostname_check_failed}

Will swap out my Tesla usage with this. I love sensible defaults and Req seems less code to write. I hate writing code.

In my app I’m making requests to user-defined URLs, so errors are expected and I guess my feedback is mostly around that. Have a look at this:

iex> Req.request(url: "https://bad.domain")
22:23:59.915 [error] retry: got exception, will retry in 1000ms, 3 attempts left
22:23:59.915 [error] ** (Mint.TransportError) non-existing domain
22:24:00.932 [error] retry: got exception, will retry in 2000ms, 2 attempts left
22:24:00.933 [error] ** (Mint.TransportError) non-existing domain
22:24:02.945 [error] retry: got exception, will retry in 4000ms, 1 attempt left
22:24:02.945 [error] ** (Mint.TransportError) non-existing domain
{:error, %Mint.TransportError{reason: :nxdomain}}

• Would be nice to have Req.get (not only the ! versions),
• Would be nice to be able to abstract away the adapter: here I need to know the adapter and the possible errors as defined by the adapter; changing the adapter requires changing the error handling code,
• I think the default retries should be more aggressive: something like 2 attempts with just a few milliseconds of delay,
• I don’t think that retrying non-existing domain error is a sensible default.

And two general comments:

• As a step “consumer” it would be nice to be able to ignore the difference between request, response and error steps,
• It’s unclear how request options relate to step options. Personally I’d find it clearer if Req.request(foo: [bar: 1]) meant that [bar: 1] are the options for the :foo step.

Thanks for feedback!

• Would be nice to have Req.get (not only the ! versions),

I might ultimately do that but no plans at the moment. I think the ! version is what most people would use most of the time, i.e. crash on transient errors (after N retries) or some serious misconfiguration (of eg ssl options), at which point it’s not like at the call site we can do much in that situation anyway so might as well crash. I’ll definitely keep this in mind though.

• Would be nice to be able to abstract away the adapter: here I need to know the adapter and the possible errors as defined by the adapter; changing the adapter requires changing the error handling code,

Do you have specific kinds of errors you want to differentiate between?

• I think the default retries should be more aggressive: something like 2 attempts with just a few milliseconds of delay,

On the flip side if we’re erroring because service is under load, retrying again in a few milliseconds won’t help and in fact will make things worse. I went with the backoff values (1s, 2s, 4s, 8s, …) that curl has. It is very easy to provide your own retry strategy, fwiw.

• I don’t think that retrying non-existing domain error is a sensible default.

Agreed. Looking at it again, retrying on transport errors (which, to your point, are adapter specific) needs rethinking. curl handles some of 4xx/5xx by default plus opts-in to handling ECONNREFUSED with --retry-connrefused and that seems sensible enough. I will look into that.

• As a step “consumer” it would be nice to be able to ignore the difference between request, response and error steps,

Not sure what you mean by this, can you elaborate?

• It’s unclear how request options relate to step options. Personally I’d find it clearer if Req.request(foo: [bar: 1]) meant that [bar: 1] are the options for the :foo step.

They don’t relate, it is arbitrary. I actually started with having options be named after steps and nesting but found it a bit too verbose at times, e.g.: follow_redirects: [max_redirects: 10]. Besides this, there are options which are affecting multiple steps, for example raw: true would disable both decompress_body and decode_body. Overall I’m pretty happy with the “flatter” options but totally understand the concern. Curious what others think.

Not sure really, I just don’t like the fact that it’s an opaque term. That’s an issue that I have with a lot of HTTP clients and I’d like to see an improvement in this area. Can I log the error? It might contain sensitive information. I could pattern match, but then what happens when I switch adapters? Is it always going to be a map with a :reason key?

Makes sense. From my experience the retries usually address random network errors, so short retries are better. But I guess there’s no universal answer to this.

Would be nice nice to be able to just say append_steps instead of append_*_steps since I guess most of the time the step already implies where it should be.

Unless I’d learn the steps and all the config options more or less by heart, the fact that they are arbitrary makes it confusing. I don’t like when things are arbitrary or too implicit. I think there are ways to address the verbosity - the step name implies what the settings do, so you could accept all of the following:

• follow_redirects: [max: 10],
• follow_redirects: true,
• follow_redirects: 10.

As for options that affects multiple steps, I’m not sure that’s the optimal design. Can I decompress the body without decoding it?

This is very nice. How hard would it be, in your opinion, to add support for the streaming request body and also streaming responses? I have a pretty specific use case where I generate files I need to send out on the fly, and I don’t want to store them at all, but they can be fairly large (> 1GB) so currently, I am generating Elixir Stream and feed it as request body to Finch (added that part to the lib) Finch — Finch v0.23.0 and when I receive the files on the other end I also turn them into Elixir stream and consume line by line. Unfortunately HTTP is all I have to talk to between both apps, but so far it’s been working great with Finch.

What do you mean by opaque term? I believe it is pretty concrete. For Req by default it’s Finch.Error, Mint.HTTPError, or Mint.TransportError. You’re totally right that if switching adapters you’d get different errors so if you have error handling code, it would need to be updated. But what I want to figure out is what kind of error handling do you actually have. Because in my experience if I get any of these errors I cannot do anything with them anyway, I cannot recover from them, so the sensible thing is just to crash.

Perhaps I should make a bit less emphasis on being able to switch adapters because honestly I don’t see the point. Finch is great. To me switching adapters is only useful in tests.

Would be nice nice to be able to just say append_steps instead of append_*_steps since I guess most of the time the step already implies where it should be.

Gotcha, sorry, that is not possible. We have three buckets, request, response or error steps, so
when we add something we need to know what type of a thing it is, we cannot infer it.

Unless I’d learn the steps and all the config options more or less by heart, the fact that they are arbitrary makes it confusing.

I’m not sure if it addresses your concern but fwiw all the available options for built-in steps
are documented in a single place: Req.request/1 options.

Can I decompress the body without decoding it?

Yes, you can set decode_body: false.

Thanks! Streaming request body is trivial as finch already does it (thank you for adding it!) but streaming response is pretty tricky. See Response streaming · Issue #82 · wojtekmach/req · GitHub for some discussion. It’s definitely on my mind and unless we have it, I won’t consider Req complete, but there’s no concrete plans at the moment, unfortunately.

You know the errors because you’re the author. As a user, I can only see this:

@spec request(Req.Request.t() | keyword()) :: {:ok, Req.Response.t()} | {:error, Exception.t()}

Not as opaque as hackney:

request(URL::url() | binary() | list()) -> {ok, integer(), list(), client_ref()} | {ok, integer(), list()} | {error, term()}

but still not very useful. Ideally this would be {:error, Req.Error.t()} with a well-defined set of possible values.

In my use case those errors are expected (e.g. mistyped URL), so I don’t want to crash.

Supporting only one adapter is perfectly fine for me - I was pretty happy with HTTPoison at some point. Support for replacing the adapter for tests is very important though.

I’m skeptical about adding a Req.Error because I think it would be inferior to e.g. Mint.TransportError (it has a nice Exception.message/1 callback implementation) and I’m not sure I can reliably keep them in sync. I’m skeptical about adding strict error contract, one that would be useful for control flow, because I don’t think errors should be used for control flow. (I’m kind of doing that in retry step and maybe that’s my mistake.) I don’t have an answer for it but I’ll keep at it. Thanks for bringing this up!