By default, retry only HTTP methods known to be idempotent, user can optionally configure other methods - fixes #298 #340
Conversation
| @@ -86,7 +93,7 @@ def call(env) | |||
| env[:body] = request_body # after failure env[:body] is set to the response body | |||
| @app.call(env) | |||
| rescue @errmatch | |||
| if retries > 0 | |||
| if retries > 0 && @options.retryable_methods.include?(env[:method]) | |||
There was a problem hiding this comment.
I would rather encapsulate the check in a retry_request?(env) method and use it in this conditional
|
This is a good direction. However, I think it's essential that we allow configurability not just by HTTP method, but by the type of exception as well. How about we allow injecting a lambda via options that will be given the exception instance and the |
|
@mislav whooops, sorry for the whitespace stuff, that's an unintended change. |
|
I've tackled the unrelated CI failures on master. Please rebase to get CI passing for your PR |
|
@mislav and there it is, made it a block (much nicer, definitely) and removed all the horrible whitespace changes. And thanks for the incredibly fast and awesome feedback 👍 |
| rescue @errmatch | ||
| if retries > 0 | ||
| rescue @errmatch => exception | ||
| if retry_request?( retries, env, exception ) |
There was a problem hiding this comment.
Nitpick: we don't use spaces inside parentheses
|
Yep, looks much cleaner now. Good work. I still think we need to stop retrying non-idempotent requests by default. Yes, that will break backwards compatibility, but we can ship it in a next major release. Users who want to retry POST in some cases can explicitly do so via the retry configuration block. Could you add the restriction as a separate commit? As for the old tests that do |
|
@mislav should I make the default for non-idempotent methods at the default handling block or have it as as separate parameter? |
|
Let's always retry idempotent methods, as that should be safe theoretically. So, the internal logic hierarchy would be this: |
|
Also, the next time you're rebasing: commit message subjects should be shorter, ~50 chars, and issue and other references put in the commit message (not subject). Sorry, I'm super-nitpicky about this stuff 😈 |
|
@mislav included the idempotent_methods option and better commit messages as well 😋 |
|
Looks great, thanks!. Although, I don't think idempotent methods list needs to be configurable. We know which methods are idempotent according to spec, and for every other method a person can use So if you could take that option out, this will be ready to merge. You should rebase to master once again, as I pushed some changes to reduce possiblity for intermittent CI failures you were getting here. |
retry_if allows users to provide a block that will receive the env and exception objects and should decide if the call should be retried or not. Fixes lostisland#298.
Only run retries if the method is known to be idempotent. If it isn't, switch to use the retry_if block to make the final decision, if it is known to be idempotent allow it to be retried by default.
|
@mislav rebased and removed option :D |
Retry only HTTP methods known to be idempotent User can optionally configure other methods. Fixes #298
This changes the default at the retry middleware to retry only on methods known to be idempotent, other methods (
POST,PATCH) will not be retried by default but the user can do so by sending in a:retryable_methodsoption when creating the connection.Fixes #298.