-
Notifications
You must be signed in to change notification settings - Fork 790
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 rate limit buffering support #430
Conversation
- Break request into requestInner - Break LockBucket into LockBucketObject - Change getBucket to GetBucket so it can be externally accessed - Add RequestBuffer interface - Add RequestBuffer pointer to Bucket struct - Add RequestPostWithBuffer() function to Session
voice.go
Outdated
@@ -659,8 +658,6 @@ func (v *VoiceConnection) opusSender(udpConn *net.UDPConn, close <-chan struct{} | |||
return | |||
} | |||
|
|||
runtime.LockOSThread() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you PR this separately please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this change was directly merged into the master branch, but never pulled into the develop branch, and it was accidentally swept up by my pull request. You should properly merge this into the develop branch, and then I can re-do my pull request and it won't show up anymore.
I've updated this and decided to go for the more general route of exposing the rate limiter itself rather than force bots into using a specific buffering method. There is no longer any new functionality - a few functions have been split up and a few variables have been exposed. The Bucket object now exposes a generic |
I'm not sure that exposing the rate limit is the best idea, |
That doesn't solve the problem. I only want to aggregate messages if the bot is about to be rate limited. I have to have access to the rate limit to be able to detect this. There is simply no possible way to only aggregate messages in response to a rate limit without having access to the rate limit. If this change is not acceptable I will simply retract the pull request. |
OK i think i got it so is your problem that you edit your message to aggregate the new one on each user join and so when your bot it the limit he need to wait but during this time new user can arrive causing your bot to quickly hit the limit again ? |
No, I want to specifically avoid that by detecting that the bot will be rate limited after the next message it sends, so that it sends 4 messages and then aggregates the rest of the messages into one giant 5th message that it sends before the rate limit is hit. Here is an example of how it behaves right now: I strongly oppose simply not allowing bots to access their rate limit status. This is just throwing information away that bots can use to effectively respond to being rate limited without resorting to simply sending a message everyone 1 second and impacting their responsiveness. There is no excuse for throwing away the rate limits that the discord API specifically says you should pay attention to. Anyone hardcoding a 1 second aggregation bucket is implicitely hardcoding assumptions about the rate limit into their code, which is exactly what the discord API says you shouldn't do. Discordgo must expose it's rate limiting information. If someone would rather expose it in a different way, that's fine, but somehow, that information must be exposed in order for people to write well-behaved discord bots. Simply not exposing the rate limiting information is not an option. |
Okay i understand i will just leave you a piece of code which hasn't got delay but however on Ratelimit block will send the big packet 😄 this piece of code just permit you to manage if too many messages arrive at the same time but it doesn't do it just before the ratelimit so yes I think you PR in some cases can be usefull ;)
|
Aggregating messages AFTER the rate limit is not acceptable behavior. I also have to point out that your code fundamentally doesn't work in a multi-channel context, because the rate limits between channels are different. This is why the buckets must be exposed and why there is now The rate limits MUST be exposed to support this behavior. |
I've hit rate limits like this myself and having the rate limits exposed would be a great thing. I'm 👎 on the |
I have a bot that often updates its status, and I have issues with ratelimit as well. So this change would be greatly appriciated. |
it's been over a month, what's the status on this PR? |
@sciencefyll This PR appears to be completely dead. The maintainers do not want to expose the rate limit. If you want this feature, you will have to use the develop branch of my fork. I attempt to keep the develop branch in sync, but the master branch is probably out of date. Sorry. |
@jonas747 care to take a look? |
looks fine to me |
If this pull request isn't accepted soon I'm going to close it instead of repeatedly merging changes into it over and over. |
You use you own fork or? I guess I have to switch from the official to yours, so do you maintain your fork? |
I use my own fork. Every now and then I sync it with develop. I will not guarantee any support whatsoever for my fork because I shouldn't need it in the first place, so you can use it at your own risk. |
Currently my only hang up that is left here is the Userdata interface{} field. I'd love to see some discussion about why it is required and any possible alternatives that would also solve it's requirement. |
The Userdata field is mandatory because any bot that wants to do anything useful with rate limits must store information on a per-bucket basis, because any sort of buffering or waiting has to be done on the same bucket. My bot uses the Userdata field to implement a message buffer that merges text messages together. The requirement here is that bots must be able to assign arbitrary user data to buckets. A hash cannot work because there's no way to tell when or if a bucket will be discarded, so the hash grows without bound. A "Bucket Creation Hook" might work if you allow the bot to create it's own bucket object that embeds the internal Bucket object and then casts back to it. Neither of these seem like compelling alternatives to the much simpler method of just having a |
* Fix bwmarrin#406: reconnect() can be called while still connected * Add memberMap to speed up member queries * Fix error return value and remove deletion * Fix GuildAdd member map initialization edge case * Add rate limit buffering support - Break request into requestInner - Break LockBucket into LockBucketObject - Change getBucket to GetBucket so it can be externally accessed - Add RequestBuffer interface - Add RequestBuffer pointer to Bucket struct - Add RequestPostWithBuffer() function to Session * Remove internal implementation, export ratelimiter instead
My anti-spam bot needs to be able to detect when it is about to get rate limited, and then in response to this start aggregating small messages together into a single larger message. This is intended to deal with bot raids, which can trigger 300+ "user has joined" notifications at the same time.
Doing this in a thread-safe manner requires several modifications to discordgo, and the core "buffered message" function must also be implemented in the
Session
object because it must access theratelimiter
object. While this pull request tries to make the absolute minimum amount of changes necessary to support this feature, it could be shrunk even more by exposing the rate limiter object and internalrequestInner
function outside of discordgo, which would then allow the bot to implement the buffered Post function.However, if that involves exposing too much of the internal library, the following is the minimum number of changes required to make this work:
If you believe RequestPostWithBuffer should, in fact, be added to Session, tests will need to be made to ensure it works properly, which are not currently included - suggestions for how to implement those tests or where similar tests can be found are appreciated.