Don't call retry_block unless a retry is going to happen

[jrochkind]

Description

Prior to this PR, retry_block could be called in some cases where a retry would not happen. I consider it a bug, and thus not a backwards incompat to change, but feedback welcome?

Prior to PR, retry_block would not be called if for instance a custom retry_if said not to retry. But it still would be called if if a retry-after header would exceed max_interval, even though no retry would be taking place.

After this PR, a present retry_block is only called if and only if a retry is actually going to happen. It will not be called if a retry is not going to happen because max_interval has been exceeded by a retry-after header.

Additional Notes

With basic retry_block specs that didn't exist before.

What I really wanted is the actual sleep_amount to be available to the retry_block, so i could log "retrying again in #{sleep_amount}". Looking for a way to do that is what made me spot this bug. But I am not sure if there's any way to add that in a backwards-compatible way -- adding another arg to the block is probably not backwards compatible? Is there any other way to slip it in there?

Appreciate any advice on if there's a feasible way to make sleep_amount available to the retry_block, for a possible subsequent PR.

[iMacTia]

Hi @jrochkind and thanks for spotting this!
I completely agree this fixes a legitimate bug, and I also agree this change is backwards-incompatible.
But fear not, main branch currently holds the code that will become Faraday 2.0, so it's perfectly fine to get this merged in.

Please add these things to the PR and I think it should be ready to be merged:

1. CI is failing due to linting, could you please review and fix that?
2. Could you add a note about this breaking change in UPGRADING.md?
3. It's OK if you want to add sleep_amount to the parameters sent to the block. Just remember to update the method documentation and UPGRADING.md accordingly. I may be wrong, but I believe this won't break existing implementations because Procs can accept more parameters without breaking (as opposed to lambdas), but it would be great if you could confirm this

[jrochkind]

@iMacTia Thanks for the quick response! Yes, I'll fix the linting (thought I already did but have been fighting with rebasing) and add to UPGRADING.

On backwards compat...

1. While proc can have variable arguments, lambda specifically cannot. And the user could pass a proc or lambda in as an arg, and the docs show lambda with "stabby lambda" syntax, which would break if we add an arg. BUT if we're open to backwards compat changes for 2.0....
2. But if we're open to backwards compat for 20.0, what about not only adding an arg for sleep_amount (that might not be the right word for a public arg), but also maybe changing them to keyword args, which have been supported in procs/lambdas for... I can't find the version, but a while now? Not sure about this one, appreciate feedback, but the number of args (plus adding one more!) seems to me like keyword is more convenient than positional.
3. There is a current arg which is shown in docs as retries but is really "number of retries remaining", which I'm not sure is what people would expect or is most useful -- what about changing it to actual retries/retry index (0- or 1-based?)

Appreciate any thoughts!

[iMacTia]

Points 1-2 make sense! I was thinking about that as well but was hoping that one more parameter would be easier to migrate to. We need to ensure Ruby 2.6 supports them though, as that will be the minimum Ruby version that we'll support with Faraday v2.0.

As for point 3, I agree retries is not self-explanatory, so we might as well use this opportunity to rename it to something else (e.g. retries_left). We could also add an alternative retired retried that instead says how many retries were performed.

I wouldn't like this PR to blow up though, there are a few good ideas here but we don't necessarily need to have them all implemented together. I'd suggest adding to this PR only the strictly necessary changes (i.e. backwards-incompatible changes), moving the rest to a separate PR. Does that make sense?

[jrochkind]

@iMacTia I agree I would do the others as a separate PR, I think this PR is good as is!

I don't understand "retired". I would personally replace the current retries with a param that is if this is first or second or third retry -- I feel like this is what I want to know rather than how many retries are left. (You can calculate the converse yourself either way). But I'm not sure what it should be called, or if it should be 0 or 1 indexed. But I don't think there's a need for both -- if you want to leave the current one, then maybe we'll just leave it as-is, but with a new name, like retries_remaining? Let me know.

If we switch to keyword args, the names get even more important, compared to positional args.

I am positive keyword args in procs/lambdas are fine in ruby 2.6 -- not sure what version they were introduced, but certain it was before 2.6, so if you aren't worried about before 2.6 it's not a problem.

I would love to submit that as a separate PR once this one is merged!

[jrochkind]

(I believe keyword args in procs/lambdas may have been introduced in ruby 2.2.0. https://docs.ruby-lang.org/en/2.2.0/Proc.html Sounds like that's fine for faraday 2.0!)

[iMacTia]

I'm sorry @jrochkind I meant retried 🤦‍♂️! I've updated my message above.
retried would hold the number of retries that already happened, as opposed to how many are left.

Good plan, I'll review this PR as soon as I have time and we can address the rest in another one 👍

[iMacTia]

@jrochkind just a quick nudge that I'm considering moving the retry middleware into a separate middleware gem as of 2.0.
The main reasons are that the middleware is quite big and hard to maintain, doesn't have any specific tying to other Faraday internals and can be separately versioned when we want to introduce breaking changes.
Also, we're quite close to releasing Faraday 2.0 and I don't want this to be a blocker

[jrochkind]

Hello, I've been on vacation for the new year holidays since our last exchange, but can submit something on the re-factoring of args on Monday, when I'll be back, whether that's here or another repo!

I'm not sure what the name and semantics of the retries argument should be -- I tend to think it should count up, rather than down. (Note the retry_block is called before waiting the delay that will be waited before the next retry!)

I think it's good to change these arguments' semantics -- and try to get it right -- at the same time as a switch to keyword args, because that keyword arg switch will be an obvious "fail quick" backwards incompat, instead of something that has a greater risk of not raising but not doing what you want, or raising in a more confusing way.

Thank you for your engagement on this, and for your maintainance of faraday!

[iMacTia]

Hey @jrochkind, no worries at all! We're all busy spending some time with our families and loved ones!
I totally agree with everything you said, and I think we can probably do a bit more planning on it to be sure we're on the right track. Potentially we could see how different approaches would compare with some code examples or even just a draft PR, which will surely help us make a better idea!

I can confirm I've now moved the retry middleware into its own repository and it is being removed from the Faraday 2.0 release (#1356). This allows me to ship Faraday 2.0 when ready, and we can take our time to review the best approach for this change directly in the faraday-retry repository, then ship it as a major version later on 👍

[jrochkind]

OK I will PR there shortly.

Since you've already released it as 1.0, the changes we're talking about will require a 2.0 release. Not sure if that is what you were intending. You are ok with a quick 2.0 release very soon on the heels of that 1.0 @iMacTia ?

[jrochkind]

@iMacTia Your CHANGELOG for new 1.0 faraday-retry says:

This release consists of the same middleware that was previously bundled with Faraday but removed in Faraday v2.0.

But it actually also includes the bugfix right here in THIS PR that was never included in a Faraday release, no? Should that be noted? I did include a note on this here in the faraday UPGRADING.md -- which now maybe needs to be removed from there, since it no longer applies to subsequent faraday release?

Phew, a lot of confusing bookkeeping.

I'm currently trying to take the code I had already written to be PR'd here, and move it to the new files in the new repo.

[iMacTia]

@jrochkind yes a 2.0 release shortly after 1.0 is perfectly fine! 1.0 is really just supposed to be a "moved-out" version.

Good shout on the change, I didn't remember that was also technically backwards-incompatible. Although we might argue that was actually solving a bug rather than changing the behaviour, so I doubt that's gonna cause any issue.

Still, I agree it should be noted on faraday-retry as you suggested 👍

[jrochkind]

Oh I do NOT actually consider the change here in #1350 to be backwards incompat! Although it's debatable, that's not my opinion.

But we list changes that aren't just backwards incompats, bugfixes and such too, right?

[iMacTia]

Yeah that makes sense 👍, and it makes me feel better knowing that we both think the change is not dangerous 😄!