Retrying by default is a bit surprising

[PragTob]

Cześć!

Thanks a lot for Req, so super dziękuję :green_heart:

I was just a bit surprised when I figured out Req retried GET/HEAD requests by default. It does make sense of course, but it also means we spend potentially much longer than I'd have originally accounted for to make a request.

It's probably that it's (imo) uncommon behavior for HTTP libraries to do this by default. The docs clearly state it, but I thought "I don't need retry, so I won't read the retry docs" and I think the README doesn't state it clearly enough. I mean technically the introductory GET example that we do get "retry on error" just like this so it is stated, but frankly I didn't understand it as such when reading it. And... when I don't understand it I suspect others won't as well.

I wanted to PR against the README to make it clearer, but I'm honestly not sure how/where - maybe in the Features list?

Thoughts?

Thanks again! :green_heart:

[wojtekmach]

Hallo! Good call. In "Features" we have

Request body compression and automatic response body decompression (via compress_body, compressed, and decompress_body steps).

And the idea was the word "automatic" is operative here, in other words, request body compression is opt-in but response body decompression is automatic, it is opt-out.

And so perhaps this change would be enough?

-   * Follows redirects (via [redirect] step)
-
-   * Retries on errors (via [retry] step)
+   * Automatically follows redirects (via [redirect] step)
+
+   * Automatically retries on errors (via [retry] step)

WDYT? We can also change "via" to "see" to nudge people more to read the docs? Anyhow, suggestions welcome!

[wojtekmach]

Can also be super explicit:

  * Set request body compression with `compress_body: true`. See [compress_body].

  * Automatically decompresses response body. Set `decompress_body: false` or `raw: true` to disable. See [compressed], [decompress_body] steps.

  (...)

  * Automatically follows redirects. Set `redirect: false` to disable. See [redirect] step.

  * Automatically retries on errors. Set `retry: false` to disable. See [retry] step.

Dunno if this would be too much information for what should be just an overview.

The so to speak canonical docs for feature set are here: https://hexdocs.pm/req/Req.html#new/1 where we have more details and can add even more.

paging @whatyouhide for some docs opinions!

[whatyouhide]

@wojtekmach are we sure we don't want retry: true instead, so that retrying is opt in? In any case I like the latest suggestion, the most explicit:

  * Automatically follows redirects. Set `redirect: false` to disable. See [redirect] step.

[wojtekmach]

I'd like to keep retries on by default.

but it also means we spend potentially much longer than I'd have originally accounted for to make a request.

Yup, fair enough. So this is orthogonal but I believe the best solution would be to have an opt-in :timeout and/or :deadline options. They'd account for all retries, redirects, etc. In other words you tell Req how much time it has and you don't care what it does with it. WDYT?

[whatyouhide]

I believe the best solution would be to have an opt-in :timeout and/or :deadline options. They'd account for all retries, redirects, etc. In other words you tell Req how much time it has and you don't care what it does with it. WDYT?

This is not too bad but I'd just warn against slippery sloping here. This is sort of complex, because then Req has to take decisions on what backoffs to use in order to fit the :timeout option and whatnot... I’m not sure this is something that falls under the "batteries-included" umbrella.

[PragTob]

:wave:

I also like the 2nd "super explicit" suggestion best :)

As for retrying, as it is somewhat surprising behaviour my vote would also be that it'd be an explicit opt-in. That said, I think that's your call as creator and maintainer - I agree in the sense that it should be safe. Just to add to the surprise, often times requests should be/are done in a background queue, which also already has exponential backoff on leading you to do potentially way more requests than you intended. Then again, it's only GET/HEAD and so should be fine (tm).

A total :timeout option that takes into account the retries etc. would be nice, but also feels like a bit much for an HTTP library to take on. As in, I don't even expect the HTTP clients to have retry support, but appreciate that Req has. :timeout as a feature makes sense, but the question then is where does it stop - i.e. what's still Reqs responsibility vs. something else's? All features breed more complexity and maintenance :grimacing:

As I wrote a bit, bottom line is I'm happy with the doc changes proposed here - everything more is a more involved library design discussion :)

[wojtekmach]

Python requests has retry by default for network errors (not 500s, for example) but it looks like everyone else has them opt-in. I'll think about it some more but I'll probably make retries opt-in for Req v1.0 since that's probably less surprising.

[whatyouhide]

@wojtekmach I was just thinking about this today and yeah, I truly start to feel like opt-in is better than opt-out here. Req retries are blocking, that is, they block the process and sleep while waiting for the next attempt. In soft-realtime systems like the BEAM, I think that behaviour is generally not lovely to have as a default behaviour; for example, you might see request latency spikes if the cause is Req retrying, and you'd have maybe not a super straightforward clue that it's cause of Req retries.

Opt-in, on the other hand, are something you probably always think about. I make a request to a server, do I need to retry it if it fails? Then I just add retry: true and Req behaves correctly by default (only GETs and co, etc).

[PragTob]

FWIW I agree but didn't want to make that call here. Glad it lead to a productive discussion though :green_heart: