Proposal: Improve error handling

Currently the contract is Req.request(req) :: {:ok, Req.Response.t()} | {:error, Exception.t()}, that is, users of the library might get arbitrary exceptions.

In recent releases we added a few Req exceptions:

Req.TooManyRedirectsError - previously we were raising a RuntimeError, now we return this exception
Req.TransportError - previously we were returning Mint.TransportError. We added this struct because we want to have Req.Test.transport_error(conn, reason) and so should have a Req specific exception.

Currently we automatically retry "transient errors". By that we mean 408/429/500/502/503/504 responses and exceptions with reason: :timeout | :econnrefused | :closed. We don't check the exception struct, just the reason.

We have a few issues:

#244 - should we retry all Req.TransportError exceptions? My rationale as not to was that for reason: :nxdomain it is very unlikely that retrying such would ever be successful, usually it's a typo in domain name, and so I would not consider it "transient". (IPv4/IPv6 mismatch shows up as this one too to further complicate things however.)
#295
Similarly to above, at the moment we are crashing when decoding invalid tar and zip.
#333
#329
Finch has Finch.Error exception. Should it (or just particular :reason values) be considered transient?

Proposal

Add %Req.HTTPError{reason: reason, protocol: :http1 | :http2}. It standardizes Mint.HTTPError and Finch.Error.
"transient error" means one of:
- response 408/429/500/502/503/504,
- %Req.TransportError{reason: :closed | :econnrefused | :closed}.
- %Req.HTTPError{reason: :unprocessed, protocol: :http2}
Remove compatibility guarantee (-ish, we're still pre 1.0 after all) for what "transient error" is, that is, document that we can add new status codes and exception structs/reasons in the future.

If someone is really concerned about it, they'd explicitly configure :retry for their needs.
Add Req.DecodeError:
```
%Req.DecodeError{
  format: :json | :tar | :zip | atom(), # atom() for user provided decoders
  position: non_neg_integer | nil,
  data: binary
}
```
This matches %Jason.DecodeError{position:, data:}. (There would be an additional private :token field which Jason.DecodeError has too.)

The :position field would only be used by JSON decoding at the moment but I think that's OK. There's some additional uncertainty given OTP 27 :json module; at the moment the decoding error does not return the byte offset but FWIW EEP-68 says:

The exceptions might be enhanced through the Error Info mechanism with additional meta-data like byte offset where the error occurred.

I'm on a fence whether to have %Req.DecodeError{}.data or .response, a full response struct as mentioned in #295.

Add Req.DecompressError:

%Req.DecompressError{
  format: :gzip | :br | :zstd,
  data: binary()
}

Regarding Req.DecodeError and Req.DecompressError, I personally don't intend to gracefully handle those in my apps but even though Req is very opinionated, I believe with regards to raising/returning errors it's probably better to be less opinionated nad return errors so users can work with them however they want.

OK, one way to think about different errors is whether to normalise them or not. It makes sense to normalise Mint.TransportError->Req.TransportError and Mint.HTTPError|Finch.Error->Req.HTTPError because then retry step just gonna look at these standardised errors. Furthermore, these errors are very well specified.

The problem with Req.DecodeError is custom decoders might have more specialised and appropriate errors and so we shouldn't try to shoehorn them into Req.DecodeError. Let's nor normalise them. Instead let's do the following.

decode_body will:

return %Jason.DecodeError on invalid JSON.
return %Req.ArchiveError{format: :zip | :tar, reason: any} on invalid zip/tar. Custom steps may use this exception but they don't have to.

Similarly, decompress_body will return %Req.DecompressError{format: :gzip | :br | :zstd, reason: any}. Custom steps authors may use this exception but they don't have to.

Separately, or perhaps as part of this proposal, would it also make sense to promote the generic runtime exception to be a Req-specific exception?

For context, today when using http_errors: :raise option in requests, a generic runtime exception is raised:

req/lib/req/steps.ex

Lines 2066 to 2069 in 7616d39

    
                   raise """ 
        
                   The requested URL returned error: #{response.status} 
        
                   Response body: #{inspect(response.body)}\ 
        
                   """

Having a generic runtime error can be inconvenient in situations, when using defimpl Plug.Exception (e.g. in Phoenix) to catch errors that are very specific, and to then wrap them in custom error representation.

To be able to catch exception coming from Req & handle it in a custom way, today I could probably do:

defimpl Plug.Exception, for: RuntimeError do
   # ...
end

or:

defmodule MyCustomError do
  defexception message: "That other backend is currently failing us"
end

defimpl Plug.Exception, for: MyCustomError do
  def status(_exception), do: 503
end

defmodule MyHTTPClient do
  def my_request(simulate_code) do
    try do
      url = "https://httpstat.us/#{simulate_code}"

      Req.post!(Req.new(),
        url: url,
        http_errors: :raise,
        headers: [{"content-type", "application/json"}]
      )
    rescue
      _e in RuntimeError ->
        raise MyCustomError
    end
  end
end

MyHTTPClient.my_request(503)

2nd option could probably be fine, but if I have several different HTTP client modules each using Req, with each module implementing 5-10 functions to call API individual endpoints, wrapping each with try/rescue can get quite repetitive. So I would need to roll my own abstraction to reduce duplication introduced by try/rescue. This made me wondering whether Req couldn't assist here by providing a custom exception struct out of the box? 🤔

@gmile there are multiple prior discussions about :http_errors feature, whether it should return an exception and if so, what kind of exception. It's a bit awkward feature to be completely honest.

I think in your particular case you're gonna be better off by simple pattern matching on the result. Or replace that step with a custom one that returns your custom exception.

@wojtekmach thanks, your comment made me look into documentation for implementing steps, as I haven't explored it before. I ended up with something that looks quite sensible:

defmodule ExternalServiceError do
  defexception [:message, :status]
end

defimpl Plug.Exception, for: ExternalServiceError do
  def status(exception), do: exception.status
  def actions(_exception), do: []
end

defmodule ExternalServiceClient do
  def request(simulate_code) do
    Req.new()
    |> Req.Request.append_response_steps(custom_error_handling: &custom_error_handling/1)
    |> Req.post!(
      url: "https://httpstat.us/#{simulate_code}",
      headers: [{"content-type", "application/json"}]
    )
  end

  defp custom_error_handling({request, response}) when response.status in 400..499 do
    {request, %ExternalServiceError{status: response.status, message: "We're doing something wrong"}}
  end

  defp custom_error_handling({request, response}) when response.status >= 500 do
    {request, %ExternalServiceError{status: response.status, message: "They're doing something wrong"}}
  end

  defp custom_error_handling({request, response}) do
    {request, response}
  end
end

Also, I realized that this way ^ I can also have custom, per-service error messages I want to show to my API users via ErrorJSON in Phoenix.

Feel free to mark my comments as off-topic 👍

I would not talk about the RuntimeError that @gmile mentioned, agreed that it's off topic and this has the potential to be a beefy discussion already 😄

@wojtekmach said:

The problem with Req.DecodeError is custom decoders might have more specialised and appropriate errors and so we shouldn't try to shoehorn them into Req.DecodeError.

I agree. No shoehoring. However, having Req.request/1 return {:error, Exception.t()} with any exception is too liberal IMO.

What I mean is: if Req.request/1 returns {:error, %Jason.DecodeError{}} I’m gonna be confused as hell, because where did the JSON error happen? Decoding the body? In a custom step that decodes a JSON within a header? No clue.

Instead, other than the beautiful errors you already mentioned, you could have:

defmodule Req.DecodeError do
  defstruct [:exception_reason, :maybe_more_fields_in_the_future]
 
  @type t() :: %__MODULE__{
    exception_reason: Exception.t(),
    # ...
  }
end

This way anyone can just put their nice errors within this decode error, but Req can add all sorts of useful info. Thoughts?

having Req.request/1 return {:error, Exception.t()} with any exception is too liberal IMO.

The way I see it we either have:

status quo where user can define custom steps and they can return arbitrary errors OR
a set of possible Req.*Error structs that step authors have to shoehorn arbitrary errors into. I'd consider exceptions containing exceptions in this category (nothing wrong with it) and people writing custom steps would probably end up wanting a generic error, say Req.Error, to put their custom error into. And if we have a generic Req.Error struct we're back to square 1, we don't know where it came from. So I'm not sure about this route. OR

we embrace exception in exception and have a single Req.Error{step: atom, reason: Exception.t | term()} with something like:

def message(%{step: step, reason: %{__exception__: true} = e) do
  "#{step} failed: " <> Exception.message(e)
end
def message(%{step: :verify_checksum, reason: %{expected: x, actual: x}) do
  ...
end

dunno, I kind of like the recent ones like Req.ChecksumMismatchError.

So yeah I am leaning towards 1, status quo.

exceptions in exceptions, I'm just gonna call them wrapper exceptions, are not very common in Elixir, are they? One thing that is really appealing is keeping the original exception which has its own callback implementations. For example, I proposed to have a %Req.HTTPError{protocol: :http1 | :http2, reason: term} which is... tricky because:

defexception Mint.HTTPError do
  def message(%{module: module, reason: reason}) do
    module.format_error(reason)
  end
end

and so what do I do, convert %Req.HTTPError{protocol: :http1} back to Mint.HTTPError{module: Mint.HTTP1}? But what about errors coming from other adapters? (not that realistically speaking there will be other ones but still.) I don't think it's gonna work out, I haven't thought that through.

Just to be clear, I think Req.DecodeError wrapper exception is an interesting idea. But to address this directly:

What I mean is: if Req.request/1 returns {:error, %Jason.DecodeError{}} I’m gonna be confused as hell, because where did the JSON error happen? Decoding the body? In a custom step that decodes a JSON within a header? No clue.

How does it actually play out? I think if you're explicitly checking for error in your code, pattern matching and similar, at this point you most likely want full control so might as well set decode_body: false and decode manually especially since at this point you are probably expecting one particular format (eg JSON) anyway. And when you don't handle it explicitly, let it crash, then something like a Sentry alert would get Jason.DecodeError with stack trace (so we know where it came from) and that exception has data field with data failing to decode so you have some idea what it is.

So yeah, not sure.

status quo where user can define custom steps and they can return arbitrary errors OR

My hunch is that this is too liberal. You're essentially allowing users to customize the return value of your library's function, which is a bit too free I guess for my tastes. So, while wrapper exceptions are not common in Elixir, this is a case where they sort of make sense to me because you standardize on an error (Req.DecodeError) but allow users to customize the info inside it. Also, you keep the option open for the future, still.

we embrace exception in exception and have a single ...

Nah, I really think most Req-defined exceptions make a lot of sense, like the too many redirects or checksum mismatch or whatever.

OK, before solving Req.DecodeError as I alluded to there's a problem with Req.HTTPError that I'd like to address first.

We need to transform Mint.HTTPError and Finch.HTTPError structs to Req.HTTPError. Ideally we have a helpful Exception.message/1 callback implementation. While less of a practical concern at the moment, Req supports arbitrary adapters and so if new ones appear and they have custom errors with good messages, we'd like to take advantage of them.

As a reminder we want to standardise exceptions coming from adapters in general and retry on some of them in particular. We want to retry %Mint.HTTPError{module: Mint.HTTP2, reason: :unprocessed} and possibly others. I'm not sure it matters but the http1/http2 distinction might be worth to keep and hence my idea for protocol: :http1 | :http2.

I believe we have these options:

Option 1: don't sweat it

defmodule Req.HTTPError do
  defexception [:protocol, :reason]

  @impl true
  def message(%{protocol: protocol, reason: reason}) do
    "#{protocol} error: #{inspect(reason)}"
  end
end

Option 2: Best effort and otherwise don't sweat it

defmodule Req.HTTPError do
  defexception [:protocol, :reason]

  @impl true
  def message(%{protocol: :http1, reason: reason}) do
    Mint.HTTP1.format_error(reason)
  rescue
    FunctionClauseError ->
      "#{protocol} error: #{inspect(reason)}"
  end

  def message(%{protocol: :http2, reason: reason}) do
    Mint.HTTP2.format_error(reason)
  rescue
    FunctionClauseError ->
      "#{protocol} error: #{inspect(reason)}"
  end
end

Option 3: Extensible using :module field

defmodule Req.HTTPError do
  defexception [
    :protocol,
    :reason,
    # module is considered private
    :module
  ]

  @impl true
  def message(%{module: module, reason: reason}) when module in [Mint.HTTP1, Mint.HTTP2] do
    module.format_error(reason)
  end

  # At the moment Finch has HTTP2-specific Finch.Error. Req finch adapter could transform it
  # into %Req.HTTPError{protocol: :http2, reason: reason, module: Finch}. In other words,
  # `module: Finch` is a sentinel value.
  def message(%{module: Finch, protocol: :http2, reason: reason}) do
    "#{protocol} error: #{inspect(reason)}"
  end

  def message(%{module: module, protocol: protocol, reason: reason}) do
    module.format_http_error(protocol, reason)
  end
end

Any other options?

Btw, at the moment Req.TransportError is the following:

defmodule Req.TransportError do
  defexception [:reason]

  @impl true
  def message(%__MODULE__{reason: reason}) do
    Mint.TransportError.message(%Mint.TransportError{reason: reason})
  end
end

and I wouldn't sweat it, these errors should mostly be POSIX errors + :closed, :timeout, etc so pretty uniform. However a similar :module approach could be used.

Anyway, I wanted to mention all this with relation to the potential %Req.DecodeError{exception: Exception.t} because it is attacking the same problem, have standardised exception struct name and possible other fields + good Exception.message/1 implementation.

@wojtekmach I don't have a strong preference on any of these. I guess my soft preference would be to start out with :protocol/:reason, you can always add the private :module field later.

I’d too start small but then again how do we actually implement it? Option 1 or option 2 or there is another option I didn’t consider?

I like two personally but again, they're easily swappable so I’m not sure it's a blocker?

Good point, thanks, I'll go with that.

OK, so about %Req.DecodeError{exception: %Jason.DecodeError{}}.

I think it's definitely compelling to have an exception struct that I control so that I can grow it in the future.

I kind of like this uniformity: decode_body(Req.Request.t) :: {Req.Request.t, Req.Response.t | Req.DecodeError.t}.

I'm planning to add exception that represents possible zip/tar errors that decode_body supports too, I'm thinking: Req.ArchiveError{format: :tar, reason: :eof}. And so %Req.DecodeError{exception: %Req.ArchiveError{}} feels odd, wrapping my own exceptions.

Regarding uniformity,

decode_body(Req.Request.t) :: {Req.Request.t, Req.Response.t | Jason.DecodeError.t | Req.ArchiveError.t}

is still pretty uniform imho.

A long time ago I was thinking that decode_body should accept a :decoders option, for example, decoders: [json: &CustomJSON.decode/1, html: &EasyHTML.parse/1], so it's trivial to plug in custom decoders. (Because decode_body already figures out the format thanks to MIME.) I guess in that world a custom Req.DecodeError exception makes more sense (still not 100% convinced) to keep the return value uniform. I didn't end up doing that because yeah it is pretty easy to just add a custom decoder step today anyway.

@whatyouhide wrote:

What I mean is: if Req.request/1 returns {:error, %Jason.DecodeError{}} I’m gonna be confused as hell, because where did the JSON error happen? Decoding the body? In a custom step that decodes a JSON within a header? No clue.

yeah I can see that. But I could also argue that either the custom step (decoding JSON from a header) should return a custom exception or we should decode response body ourselves to have full control. Of course there's is a balance to strike, if we're gonna end up, say, decoding JSON manually on all bigger real-world projects then it means the default are not great.

So yeah that's where I am at, still figuring this out. 😅

@wojtekmach what happens when you use Erlang's json module in the future? You won't have a Jason.DecodeError anymore?

exactly, right? if someone was pattern matching on %Req.DecodeError{exception: %Jason.DecodeError{}} they would be screwed anyway. And if someone was pattern matching on just %Req.DecodeError{} what are they gonna do with it? Log it? Retry (to what end)? If you need precise control disabling automatic decoding is trivial.

But to answer, yeah, I'm not sure what I'm gonna do.

Maybe I should document that returned exception is not part of stability guarantee in that particular step and if you care, don't use it.

And if someone was pattern matching on just %Req.DecodeError{} what are they gonna do with it?

Probably log it or alert, not match on Jason.DecodeError is my guess. But either way yeah, maybe just leave Exception.t/0 now to leave your options open so you can

Yeah, I'll go with returning %Jason.DecodeError{} for now, there's time before v1.0 to change things still.

Thank you so much!

Done!

You're the best 💟

	raise """
	The requested URL returned error: #{response.status}
	Response body: #{inspect(response.body)}\
	"""