Fix Exception throw by Retry(exc parameter)#6433
Fix Exception throw by Retry(exc parameter)#6433Ixiodor wants to merge 1 commit intocelery:masterfrom Ixiodor:patch-3
Conversation
When Retry has paramater "max_retries" set, that value is valid only for "Retry Exception" and not for exc launched from "Retry Exception". So using Retry with max_retries = 0, exc exception still uses the "default max_retries" making use of "max_retries" useless. And override is an override. max_retries init value to -1 is for not preventing "None" as value for "infinite retry". Actually i changed the behaviour you wanted to apply. You wanted to use max_retries as an additional check outside the normal "max_retries". But there is a double exception begin executed and the second one dosn't follow the caller(Retry) parameters.
auvipy
left a comment
There was a problem hiding this comment.
regression tests and entry to the changelog is needed as well
|
|
||
| def retry(self, args=None, kwargs=None, exc=None, throw=True, | ||
| eta=None, countdown=None, max_retries=None, **options): | ||
| eta=None, countdown=None, max_retries=-1, **options): |
There was a problem hiding this comment.
is max_retires=-1 actually needed?
There was a problem hiding this comment.
Needed cause "None" is used for "unlimited retries". Elsewhere each retry without "max_retries" set will be converted to "unlimited retries" indirectly
| request = self.request | ||
| retries = request.retries + 1 | ||
| max_retries = self.max_retries if max_retries is None else max_retries | ||
| if max_retries is None or max_retries >= 0: |
There was a problem hiding this comment.
this change seem right but needs regression tests
|
@mchataigner Can you help on that for tests? |
|
@Ixiodor do you have a code sample so that I may understand better the issue here? |
|
@mchataigner This example will run always 11 times. No matter which route. |
|
I found a trick: That works cause Retry is protected from autoretry_for. Should we create another protected Exception? (Something like BypassException) That incapsulate the real Exception and on except will raise the one inside BypassExpcetion container. |
|
Made a PR that seems better than this. |
Description
When Retry has paramater "max_retries" set, that value is valid only for "Retry Exception" and not for exc launched from "Retry Exception".
So using Retry with max_retries = 0, exc exception still uses the "task max_retries" making use of "max_retries" useless.
An override is an override.
max_retries init value to -1 is for not preventing "None" as value for "infinite retry".
Actually i changed the behaviour you wanted to apply. You wanted to use max_retries as an additional check outside the normal "max_retries". But there is a double exception begin executed and the second one dosn't follow the caller(Retry) parameters.