Skip to content

Fix Exception throw by Retry(exc parameter)#6433

Closed
Ixiodor wants to merge 1 commit intocelery:masterfrom
Ixiodor:patch-3
Closed

Fix Exception throw by Retry(exc parameter)#6433
Ixiodor wants to merge 1 commit intocelery:masterfrom
Ixiodor:patch-3

Conversation

@Ixiodor
Copy link
Contributor

@Ixiodor Ixiodor commented Oct 22, 2020

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.

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.
Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is max_retires=-1 actually needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change seem right but needs regression tests

@auvipy auvipy added this to the 5.0.2 milestone Oct 23, 2020
@Ixiodor
Copy link
Contributor Author

Ixiodor commented Oct 23, 2020

@mchataigner Can you help on that for tests?

@mchataigner
Copy link
Contributor

@Ixiodor do you have a code sample so that I may understand better the issue here?

@Ixiodor
Copy link
Contributor Author

Ixiodor commented Oct 23, 2020

@mchataigner
Use somenthing like that:

import random

@app.task(name="TestTask", bind=True, autoretry_for=(Exception,),
                  max_retries=10)
def foo(self):
    number = random.randint(0, 1)
    if number % 2 == 0: # I wanna retry in that case
        print(str(number) + " : going to retry")
        self.retry(exc=Exception('Trying again...'))
    else: # I wanna fail elsewhere
        print(str(number) + " : going to fail")
        self.retry(exc=Exception('You failed'), max_retries=0) # That one should tell to stop retries

foo.si().delay()

This example will run always 11 times. No matter which route.
I wanna retry in some cases but i wanna fail in other cases, i wanna fail forcing no more retries. Retry is an Exception, so it get caught by autoretry_for=(Exception,), Retry will check for max_retries and max_retries is 0, so it should stop retry and launch the exc exception, but exc exception will be caught again by autoretry_for=(Exception,) and in this case you can't override max_retries, so the decorator(or default) max_retries will apply, making max_retries override in self.retry useless. This patch set a new max_retries for task and it's wrong with the starting idea, we have to find a way to set max_retries on exc launched by self.retry with same values overrided with self.retry or make exc escape from autoretry_for.

@Ixiodor
Copy link
Contributor Author

Ixiodor commented Oct 24, 2020

I found a trick:

    else: # I wanna fail elsewhere
        print(str(number) + " : going to fail")
        self.retry(exc=Retry('You failed'), max_retries=0)

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.

@Ixiodor
Copy link
Contributor Author

Ixiodor commented Oct 24, 2020

Made a PR that seems better than this.
#6436

@Ixiodor Ixiodor closed this Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants