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 support for an async defined retry and callback_error_retry #289

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

symphony-youness
Copy link

This issue raises the need for an asynchronously defined error callback: #249

This is a first approach to be able to use an asynchronously defined retry (on every retry iteration) and callback_error_retry (after the retrying is stopped)

  • I added tests for both
  • I added an await in call and anext to be able to use the context manager (passed async context manager tests)
  • I had to override the iter() in AsyncRetrying to define it as async

@mergify
Copy link
Contributor

mergify bot commented Apr 13, 2021

⚠️ No release notes detected. Please make sure to use reno to add a changelog entry.

- BLK100 error
- import order errors
@symphony-youness
Copy link
Author

Hi @jd ,
I checked out the project again and directly ran tox -e pep8 and it fails with the same errors shown in the circleci output.

@symphony-elias
Copy link

symphony-elias commented Apr 20, 2021

Hi @jd ,
I checked out the project again and directly ran tox -e pep8 and it fails with the same errors shown in the circleci output.

Indeed, pep-8 error seems not to be related to this PR

Comment on lines +2 to +16
prelude: >
Example use cases:
- if we get logged out from the server
- if authentication tokens expire
We want to be able to automatically refresh our session by calling a specific
function. This function can be asynchronously defined.
features:
- |
Asynchronous defined retry callbacks:
- retry
- retry_error_callback
issues:
- |
#249

Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
prelude: >
Example use cases:
- if we get logged out from the server
- if authentication tokens expire
We want to be able to automatically refresh our session by calling a specific
function. This function can be asynchronously defined.
features:
- |
Asynchronous defined retry callbacks:
- retry
- retry_error_callback
issues:
- |
#249
features:
- Allow to define asynchronous defined retry callbacks by using `retry` or `retry_error_callback`

@@ -69,6 +122,7 @@ async def __anext__(self):

def wraps(self, fn):
fn = super().wraps(fn)

Copy link
Owner

Choose a reason for hiding this comment

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

unrelated 😁

@@ -30,12 +37,58 @@ def __init__(self, sleep=sleep, **kwargs):
super(AsyncRetrying, self).__init__(**kwargs)
self.sleep = sleep

async def iter(self, retry_state): # noqa
Copy link
Owner

Choose a reason for hiding this comment

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

why the noqa here? 🤔

I'm ok on the idea, but the implementation has too much copy pasting from the parent iter. It needs better refactoring for applying DRY.

fut = retry_state.outcome
if fut is None:
if self.before is not None:
self.before(retry_state)

Choose a reason for hiding this comment

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

Can you also add async support for before and after callbacks?

@mastizada
Copy link

mastizada commented Jan 24, 2022

@jd @fadedDexofan it has been almost a year since the last update on this PR, is there any developments? Do you guys need a help?

Edit: Created a PR #363 to allow async callback for all parameters.

@el-youness
Copy link

I am picking back the work on this one, If you want to sync on it, I am down @mastizada

@mastizada
Copy link

@el-youness sure, happy to work on this. I made changes to support async functions for almost all functions, however, no update in the PR review for some time now :(

@el-youness
Copy link

hmmm, I will base myself on your PR, I think he said to DRY the iter method, I'll give it a go

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.

6 participants