Honor Retry-After header #773

bronislav · 2018-02-19T16:52:29Z

Description

Improve retry interval calculations to honor Retry-After header if present. This header may be set in case if a server does not respond or request being throttled.

Todos

List any remaining work that needs to be done, i.e:

Tests
Documentation

Additional Notes

I thought to create separate middleware for this purpose but then decided to just improve existing retry logic.

iMacTia

Hi @bronislav and thanks for taking the time to work on this.

I like the additions you're bringing into the retry middleware, but I think the implementation can be improved, so I left some comments.

Please let me know if I wasn't clear in any of them.

iMacTia · 2018-02-20T13:44:04Z

lib/faraday/request/retry.rb

    end

    def call(env)
      retries = @options.max
      request_body = env[:body]
      begin
        env[:body] = request_body # after failure env[:body] is set to the response body
-        @app.call(env)
+        @app.call(env).tap do |resp|
+          raise Faraday::Error::RetriableResponse.new(nil, resp) if @options.retry_statuses.include?(resp.status)


I can see what you're trying to do with Faraday::Error::RetriableResponse, however I don't think it's a good idea to introduce this exception and use it this way.
The reason is that, after we reach the maximum number of retries, the new exception will be propagated outside and this might be an unexpected behaviour compared to what happens now.

I like the idea of allowing to define a list of statuses that you want to retry, but this should be tested internally without raising any exception.
My suggestion is to add this to retry_request?.

I've tried to do this without raising an exception but this will lead to implementing two places with retry logic which seems a bit overhead. It could be done using do ... while loop but it looks hacky for me.

And I'll move statuses testing to the better place.

I've tried to move this line around but it seems that this is the best option.

iMacTia · 2018-02-20T13:54:14Z

lib/faraday/request/retry.rb

+      retry_after = calculate_retry_after(env)
+      interval    = current_interval + random_interval
+
+      retry_after && retry_after >= interval ? retry_after : interval


This method looks a bit too complicated now, so I would separate the logic to calculate the retry interval into a separate method (similarly to what you did with calculate_retry_after).
This new method could be called calculate_retry_interval(retries).

iMacTia · 2018-02-20T13:57:08Z

test/middleware/retry_test.rb

-      assert_equal middleware.sleep_amount(5), 0.1
-      assert_equal middleware.sleep_amount(4), 0.2
-      assert_equal middleware.sleep_amount(3), 0.4
+      assert_equal middleware.sleep_amount(5, {}), 0.1


We can use the new calculate_retry_interval for this and the following tests so we don't need to pass {} every time.

iMacTia · 2018-02-20T13:58:54Z

test/middleware/retry_test.rb

+        conn(:max => 1, :interval => 0.1, :retry_statuses => [429]).get("/throttled", params)
+      }
+
+      assert_in_delta 0.5, Time.now - started, 0.04


I'm a bit worried by this assertion, it might create false-positives if test execution is too slow.
Any reason why we don't simply test that Time.now - started > 0.5?

Agree. We don't care about delta in this case.

bronislav · 2018-02-20T15:46:32Z

lib/faraday/request/retry.rb

          retry
        end
-        raise
+
+        raise unless exception.is_a?(Faraday::Error::RetriableResponse)


@iMacTia I've silenced exception here, but not sure that this is acceptable and very good solution

iMacTia · 2018-02-20T17:32:14Z

I agree this is not the most elegant solution, but it seems the easiest one without refactoring too much.
I'll try to think of a better way to do this.

bronislav · 2018-02-20T19:13:17Z

I've added support for timestamp in the Retry-After header according to MDN https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Retry-After

I've also stop retrying if retry_after interval is higher than max_interval

bronislav · 2018-02-22T18:58:48Z

@iMacTia Any chance to merge it soon?

iMacTia

Review comments have been addressed, test coverage is good. The exception solution doesn't look like the best one but I couldn't think of a better approach, so let's get it merged as it is and improve later on

bronislav · 2018-02-22T21:07:59Z

@iMacTia Thank you.

iMacTia · 2018-02-22T21:09:17Z

@bronislav Merged into master, will probably release next week as this weekend I'm unavailable in case of issues 😅
If you get a chance to test master branch in a real-world application than feedback would be welcome 😃

bronislav · 2018-02-22T21:11:09Z

Yes. I'm going to use it from master and test toward real project tomorrow. Will let you know.

bronislav · 2018-02-23T16:53:04Z

I found one configuration issue: if user enable :raise_error middleware or raise custom exceptions on particular responses, those exceptions should be included in the retry middleware configuration explicitly.

This just documentation issue, I think.

iMacTia · 2018-02-28T17:52:48Z

@bronislav that should be in line with what happens currently and depends on how you set the middlewares in your stack, but thanks for raising this 👍

fabiokr · 2018-03-23T15:50:41Z

lib/faraday/request/retry.rb

        end
-        raise
+
+        raise unless exception.is_a?(Faraday::Error::RetriableResponse)


@bronislav @iMacTia Hey all, I've tried to use :retry_statuses, but it seems that this portion makes it return a nil response and no exceptions are raised when it gives up retrying the RetriableResponse, which was unexpected for me. Maybe in that case it would make more sense to return the last resp, and then let the user handle the response as he sees fit?

@fabiokr I see what you mean. Would you mind to create a pull request for this change?

You're right @fabiokr, we missed what to do in case we reach this point (too many retries) with a Faraday::Error::RetriableResponse. I agree in that case we should simply return the response, which should be available in the exception.

PR with demonstrating test and fix welcome!

@bronislav @iMacTia #783

vvo · 2020-01-06T10:36:30Z

Hi there, should we document the fact that retry-after headers are already handled? this was a surprise for me and I guess we could tell somewhere that it's already handled? thanks! (for example, here: https://www.rubydoc.info/gems/faraday/Faraday/Request/Retry)

Fixes #773 (comment)

iMacTia · 2020-01-08T20:13:16Z

@vvo good point, #1109 takes care of that 👍

Fixes #773 (comment)

iMacTia requested changes Feb 20, 2018

View reviewed changes

bronislav commented Feb 20, 2018

View reviewed changes

Honor Retry-After header while retrying requests.

24fac6e

bronislav force-pushed the retry-after-header branch from 772f741 to 24fac6e Compare February 22, 2018 19:00

iMacTia reviewed Feb 22, 2018

View reviewed changes

iMacTia approved these changes Feb 22, 2018

View reviewed changes

iMacTia merged commit 6ddf9c3 into lostisland:master Feb 22, 2018

bronislav deleted the retry-after-header branch February 22, 2018 21:09

fabiokr reviewed Mar 23, 2018

View reviewed changes

fabiokr mentioned this pull request Mar 23, 2018

Returns the http response when giving up on retrying by status #783

Merged

kyokutyo mentioned this pull request Nov 28, 2019

NoMethodError occuered during retry request. adelevie/parse-ruby-client#227

Closed

iMacTia added a commit that referenced this pull request Jan 8, 2020

Documentation fix

167aa59

Fixes #773 (comment)

iMacTia mentioned this pull request Jan 8, 2020

Documentation fix #1109

Merged

iMacTia added a commit that referenced this pull request Jan 8, 2020

Retry middleware documentation fix (#1109)

8a43a47

Fixes #773 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Honor Retry-After header #773

Honor Retry-After header #773

bronislav commented Feb 19, 2018

iMacTia left a comment

iMacTia Feb 20, 2018

bronislav Feb 20, 2018

bronislav Feb 22, 2018

iMacTia Feb 20, 2018

iMacTia Feb 20, 2018

iMacTia Feb 20, 2018

bronislav Feb 20, 2018

bronislav Feb 20, 2018

iMacTia commented Feb 20, 2018

bronislav commented Feb 20, 2018

bronislav commented Feb 22, 2018

iMacTia left a comment

bronislav commented Feb 22, 2018

iMacTia commented Feb 22, 2018

bronislav commented Feb 22, 2018

bronislav commented Feb 23, 2018 •

edited

Loading

iMacTia commented Feb 28, 2018

fabiokr Mar 23, 2018

bronislav Mar 23, 2018

iMacTia Mar 23, 2018

fabiokr Mar 23, 2018

vvo commented Jan 6, 2020

iMacTia commented Jan 8, 2020

Honor Retry-After header #773

Honor Retry-After header #773

Conversation

bronislav commented Feb 19, 2018

Description

Todos

Additional Notes

iMacTia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iMacTia commented Feb 20, 2018

bronislav commented Feb 20, 2018

bronislav commented Feb 22, 2018

iMacTia left a comment

Choose a reason for hiding this comment

bronislav commented Feb 22, 2018

iMacTia commented Feb 22, 2018

bronislav commented Feb 22, 2018

bronislav commented Feb 23, 2018 • edited Loading

iMacTia commented Feb 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vvo commented Jan 6, 2020

iMacTia commented Jan 8, 2020

bronislav commented Feb 23, 2018 •

edited

Loading