Add .retriable feature to Http

Bertg commented 4 years ago

Based on the great initial POC of @ixti this should complete #595

Delay by default backs backs off over time
Maximum delay time
Exceptions to retry from
Status codes to retry from
Custom retry logic
Respect Retry-After header if present
on_retry callback

Remarks:

RuboCop report is clean
Tried to manually update the method comments, but I'm not sure if that is correct

Bertg commented 4 years ago

@paul & @ixti If you have any remarks on the PR, let me know, and I'll see to them

ixti commented 4 years ago

I had no time to do a full review, but there's one thing that I would like to be changed. Default list of statuses to retry should be 5XX. So probably for statuses it's better to provide proc.

Bertg commented 4 years ago

@ixti Took your feedback, and made the 500+ range auto retry.

However, I would argue that the library should not rely any status code by default. Here is why:

When a server is throwing 500 there is a good chance that it is experiencing trouble. By retrying by default we are making the situation worse. The same logic if followed in the faraday gem.

Bertg commented 4 years ago

I'll also wait to to comply with the code climate feedback, until the all human feedback is processed ;)

Bertg commented 4 years ago

I think handling different types of retry statuses is unneeded complication and supporting proc only is more than enough - it will handle all the cases.

The idea is to allow for a beautiful interface for the developer.

HTTP.retriable(retry_statuses: 400..499)
HTTP.retriable(retry_statuses: [404, 500..599])

looks a lot better than

HTTP.retriable(retry_statuses: ->(s) {  (400..499).cover?(s) })
HTTP.retriable((retry_statuses: ->(s) { s == 404 || (400..499).cover?(s) })

I could move that convienece into Chainable#retriable but I don't believe that is the best place for it.

Also I think exception retry proc and status retry should be separated.

I get that. However one of the options a developer can pass is a proc should_retry to decide to retry or not, based on exception or status code. I'm trying to use the same logic here. I guess I could make a method that looks something like this:


def perform(client, req)
  1.upto(Float::INFINITY) do |attempt| # infinite loop with index
    err, res = try_request { yield }

    if retry_request?(req, err, res, attempt)
      begin
        wait_for_retry_or_raise(req, err, res, attempt)
      ensure
        # ...
        client.close
      end
    elsif err
      client.close
      raise err
    elsif res
      return res
    end
  end
end

def retry_request?(req, err, res, attempt)
  if options[:should_retry]
    options[:should_retry].call(req, err, res, attempt)
  elsif err
    options[:exceptions].any? {|err_class| err.is_a?(err_class) }
  else
    options[:retry_statuses].cover?(req.status)
  end
end

Would you prefer this?

Bertg commented 4 years ago

Made some updates!

extracted the delay logic into its own class
less procs & proc building overall
stopped handling 500+ HTTP status codes by default (see previous comment for argumentation)

Bertg commented 4 years ago

Anything I can do to help progress this PR towards merging?