http: retry request on 429

AEnterprise commented 4 years ago

Currently when a request to the discord api gets rate limited (429 response) twilight does not try it again but just returns the error. While twilight does it's best to avoid all 429's by following the headers, some are simply unavoidable no matter how hard twiilight tries, some examples:

Channel edits have sub limits of only allowing to change the channel name and description ever 10 minutes, this is seperate from the regular rate limits on editing a channel and is not reflected in the headers at all (this is in a few other places as well)
Emoji limits like creating an emoji are guild wide and not per user
another bot/user using a webhook you made and using part of it's rate limits unknown to the bot
channel message limits shared across webhooks

There are probably some more cases but these are probably the main ones that can cause an unavoidable rate limit because the bot did not know about it.

I'd like to add a retry mechanic to twilight where a upon a 429 the request is tried again (up to x times), and if a request is delayed for a long time due to rate limits, log it as a info message

zeylahellyer commented 4 years ago

Note that this will cause every request to have to be initially constructed from cloned parts (Request itself doesn't even implement Clone) prior to initial sending. I'd personally rather have the user manually construct a retry request using the returned ratelimit information if needed.

AEnterprise commented 4 years ago

doing it yourself on the bot side is a huge mess as you will have to wrap every single twilight request you make with logic for this, there is no generic way to do this, you just have to wrap around every single method

7596ff commented 4 years ago

I don't like the possibility of one request function call possibly sending more than one http request. And as Vivian said, the data is there for someone to implement this on bot side.

zeylahellyer commented 4 years ago

After thinking about it more I've had a change of heart. An opt-in client builder configuration to take the performance penalty in exchange for saving yourself work and to modify client behavior (where one request can potentially unexpectedly equate to sending more than one request, as noted by Cassie) would be acceptable to me.

zeylahellyer commented 3 years ago

Just a note that whoever picks this up (I have no plans on working on it) will have a little bit of work ahead of them due to the fact that requests and multipart forms aren't cloneable.

AEnterprise commented 3 years ago

i do plan on picking this up, i just have not gotten around to it due to other things being more urgent

vilgotf commented 2 years ago

Note that tower::retry supports this. Adding tower to twilight-http requires GAT with the current API AFAIK

vilgotf commented 2 years ago

Just a note that whoever picks this up (I have no plans on working on it) will have a little bit of work ahead of them due to the fact that requests and multipart forms aren't cloneable.

This should always work as Body is never a stream https://docs.rs/reqwest/latest/reqwest/struct.Request.html#method.try_clone

vilgotf commented 1 year ago

Maybe this can be left to users? This library seems very pleasant to use https://github.com/Xuanwo/backon

AEnterprise commented 1 year ago

Leaving this to the user is a terrible option imo. This requires wrapping every single rest call twilight exposes separately (because in reality you'll want this on most if not all your rest calls due to shared rate limits)

laralove143 commented 1 year ago

i want to bump this, i can also work on it if you tell me the api wanted

itohatweb commented 1 day ago

Random thought, maybe we could store the request body in sth like Bytes which would allow for cheap cloning of the biggest part of the request :thinking:

twilight-rs / twilight

http: retry request on 429 #600