Retry block change

[jrochkind]

retry_block takes keyword arguments instead of positional, which is much more manageable with so many args. (keyword args on lambda/proc objects have been supported since ruby 2.1)

There are some argument changes too:

• add new will_retry_in, for number of seconds until retry that event is for
• change retries_remaining to retry_count, which counts up from 0. I think this is more likely to be what someone wants. (You can of course calculate the converse either way with options.max)

Questions:

1. is will_retry_in the right name? Best i could come up with, but happy to change to whatever you prefer.
2. is retry_count the right name? Same.
3. should retry_count start at 0 or 1? I figured 0, in part because at the point retry_block is called, no retries have actually happened yet, it's called before the retry (and in fact before waiting the will_retry_in interval). This is existing behavior. But can change if you prefer 1.
4. Is retry_block even the right name for this? As long as we're doing backwards incompats, should it be retry_proc or something instead? I didn't change it cause I have no strong feelings, but figured it was worth bringing up. Perhaps a name change would make bakwards compat even more obvious too.

• retry_block takes keyword arguments, and has a new one for will_retry_in seconds
• change retries_remaining retry_block arg to retry_count (O-based, incrementing)

[jrochkind]

OK, rubocop is telling me annoying things like the method has too many lines!

I am not sure how to make it pass rubocop without actually harming readability, but I'll first wait for feedback on the actual logic choices, before figuring out or asking for help on how to make rubocop happier.

[iMacTia]

@jrochkind don't worry about Rubocop stuff, this middleware needs good refactoring, but we can get to that later.
Feel free to update .rubocop_todo.yml so that the failing cops are passing 👍

The implementation and logic look good 🙌!

[jrochkind]

I tried extracting a separate method call_retry_block, which got rid of the existing rubocop warnings about complexity and method length -- but resulted in a new one, the class being one line longer than rubocops configured 100-logic-line limit!

i'm not sure if that change improved or harmed the actual code more readability/maintainability though.

So I guess I'll just add the existing ones to the .rubocop.todo, I'm not famiiar with that, so I'll have to figure out how it works.

Otherwise, on the various questions I asked, @iMacTia , you're thinking things are fine how they are? Or just don't have energy to think about them?

[iMacTia]

So I guess I'll just add the existing ones to the .rubocop.todo, I'm not famiiar with that, so I'll have to figure out how it works.

Tha file is auto-generated, you just run rubocop -A --auto-gen-config and it will be updated automatically 😃

Apologies about the questions, today was Faraday 2.0 release day and I've been extra busy, will go through them now:

1. will_retry_in makes sense, I can see the code documentaiton already says it's a seconds value 👍
2. retry_count also makes sense, I believe this is also what sidekiq uses 😄
3. Starting at 0 sounds better, for the reasons you mentioned. Again the code documentaiton is clear on that 🙌.
4. This is a good point, but there's one thing that scares me. This could cause silent failures. If we rename the option, then values that are being passed today with the old name will simply be ignored, and the block will stop executing. If we rename it then we should also put a guard to raise an error (or a warning, and support both?) in case the old option is passed. We could then drop it in v3.0.

[jrochkind]

Thanks!

Hm, I thought unrecognized options would raise. If unrecognized options are silent failures... yeah.

I think that is not great, I think best contemporary ruby practice is to have unrecognized keyword args immediately raise an ArgumentError, because having typo or old args be silent failures is bad news generally. using actual ruby 2.0+ keyword arguments, you get ArgumentError on unrecognized kw arg automatically, I think the silent failure came from the "path of least resistance" back when "keyword arguments" were always just Hashes.

But looking at the actual code here... which probably comes from pre-ruby 2.0 days too.... it's not obvious to me how to make that so, and it's probably an issue for Faraday in general. I guess maybe Options.from would have to take an allowed_keys arg and raise on the missing ones... or something...

So okay, for another (maybe, probably not) day.

And we'll leave the option name alone, it's fine even if not ideal!

Thank you for your help!

[jrochkind]

@iMacTia Note rubocop would add 'rubygems_mfa_required' => 'true' to gemspec with -A), but I won't do that, don't know if you want that/use MFA on rubygems, that's up to you to do. :)

[jrochkind]

Gah, I might be using a different version of rubocop than CI, cause that thing has become an issue Error: unrecognized cop or department Gemspec/RequireMFA found in .rubocop_todo.yml -- I'll hand edit the .rubocop_todo.yml to get by that, just trying to get get by here!

[iMacTia]

The above is all true for keyword arguments, but as you correctly pointed out we don't use those in the initialiser. Instead we use some old custom RetryOptions, which I'd gladly get removed in a future refactoring

[jrochkind]

OK, it's green!

Since I have you here I"m going to take the liberty of mentioning something else unrelated -- congrats on Faraday 2.0 release. In my communities people are trying to figure out how to make a gem that can work with faraday 1 OR 2 -- since the apps using our gems (that use faraday) may have other dependencies still stuck on faraday 1, we need to support faraday 1 or 2 for a while.

Like... maybe Faraday.default_adapter ||= :net_http? Are there other gotchas? The middleware moved out to other gems that are not dependencies already, like retry -- if you add faraday-retry dependency but are using faraday 1.0 still, it shouldn't disturb anything?

Some of this might need to be figured out in practice, but since Faraday is so often used by gems ("intermediate dependencies"), it would probably be a useful addition to the upgrade guide if anyone can figure out good patterns for an intermediate dependency supporting both Faraday 1 and 2, at app's choice.

[iMacTia]

Thanks for raising the point regarding upgrading to Faraday 2.0, I've had a few people reaching out already with similar comments. I'm currently thinking what's the best way to tackle this and could summarise them on a specific doc.
The biggest issue is still the fact that there will be some "optional dependencies" and those might be hard to tackle.

If you could point me to where this is being discussed in open communities, it would provide me with valuable use-cases!

[jrochkind]

@iMacTia

Some discussion was on a slack, but here is some public discussion. It's a little bit confusing.

• restrict faraday version to 1.x rsolr/rsolr#228
• Add an explicit fallback to net_http to support both faraday 1.x and 2.x implementations rsolr/rsolr#229

And here's a problem I noticed if your gem uses retry, and you want to allow any consuming apps to use either faraday 1.x or 2.x, there's really no great way to. :(

• Problems with retry_503 option rsolr/rsolr#230

I wonder if you want to consider adding a faraday-retry dependency back to faraday for now, like you did with other extracted middleware.

(I also wonder if default_adapter should have remained :net_http, but that ship has sailed).

[iMacTia]

I wonder if you want to consider adding a faraday-retry dependency back to faraday for now, like you did with other extracted middleware.

A potentially better solution would be to remove them from the codebase of Faraday 1.x as well, and add them back as external gem dependencies. This way you'd be able to add them to your gem as well and it would work fine with both Faraday 1.0 and 2.0 (the correct version would be loaded accordingly)

[jrochkind]

Yes, that's an interesting idea. In Faraday 1.0 faraday-retry (~> 1.0) could be an actual stated dependency as an external gem -- in faraday 2.0, it would not be listed as a dependency. But an intermediate dependency could list it -- and that would work with faraday 1.0 (just duplicating what's already listed), or faraday 2.0 (adding the dependency).

One trick would be that we have some intermediate gems who currently allow faraday down to >= 0.9.0, and would like to leave that there if possible. is it possible for faraday-retry gem to have a gemspec that allows working with faraday down to... 0? I'm not sure the exact minimum version faraday-retry 1.0 will work with for real... But that'd be easy to figure out with the automated tests. And then set it's gemspec requirement accordingly -- so long as it's at least 0.9.0, that works for me at least.

But not sure what happens if you load the faraday-retry gem with an older existing faraday 1.0 that still has it's own retry middleware...

So okay, another alternate option would be to make faraday-retry gem no-op if loaded with faraday 1.x. Make sure it avoids registering itself with faraday in that case, it just sits there as unused code. So an intermediate gem could express it as a dependency -- in faraday 1.0, it'll sit there as un-needed un-used code, but won't hurt anything. In faraday 2.0 where it's needed, it'll be there.

All of this of course applies to faraday-multipart too, although that one doesn't effect me (as far as I know yet!)

[iMacTia]

@jrochkind FYI -- lostisland/faraday#1367

[iMacTia]

is it possible for faraday-retry gem to have a gemspec that allows working with faraday down to... 0?

That might be a bit hard to be fair. We're not actively maintaing Faraday 0.x so the required ruby version there would be hard to satisfy. Also, Faraday 1.0 is basically backwards-compatible with 0.x

But not sure what happens if you load the faraday-retry gem with an older existing faraday 1.0 that still has it's own retry middleware.

I played with this locally a bit, and it seems fine if you have it on the gemfile, as far as you don't require it.
This is the workaround I did in faraday-multipart to achieve that.