downstream icon indicating copy to clipboard operation
downstream copied to clipboard

Incomplete typing in Downstream.Error

Open durub opened this issue 2 years ago • 0 comments

Hey,

I've built a very simple exponential backoff to handle a specific remote which sometimes fails for no reason or timeouts due to being temporarily slow.

  defp download(url, retries \\ 3) do
    opts = [timeout: 5 * 60_000]

    # download_from_external_url uses Downstream.get internally
    with {:error, %{status_code: nil}} = e <- download_from_external_url(url, opts) do
      if retries > 0 do
        Process.sleep(1000 * Integer.pow(3, 3 - retries))
        download_call(url, retries - 1)
      else
        e
      end
    end
  end

Even though it's valid, Dialyzer complains about that:

The pattern 
          _e@1 = {error, #{status_code := nil}} can never match the type 
          {'error',
           #{'__exception__' := _,
             '__struct__' := 'Elixir.Downstream.Error',
             'device' := atom() | pid(),
             'reason' := _,
             'status_code' := non_neg_integer()}} |
          {'ok',
           #{'__struct__' := 'Elixir.Plug.Upload',
             'content_type' := 'nil',
             'filename' := binary(),
             'path' := <<_:64, _:_*8>>}}

The Downstream.Error type is defined as follows:

@type t :: %__MODULE__{device: IO.device(), reason: any, status_code: non_neg_integer()}

In the type, device and status_code aren't allowed to be nil.

However, there are two cases in which an error comes without a status_code or device (nil):

  1. When an error with :unexpected_error happens in Downstream.Download
  2. When a timeout happens in Downstream.get

Does it make sense to change the type to accommodate these scenarios? I can make the change and do a PR if it makes sense.

We can change the type to something like this:

  @type t :: %__MODULE__{device: IO.device(), reason: any, status_code: non_neg_integer()} |
             %__MODULE__{device: nil, reason: :unexpected_error, status_code: nil} |
             %__MODULE__{device: nil, reason: :timeout, status_code: nil}

Or if that doesn't work (Dialyzer has some limits when it comes to union types, but 3 should be OK), there's the simple option:

@type t :: %__MODULE__{device: IO.device() | nil, reason: any, status_code: non_neg_integer() | nil}

What do you think, Matt? @mpiercy827

durub avatar Feb 21 '23 21:02 durub