fix: Different ratelimits for the same route (implement discord buckets) #1546
Conversation
Sometimes Discord won't send any ratelimit headers, disabling the semaphore for endpoints that should have them.
|
Is this ready to merge? |
In my tests it was working fine, but more would be better, specially by people that were having this issue (like who created the issue). |
|
Did a few more tests and notice But it still seems something is weird atm, the buckets are being disabled instead of updated with the correct ratelimit. From what I saw, it's working because it's solely relying on the headers that for some reason seem to not being passed correctly. I'll need some more time to see what's going on. |
|
Ok, I believe I understood the real issue going on, just need some time to find where. |
SubZero0
changed the title
|
Think I got the real reason that was happening and why I reached the wrong conclusion the first time. The issue was that methods with different http methods but the same endpoint were sharing the same bucket for ratelimits, so while one wouldn't have specific limits, the other would. Example: GET Message and PATCH Message. So getting a message would disable the bucket, then, when trying to PATCH, it wouldn't be able to update the limits, making possible to reach the actual ratelimit. I still need to check and test if different http methods might share the same bucket, like PATCH/POST. |
SubZero0
changed the title
|
Added Now I believe there's two tests needed to be done:
Reference |
SubZero0
changed the title
- BucketId is it's own class now - Add WebhookId as a major parameter - Add shared buckets using the hash and major parameters
|
The last commit was to change the BucketId to keep track of the major parameters of the route to easily create a shared bucket between different endpoints since the previous one only used the endpoint. It's all related to the 2nd point I made on my previous comment. I still need to check the 1st and do the necessary changes (+ add some missing documentation). |
|
Added a way to redirect requests to another bucket when it's discovered it should be a hash one. This will deal with the 1st point I made on my previous comment. Now I just need to do some tests to see how it's behaving and check for possible issues. |
|
I think I reached a stable ground. I'll open it for review, but probably would be better testing it on a real bot, since test ones can only achieve so much trying to "spam" the API with random requests. |
SubZero0
changed the title
|
@SubZero0 I can try to test it on one of my nodes. Any good ways on how i could install it? :P |
@BramEsendam You will need to clone the repo and compile this branch (or reference it from your project). I have been running this for almost 3 days on my bot (2 shards, 1.6k servers) and so far so good. Didn't reach any true ratelimit or got deadlocked afaik (at least there was no report of the bot not responding, so can't say 100% for sure). |
|
I sadly cant say either because I only ever reached the role "ratelimit" once on 1 of the myget preview packages (And I do call a lot onto roles for patreon related stuff based on tiers of people returned from their API as well in an webhook on ask.net core 3.1). Although I do check if the users already got the roles (and list them out for AddRoles) and if they do have them already I have it bail out to try to prevent ratelimit (why request if it wont do any changes am I right?). And it basically has 1 command to basically re-cache everything it caches. |
|
I have actually manged to ratelimit a lot more because of patreon themselves seeming to request my locally hosted api all at once which then causes a race condition which then causes ratelimits... |
@AraHaan Not sure if I follow what you are saying, but I believe you aren't running Discord.Net with this patch since I changed the way how it'll display the bucket. Your message is
|
|
Ye, I am using the latest myget package version of d.net on it currently. Also it seems after a while since that ratelimit triggers that it also seems to look offline all the time after that. Alright so I have decided to clone this to make slight edits to where the reconnection logic is to actually make logs from within it on every line (would contain line numbers) to try to find where in the code that me and others actually experience the deadlock at. |
|
good news it looks like this branch works for now (have not tested it long enough to get my issue yet though) I also posted 1 idea though in an open issue for ConnectionManager.cs that might be an issue however not clear yet if it really is though. It might be an issue from within ApiClient or the event that is registered for that class inside of DiscordSocketClient or in the event itself somewhere. I would have to do more digging and more logging to find out what is really going on. |
|
@AraHaan I believe your post should be in that issue, because this PR has nothing to do with the connection deadlock (or shouldn't have). It's about fixing the ratelimiter that doesn't work properly by not accounting shared buckets between endpoints. I would also recommend uploading those logs as a file, as you can see, it's huge and makes reading other comments really hard. |
|
Alright I think that this is stable enough for me on my bot. Not sure about others but I think this is safe to merge. |
|
Is this still getting added. :? |
|
I think I found what can cause deadlocks in the code and narrowed it down to the usage of SemaphoreSlims that also causes my code to deadlock as well when I try to have my code also wait until the invoke is completed on an async timer which is related in the actual semaphore on the Wait or WaitAsync calls in that class waiting indefinitely even when there are nothing executing on the semaphore. I have currently no idea what to do to fix the semaphore issue both in my code and in discord.net. It just makes me feel like the usage of SemaphoreSlims could be a major contributing factor to the connection deadlocks. |
I have no idea what you are talking about since this PR and the classes it modifies don't use SemaphoreSlim. |
|
ok so @SubZero0 I have fixed my issues on my end in combination to what was at fault on the lib's side of things in #1580. After fixing those I have realized that this is stable enough and can be merged without issues. My bot might only have 1 command but it is a large bot too because of it being ran in an ASP.NET Core webapi process and as such having an web interface and a lot of other integrations like calls to the patreon api to verify patrons and stuff as well as oauth providers to login to it's web interface using discord and patreon. It was a whole mother to set up and get working properly. However with these fixes to the ratelimits and my @BitPatty mind testing this pr? |
|
Will test it on my issue and report back asap |
|
Tested, Fixes #1535 Thanks 👍 |
|
@SubZero0 ok the issue creator has tested that this fixes the issue. |
|
Any ETA? |
…ts) (discord-net#1546) * Don't disable when there's no resetTick Sometimes Discord won't send any ratelimit headers, disabling the semaphore for endpoints that should have them. * Undo changes and change comment * Add HttpMethod to BucketIds * Add X-RateLimit-Bucket * BucketId changes - BucketId is it's own class now - Add WebhookId as a major parameter - Add shared buckets using the hash and major parameters * Add webhookId to BucketIds * Update BucketId and redirect requests * General bugfixes * Assign semaphore and follow the same standard as Reset for ResetAfter
Summary
The same route can have different ratelimits depending on the http method (post, patch, get, ...) and different endpoints can share the same limit on their route.
An example would be
GetMessageAsyncandIUserMessage.ModifyAsyncthat areGET channels/{channel.id}/messagesandPATCH channels/{channel.id}/messagesthat would previously have the same bucket,channels/{channel.id}/messages, but GET has no specific limit, while PATCH does.The issue happening was:
An easy way to repro it without this fix:
Fixes #1535
Changes
X-RateLimit-BuckettoRateLimitInfoWebhookIdas a major parameter intoBucketIdsBucketIdas it's own classstringto theBucketIdclassReferences
discord/discord-api-docs#981
discord/discord-api-docs#1551
https://discord.com/developers/docs/topics/rate-limits#header-format