Question about `retry: :always` Req option

[pdgonzalez872]

Hi @wojtekmach!

Thanks for the library! I've said it many times and every time I get a chance I like to say that I love your work. I love Req too!

I wanted to reach out about the following behavior and see if I am missing something:

I created a small repro script, using mix_install_examples (again, amazing work and resource for the community :heart:) as follows:

# server.exs
# https://github.com/wojtekmach/mix_install_examples/blob/main/plug_cowboy.exs

Mix.install([
  {:plug_cowboy, "~> 2.5"}
])

defmodule Router do
  use Plug.Router
  plug(Plug.Logger)
  plug(:match)
  plug(:dispatch)

  get "/" do
    IO.inspect("Yay")
    send_resp(conn, 200, "Hello, World!")
  end

  match _ do
    send_resp(conn, 404, "not found")
  end
end

plug_cowboy = {Plug.Cowboy, plug: Router, scheme: :http, port: 4000}
require Logger
Logger.info("starting #{inspect(plug_cowboy)}")
{:ok, _} = Supervisor.start_link([plug_cowboy], strategy: :one_for_one)

# unless running from IEx, sleep idenfinitely so we can serve requests
unless IEx.started?() do
  Process.sleep(:infinity)
end

# req.exs
# https://github.com/wojtekmach/mix_install_examples/blob/main/req.exs

Mix.install([
  {:req, "~> 0.3.1"}
])

opts = [retry: :always]

Req.get!("http://localhost:4000/", opts) |> IO.inspect()

Then, we:

• run the server: elixir server.exs
• run the req to hit the server: elixir req.exs
• get the following output:

paulo_notes_hg [main] $ elixir scripts/random/req_why_200_retry/req_why_200_retry.exs

17:14:20.533 [error] retry: got response with status 200, will retry in 1000ms, 3 attempts left

17:14:21.540 [error] retry: got response with status 200, will retry in 2000ms, 2 attempts left

17:14:23.581 [error] retry: got response with status 200, will retry in 4000ms, 1 attempt left
%Req.Response{
  status: 200,
  headers: [
    {"cache-control", "max-age=0, private, must-revalidate"},
    {"content-length", "13"},
    {"date", "Thu, 13 Oct 2022 00:14:26 GMT"},
    {"server", "Cowboy"}
  ],
  body: "Hello, World!",
  private: %{}
}

I guess I misunderstood what [retry: :always] meant. I thought it would retry only if needed. If we got a 200, we would not error and try again. Am I missing something?

[wojtekmach]

Good catch. Looking back not sure what should be the semantics of :always so I will most likely remove it in favor of either more explicit options or leaving this to the user altogether which they can do today via a lambda.

out of curiosity how did you expect this would work?

[pdgonzalez872]

out of curiosity how did you expect this would work?

We thought it was a "retry until it worked" option. Then we saw retries for 200s and started to look into it more.

I also think we should mention the use-case, it may give more context to you:

We need to retry POST requests. We want exactly what :safe provides, but for POSTs.

When we saw we were not retrying (because of it being a POST) we looked into retry: :always. We actually agreed with the approach of NOT allowing that to happen since POSTs in essence alter data. But, some apis ask for POSTs that are really GETs. GraphQL deals with POSTs as well so maybe that's something to consider like you mentioned above.

Thanks for looking into this!

[wojtekmach]

We thought it was a "retry until it worked" option.

Gotcha, that's very reasonable.

We need to retry POST requests. We want exactly what :safe provides, but for POSTs.

Yeah, that's very reasonable too.

I'm thinking about introducing another value, :transient, which means http response 408/5xx or transport error :timeout, :closed and perhaps some others. And then we'd have :safe_transient which is :transient but just for safe HTTP methods.

So for you, retry until it works, it would be: retry: :transient, max_retries: a_large_integer. I'll add max_retries: :infinity too. :)

[pdgonzalez872]

excellent, that'd be great! We like the default max_retries, but maybe you are seeing something we will likely have to deal with later on and we don't know about it yet 😂

retry: :transient, max_retries: :infinity -> "don't you stop trying till this is done!"

[wojtekmach]

ah, sorry, just to be clear I think a max_retries < :infinity is the way to go.

[pdgonzalez872]

yes, for sure. We won't use infinity, I just thought it was funny 😂

[maxsalven]

Would love a built in :transient option that works with e.g. POST