Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add return-value based backoff #122

Closed

Conversation

sherifnada
Copy link
Contributor

hopefully closes #102

Putting this up for feedback. Happy to write tests/docs/etc.. after getting validation for the approach!

The basic idea is to add a new decorator, on_retry_value which takes a callable (rather than a generator). The callable takes as input the return value of the decorated method. The return value of the callable is the duration of the wait. All other params (max_wait, on_giveup, etc..) work as expected.

This allows us to handle situations where backoff depends on the return value of the method e.g: if a requests.Response object contains a retry-after type of header.

A point to highlight: This solution uses a callable rather than a generator function. This is a departure from the current patterns. However, it felt like a generator doesn't quite fit the mold here as we need to pass the return value every time we call the decorated method.


How do we achieve this now, you might ask?

We engage in forbidden necromancy and bloodmagic whereby we use the on_exception decorator with constant generator with value 0, then raise an exception (essentially) containing the length of time we should wait. Then in the on_backoff handler we read the value from the exception and sleep. theremustbeabetterway.gif

@sherifnada
Copy link
Contributor Author

@bgreen-litl hello! I can't assign you as a reviewer but I believe(?) you are the right person to tag here?

@bgreen-litl
Copy link
Member

Thanks for this. I've wanted to come up with a solution for the Retry-After header case for a while but I haven't had a chance to experiment carefully yet. I am generally pretty hesitant to significantly expand the surface area of the API without being really sure it's warranted. I would probably be reluctant to add a new decorator to address this until I convince myself it is warranted for this case. I wonder if instead we could support the optional use of a function rather than a generator in the place of the wait_gen param while using the existing decorators? One can use inspect.isgeneratorfunction() to determine if the param is a generator. If it is a callable but not a generator, perhaps we could pass in the return value in the case of on_predicate, and the exception instance in the case of on_exception?

I'm thinking of something like this:

def retry_after(request_exception):
    return int(request_exception.response.headers["Retry-After"])

@backoff.on_exception(retry_after, requests.exceptions.RequestException):
def get_url(url):
    resp = requests.get(url)
    resp.raise_for_status()
    return resp.json()

@bgreen-litl
Copy link
Member

bgreen-litl commented Apr 17, 2021

Or perhaps we could make use of send() to the generator? I have never used this feature before, but maybe this would be a use case for it and we would maintain the requirement that the wait_gen argument be a generator function.

def retry_after():
    e = yield
    yield int(e.response.headers["Retry-After"])

@backoff.on_exception(retry_after, requests.exceptions.RequestException)
def get_url(url):
    resp = requests.get(url)
    resp.raise_for_status()
    return resp.json()

@sherifnada
Copy link
Contributor Author

@bgreen-litl thanks for the great ideas. Agreed that ideally the surface area of the iface doesn't expand. Let me try some of these ideas and get back to you soon!

@sherifnada
Copy link
Contributor Author

implemented the second idea you suggested here: #124 . Looks much better! Feel free to close this PR and move the discussion there.

@bgreen-litl
Copy link
Member

Closing since this was superseded by #124

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.

Option to use the "Retry-After" header
2 participants