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

Increase delay less aggressively on rate limit #419

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ghost
Copy link

@ghost ghost commented Oct 8, 2022

Immediately jumping to 6000-6500ms delays just because that's how long the API makes you wait after a bunch of quicker deletions is unnecessarily aggressive.

Immediately jumping to 6000-6500ms delays just because that's how long
the API makes you wait after a bunch of quicker deletions is
unnecessarily aggressive.
@abigailwillow
Copy link
Contributor

Am I correct in saying that this just increases the delay by a hardcoded 50 milliseconds and completely ignores the rate limit amount coming from Discord's API?

@ghost
Copy link
Author

ghost commented Oct 8, 2022

Yes, but it will eventually keep adding on further 50ms delays if you still keep getting hit with rate limits, then it will stop around 2000-2500ms in my experience, which is definitely much faster than the 6000-6500ms it kicks you to normally (which really only happens because the initial delay was way too short).

Do note that it still waits N * 2 ms one time when it gets told to wait N ms by the API, so it's not totally ignored.

@abigailwillow
Copy link
Contributor

abigailwillow commented Oct 8, 2022

The API definitely rate limits you longer than necessary, but I wonder if we can for example add a fraction of the APIs indicated delay to the current delay instead of relying on getting rate limited many times in a row, which could necessitate a longer delay and might increase the likelihood of users getting banned for automating user accounts.

@ghost
Copy link
Author

ghost commented Oct 8, 2022

Yeah, or maybe make the one-off cool-off delay longer, I don't know.

@KevinBatdorf
Copy link

The rate limit wait response should only determine cool down time and not be used to adjust the deleteDelay. incrementing by 50 until it reaches the "sweet spot" is the right approach (a further enhancement could be to decrease by 50 over time as well). This PR 👍

The current implementation will even lower the delete delay in some cases. For example:

Screen Shot 2022-12-14 at 11 27 44 AM

@victornpb
Copy link
Owner

The rate limit wait response should only determine cool down time and not be used to adjust the deleteDelay. incrementing by 50 until it reaches the "sweet spot" is the right approach (a further enhancement could be to decrease by 50 over time as well). This PR 👍

Yes you are correct, this used to work because the cooldowns used to be way less aggressive. This approach of adding 50ms seems fine, but I fear if you're to far from what discord considers ok and you get multiple cooldowns in sequence, that could result in a more severe penalty.

Has anyone confirmed if this approach is working?

@thewriteway
Copy link

I'm not sure this is the full solution as the main issue is the API rate limit is too easy to trigger on mass operations because the initial search and delete delays are not sufficiently long.

Perhaps this in combination with upping the default search delay to at least 500 and the default delete delay to at least 1500.

I've never had it run successfully with lower timed defaults.

@abigailwillow
Copy link
Contributor

abigailwillow commented Feb 22, 2023

Perhaps this in combination with upping the default search delay to at least 500 and the default delete delay to at least 1500.

I've never had it run successfully with lower timed defaults.

In some limited testing search delay really didn't seem to matter all that much. Delete delay seems much more impactful and I've found that 2000 ms is really the safer margin. Often enough 1500 ms won't even get you through that many messages before rate limiting. There's currently an open PR (#402) to address this.

If you indeed find yourself getting rate limited more often with the default search delay of 100 ms please post your findings!

@victornpb victornpb added the PR outdated / needs rebase PR needs to be updated because it is behind master label Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR needs further testing PR outdated / needs rebase PR needs to be updated because it is behind master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants