Bugfix - Attempt Number Accuracy #12
Conversation
|
Maybe it would be better to create a v1 release with this change since it is breaking? |
That's not necessary, thanks to the v0 major version right now: https://golang.org/doc/modules/version-numbers#v0-number |
|
It might not be strictly necessary, but it would be nice considering the gross nature of this change ;) I hear you though. In all seriousness, thanks for making this package. I've been using it a lot, and it works well. What other changes do you have planned before making an official v1 release? |
At this point, this has been released for over 3 months, so that ship has kind of sailed. I'll keep it in mind for the future, though. I appreciate the feedback. 😃
I appreciate it! Thank you! 😊
I've been trying to come up with a way to integrate Its been a very stable package and has received very few changes in over 5 years, so I'm in no rush. 🙂 |
What Happened?
Ooof... this is gross.
There's a couple of issues here that have existed for 5+ years:
retry.Actionfunctions are sent an attempt number, but its been erroneously 0-based. Thestrategy.Strategyalso receives an attempt number, but that's supposed to be 0-based, as its supposed to allow for pre-run-logic. Sooo, yea, the attempt number sent toretry.Actionhas been off-by-one for all these years. 😞strategy.Limitfunction has also had an off-by-one error for all these years... as the strategies are again supposed to be 0-based. Worse even, is that it used to be correct, and then somewhere along the line I thought I "fixed" it with commit 678601c. Damn. 😢The Fix
In any case, this PR fixes these errors!
The
retry.Actionattempt number will now always start at1and represent the actual attempt number of times that the action has been called, whilestrategy.Strategyfunctions will continue to receive the pre-action attempt number starting at0as usual. Thestrategy.Limitfunction has been fixed and tests have been expanded/improved to prevent this from happening again in the future. 🤞Impact
Yea, so this will definitely break backwards-compatibility. Existing
retry.Actionimplementations will have to adapt to the new attempt number being+1compared to before, and existing code that usesstrategy.Limitwill have to expect that it'll there will be one fewer attempt than before (it was wrong before and giving one too many).I'll be tagging a release once this is merged.