Add .retriable feature to Http

[Bertg]

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]

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

[ixti]

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]

@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]

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

[ixti]

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. Also I think exception retry proc and status retry should be separated.

[Bertg]

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]

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]

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

[rkhapre]

This is available now in Pre 5.x releases?

[yashtewari]

Any updates on this? @ixti are you still in favor of adding retry functionality to Http?

[tarcieri]

The 5.x release is out. If you are still interested in pursuing this, can you rebase?

[czj]

I would still love to get this PR merged :-)
Is there any way we could help @levups ?

[ixti]

I can try to take a look on this next week.

[czj]

That's very kind @ixti ! Thank you.

[tomprats]

Would love to have this. I looked into major changes since it's release and besides the hashrocket one, I don't think anything changed to break this.

@ixti would you mind taking a look again?

If you want, I can rebase, but I think if I do it, it'd have to go into a new PR

[ixti]

Sorry been swamped with lots of stuff. Will take a look at the rebased PR as soon as possible. And hope to make it land this month :D

[danielb2]

@ixti any progress?

[wilsonsilva]

This is neat! I would love to get this out of the box.

[tarcieri]

I would be okay with merging this if someone fixed the merge conflicts

[tomprats]

@tarcieri I fixed it on #775

[tarcieri]

Oh hey, sorry about that. Will take a look soon.

[tarcieri]

Merged in #775

[Bertg]

Hurray!

It took 1608 days to get the original work by @ixti to make it in. Not the fastest PR to ever get merged, but glad it did and to be a part of it :)