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

Improve ratelimit bucket handling #1103

Merged
merged 33 commits into from
Jan 14, 2020
Merged

Conversation

MinnDevelopment
Copy link
Member

@MinnDevelopment MinnDevelopment commented Sep 24, 2019

Pull Request Etiquette

Changes

  • Internal code
  • Library interface (affecting end-user code)
  • Documentation
  • Other: _____

Closes Issue: NaN

Description

Rewrite for the rate limiter to correctly handle buckets.

The method DOES matter for this according to all my tests
and edit seems to use the same bucket as send
@MinnDevelopment

This comment has been minimized.

@MinnDevelopment MinnDevelopment added this to the v4.1.1 milestone Dec 5, 2019
@MinnDevelopment MinnDevelopment removed the status: freezer this is currently put on hold label Jan 1, 2020
@MinnDevelopment

This comment has been minimized.

@MinnDevelopment
Copy link
Member Author

MinnDevelopment commented Jan 2, 2020

The new rate limit system includes both the method and bucket hash for queues:

A bucket is determined via the Path+Method+Major in the following way:

  1. Get Hash from Path+Method (we call this route)
  2. Get bucket from Hash+Major (we call this bucketid)

If no hash is known we default to the unlimited+method/path hash. The hash is loaded from HTTP responses using the X-RateLimit-Bucket response header. This hash is per Method+Path and can be stored indefinitely once received. Some endpoints don't return a hash, this means that the endpoint is unlimited and will be in queue with only the major parameter.

To explain this further, lets look at the example of message history. The endpoint to fetch message history is GET/channels/{channel.id}/messages. This endpoint does not have any rate limit (unlimited) and will thus use the hash unlimited. The bucket id for this will be unlimited+GET/channels/{channel_id}/messages:guild_id:{channel.id}:webhook_id where {channel.id} would be replaced with the respective id. This means you can fetch history concurrently for multiple channels but it will be in sequence for the same channel.

If the endpoint is not unlimited we will receive a hash on the first response. Once this happens every unlimited bucket will start moving its queue to the correct bucket. This is done during the queue work iteration so many requests to one endpoint would be moved correctly. For example, the first message sending:

public void onReady(ReadyEvent event) {
  TextChannel channel = event.getJDA().getTextChannelById("123");
  for (int i = 1; i <= 100; i++) {
    channel.sendMessage("Message: " + i).queue();
  }
}

This will send 100 messages on startup. At this point we don't yet know the hash for this route so we put them all in "unlimited+POST/channels/{channel_id}/messages:guild_id:123:webhook_id". The bucket iterates the requests in sync and gets the first response. This response provides the hash for this route and we create a bucket for it. Once the response is handled we continue with the next request in the unlimited bucket and notice the new bucket. We then move all related requests to this bucket.

I have done some exhaustive testing on this and it seems to work properly now. We won't know how well it works until putting it into a real bot.

Edit: The unlimited hash now includes the route to restore old per-route concurrency.

@MinnDevelopment
Copy link
Member Author

We should wait on a response to discord/discord-api-docs#1295

@MinnDevelopment MinnDevelopment changed the title Fix ratelimit buckets Improve ratelimit bucket handling Jan 14, 2020
@MinnDevelopment MinnDevelopment merged commit ea4fc81 into development Jan 14, 2020
@MinnDevelopment MinnDevelopment deleted the patch-ratelimits branch January 14, 2020 09:25
Andre601 pushed a commit to Andre601/JDA that referenced this pull request Apr 21, 2020
Andre601 added a commit to Andre601/JDA that referenced this pull request Apr 21, 2020
Andre601 added a commit to Andre601/JDA that referenced this pull request Apr 21, 2020
Andre601 added a commit to Andre601/JDA that referenced this pull request May 21, 2020
* Fix description for RestAction#flatMap

* Remove invalid NSFW info

* Fix bug in buffer allocation for ZlibDecompressor

* Don't insert null into override cache

* Keep a strong reference to user from members

Fake members will use fake users that are not cached

* Add functionality to set/get state of embed suppression (discord-jda#1117)

* Add functionality to set/get state of embed suppression

* Provide Enum for MessageFlags

* Improve handling of events for unavailable guilds (discord-jda#1184)

* Improve handling of events for unavailable guilds
* Add UnavailableGuildLeaveEvent
* Update docs for Guild#isAvailable

* Improve ratelimit bucket handling (discord-jda#1103)

* Let ChannelUpdateHandler add overrides to cache

This is a bug since the override shouldn't be in the interface
if the member is not actually in the cache. With the current
implementation we have a cache inconsistency that could NPE.

* Change handling of GuildDeleteHandler to avoid deadlock

There really is no point in holding a write-lock here

* Bump version

* Add South Korea Region

* Add getPermissionHolder method to PermissionOverride (discord-jda#1183)

* Make ignored events for unavailable guilds a debug log

* Add ErrorHandler utility class (discord-jda#1200)

* Add ErrorHandler utility class
* Improve logging of 429 warnings

* Add clearReactions(String) (discord-jda#1192)

* Add clearReactions(String)
* Add MessageReactionRemoveEmoteEvent
* Fix some outdated documentation
* Log and rethrow errors in BotRateLimiter workers

* Properly encode reaction emoji for clearReactionsById(...)

* Updated all copyright headers for 2020 (discord-jda#1198)

* Add (Guild) Invite Events (discord-jda#1196)

Adds GuildInviteCreateEvent and GuildInviteDeleteEvent

* Allow queued requests to finish

* Support gateway intents and configurable member caching (discord-jda#1190)

* First pass on member cache policy
* Add INVALID_INTENTS to close code enum
* Deprecate AccountType.CLIENT
* Add Guild#unloadMembers
* Update README with better examples
* Improve handling of channel overrides
* Add documentation for intents and member cache policy
* Remove references to guild subscriptions
* Add GuildMemberRemoveEvent
* Improve retrieveMemberById and retrieveUserById
* Changes to the builders, we use factory methods now :)
* Disable presence cache by default
* Add JDA#getGatewayIntents

* Fix NPE for GUILD_UPDATE handling

* Fix GuildChannel#createCopy for uncached members (discord-jda#1229)

* Deprecate GuildUpdateBannerEvent#getNewBannerIdUrl (discord-jda#1217)

* Allow to open private channel with user id (discord-jda#1224)

* Change defaults for requester to be non-daemon and have a shorter timeout (discord-jda#1215)

* Added Server Insights permission (discord-jda#1221)

* Add RestAction#timeout and RestAction#deadline (discord-jda#1220)

* Fix NPE in PermissionOverrideAction constructor

* Fix another NPE in success handler

* Documentation updates (discord-jda#1222)

* Swapped manage server and manage channel in Invite#isExpanded docs (discord-jda#1210)
* Document another possible null return case of Message#getMember (discord-jda#1204)
* Fix EmbedBuilder constructor javadocs (discord-jda#1214)
* Update example for jda-nas
* Add more details to modifyMemberRoles docs
* Fix typo

* Added getGuildChannelById methods to ShardManager (discord-jda#1234)

* Add better configuration methods to the builders (discord-jda#1240)

* Update README
* Add enableCache and disableCache
* Add enableIntents and disableIntents
* Update docs for events to mention required intents when necessary.
* Deprecate the old setters for cache flags

* Include voice server endpoint in connection failure logs

* Initialize SelfUser for all shards before logging them in

* Fix bug with new channel overrides having incorrect permissions

* Add the close code to the disconnect warn

* Adding a check in reaction methods before calling getTextChannel() (discord-jda#1216)

* Add JDA#cancelRequests (discord-jda#1223)

* Fix boost time not being handled in guild create

* Add GuildVoiceStreamEvent

* Fix IllegalStateException in Requester

Closes discord-jda#1254

* Change reconnect code to 4900 to avoid confusion (discord-jda#1250)

* Change reconnect code to 4900 to avoid confusion
* And update the disconnect handling to better log things
* Make resume reconnect log on debug
* Log server error on error level
* Add catch in gateway worker

* Add Guild#getMaxFileSize and Guild.BoostTier#getMaxFileSize (discord-jda#1244)

Fixes discord-jda#1243

* Add fallback operators onErrorMap and onErrorFlatMap to RestAction (discord-jda#1219)

* Fix incorrect file size check

* Documentation updates (discord-jda#1241)

* Add note about presence update and member intents
* Fix typo in readme
* Add MessageChannel#retrieveReactionUsersById and update reaction related docs (discord-jda#1236)
* Add requirements section to member/user events

* Json Parsing Optimizations (discord-jda#1267)

* Remove initial reconnect delay for resumes

* Add prune overload to disable wait parameter

* Add Guild#retrieveMetaData (discord-jda#1171)

* Lazy load member limits for guilds
* Improve implementation of update handler
* Add Guild#retrieveMetaData

* Add check for heartbeats in WebSocketClient (discord-jda#1282)

* Add retrieveMembersByPrefix (discord-jda#1276)

* Add retrieveMemberByPrefix
* Add event pool configuration
* Fix javadoc errors
* Rename chunkSyncQueue -> chunkQueue

* Create file if it doesn't exist in downloadToFile

* Add members to cache from retrieveMember methods

* Improve handling of join times for members

* Add Message.JUMP_URL_PATTERN

* Replace disconnect handling to recover from strange thread states (discord-jda#1295)

* Shutdown the WebSocket connection after everything else

* Use executor to shutdown in DefaultShardManager

* Add methods to bulk retrieve members (discord-jda#1292)

* Add methods to bulk retrieve members
* Add documentation
* Add missing annotations to Task interface
* Use random number as nonce

* Improvements to handling of Error (discord-jda#1285)

* Remove usage of AssertionError
* Improve handling of Errors

* Change domain to discord.com (discord-jda#1288)

* Fix NPE in ChannelManager#sync

* Add User#getFlags (discord-jda#1271)

* Add Emote#isAvailable() method

Co-authored-by: Florian Spieß <business@minnced.club>
Co-authored-by: Michael Ritter <ritter.michael92@gmail.com>
Co-authored-by: horyu1234 <horyu1234@naver.com>
Co-authored-by: DMan <DManstrator@users.noreply.github.com>
Co-authored-by: Austin Keener <keeneraustin@yahoo.com>
Co-authored-by: averen <31487840+averen@users.noreply.github.com>
Co-authored-by: Mathéo CIMBARO <25774021+DiscowZombie@users.noreply.github.com>
Co-authored-by: Artuto <artutogamer@gmail.com>
Co-authored-by: Nik <nik@monsterlyrics.co>
Co-authored-by: Sebo Molnár <git@mlnr.dev>
Co-authored-by: gpluscb <20143778+gpluscb@users.noreply.github.com>
Co-authored-by: Napster <napster@npstr.space>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant