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

Too many requests - 429 => special RetryOptionsBase which wait for the time in Retry-After-Header #59

Closed
wreggyl opened this issue Feb 4, 2022 · 12 comments

Comments

@wreggyl
Copy link

wreggyl commented Feb 4, 2022

Hi,

one of our current server limit the API with 429 and send the waiting time in the retry-after header (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Retry-After)

WIth python requests module and the serial work with retry strategy it was easy to wait exactly how long the server tells us in the header and retry again.

With async I have no idea how to implement the same strategy, because the waiting time I get as a response is only applicable for this request.

Do you have any idea how to maximize the parallel calls without waiting too long?

Thx in advance

@haomi123
Copy link

I have the same problem. Have you solved it now?

@wreggyl
Copy link
Author

wreggyl commented May 17, 2022

Hi, not really. The main problem was that the server side application (not our under control) used the 429, but give us the wrong waiting time in the retry-after-header. Not sure how this should work, but the server had a strong rate limit of 5 parallel requestes and if we send 60 in parallel we got 5 successful and for 55 we got a 429 with a retry after header of 1 seconds. So also in the next retry we still get only 5 valids and the other 50 goes in the next retry -> the server does not tell us remaining calls so that we can optimize the requests and the values provided in retry-after-header or not correct.

So we decided to go with a ListRetry.

Furthermore for this issue with issue 61 we subclassed RetryClient and _RequestContext. In this we only changed that we made a DeepCopy of the Formdata. We didn't put this for now as a pull request, because in Aiohttp it was explizitly implemented that this cause an exception (aio-libs/aiohttp#4351)

So not sure what is the best way to solve it correctly - this one now works for us. Except that we didn't get the "parallel" calls fast enough and are stressing the server with more retries than needed due to missing values in the response.

Additionally we changed also the Retry behaviour on 500er codes. The server gives this too often and normally a retry does not fix anything -> so 500 is more a general exception in our case and should not be retry. Except if there are network issues with a EOF -> in this case we want to retry it.

Another topic on which we currently fight: Our server is using OpenID Connect and this means that after a specific time the access_token expires and needs to be refreshed. So normally we need to check the validity of the token before a request is send.

Hope this helps a bit.

@inyutin
Copy link
Owner

inyutin commented Aug 4, 2022

Now you can use response as a parameter to your RetryOptions. Please, look at this commit
013cfbe

So you probably can create a function like that:

def get_timeout(self, attempt: int, response: Optional[ClientResponse] = None) -> float:
    if response is None:
        return 5.0
     return response.header['Retry-After']        

This should fix your problem

@inyutin
Copy link
Owner

inyutin commented Aug 4, 2022

@wreggyl @haomi123

@inyutin inyutin closed this as completed Aug 4, 2022
@Rongronggg9
Copy link

Rongronggg9 commented Aug 5, 2022

Hi there, @inyutin. I consider you should at least bump the minor version (a.B.c) instead of the patch version (a.b.C) since it is a breaking API change for those users who inherit their own RetryOption. What's more, the release note did not document it as a breaking change... Just a reminder, never mind.

@inyutin
Copy link
Owner

inyutin commented Aug 5, 2022

@Rongronggg9 thank you!
I missed that this is a breaking change. Sorry for distraction. I will now bump minor and add documentation
Thank you very much

@Rongronggg9
Copy link

The new release is fine. Should v2.5.6 be yanked from PyPI? It may help prevent loose versions from selecting it.

@inyutin
Copy link
Owner

inyutin commented Aug 7, 2022

@Rongronggg9 I would like not to do that. In my experience if people want to update a dependency - they update it to the last possible version

@Rongronggg9
Copy link

Rongronggg9 commented Aug 7, 2022

In my experience if people want to update a dependency - they update it to the last possible version

Nope. You might misunderstand me. I mean, if someone does NOT frequently upgrade their dependencies, and a loose version has been used for a long time (e.g. ~=2.5.0, ==2.5.*, >=2.5.0, <2.6.0), 2.5.6 will be chosen for any new installation. Usually, the problem is invisible to existing environments (they have been set up before 2.5.6 was released), but once someone needs to set up a new one, 2.5.6 will be chosen and installed, potentially causing problems. In such a case, downstream developers will probably be unaware of the problem until someone opens an issue. Then they are forced to either upgrade their dependencies or use a strict version instead.

If someone uses a loose version for their dependencies, they, less or more, show their belief that upstream developers won't introduce breaking change into patch versions. What's more, even if a downstream developer upgrades their dependencies, the old versions of their packages are, obviously, unaffected. That's another big problem.

@inyutin
Copy link
Owner

inyutin commented Aug 7, 2022

@Rongronggg9 you are right, this problem may exists. However I'm still not willing to delete the version. I'm afraid someone may already have adopted 2.5.6 and by deleting I will do things much worse. Thank you for pointing our to such an important problem. I hope I will not make a same mistake in the future. But I'm really not sure that deleting the version now is a good choice.

@Rongronggg9
Copy link

Rongronggg9 commented Aug 8, 2022

However I'm still not willing to delete the version. I'm afraid someone may already have adopted 2.5.6 and by deleting I will do things much worse.

Yanking a version equals NOT deleting a version. Actually, PyPI prompts you not to delete a version but to yank it when you try deleting a version. By yanking a version, pip will choose it only if a strict version (e.g. ==2.5.6, ===2.5.6) is specified. That's exactly what the case you are concerned about. Thus, you can safely yank it without affecting those packages adopting aiohttp-retry==2.5.6.

On the other hand, you may unyank a yanked version according to PEP 592, so, yeah, if you are still concerned about some cases, you may just give try. :)

Ref:
PEP 592
What's a "yanked" release?

@inyutin
Copy link
Owner

inyutin commented Aug 8, 2022

@Rongronggg9 done, thank you

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

No branches or pull requests

4 participants