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

feature: Implement gateway ratelimit #1537

Merged
merged 31 commits into from
Nov 19, 2020

Conversation

SubZero0
Copy link
Member

@SubZero0 SubZero0 commented May 18, 2020

Summary

This PR aims to add a ratelimiter to the gateway that currently has none except a Task.Delay(5000).
It follows what's described in the discord api docs.

Fixes #1442

Changes

  • Add a new type of bucket, GatewayBucket
  • Add IdentifyMaxConcurrency property to DiscordSocketConfig
  • Add GetBotGatewayAsync to IDiscordClient
  • Add session_start_limit to GetBotGatewayResponse
  • Sharded client will automatically deal with concurrent identify requests when possible

@SubZero0 SubZero0 marked this pull request as ready for review May 18, 2020 08:51
src/Discord.Net.Rest/Entities/Gateway/GatewayLimit.cs Outdated Show resolved Hide resolved
src/Discord.Net.Rest/Entities/Gateway/GatewayLimit.cs Outdated Show resolved Hide resolved
src/Discord.Net.Rest/Entities/Gateway/GatewayLimits.cs Outdated Show resolved Hide resolved
src/Discord.Net.WebSocket/DiscordSocketConfig.cs Outdated Show resolved Hide resolved
src/Discord.Net.Rest/Entities/Gateway/GatewayLimits.cs Outdated Show resolved Hide resolved
@SubZero0
Copy link
Member Author

I'll need to do a few more changes to this PR

The only account limit is for identify that is dealt in a different way (exclusive semaphore), so websocket queues can be shared with REST and don't need to be shared between clients anymore.

Also added the ratelimit for presence updates.
@SubZero0
Copy link
Member Author

SubZero0 commented May 25, 2020

After a bit more digging, I learned that the global queue is per-session, only Identify is per-account, and there's a "soft" presence update limit, so I changed my approach taking this into consideration.

  • Added IdentifySemaphoreName and PresenceUpdate to GatewayLimits
  • Gateway will use the same RequestQueue as REST per client, won't be sharing one anymore
  • Identify will use a named semaphore (the name is changeable at GatewayLimits), this will make possible to share the same semaphore between clients with or without a sharded client

I also noticed (if I understood it correctly) that if the user hits the gateway ratelimit, it won't be able to send heartbeats and might end losing the gateway connection either way. It would be possible to create a reserved limit for it but do we want to interfere this much? @foxbot

@SubZero0 SubZero0 requested a review from Still34 May 25, 2020 23:56
@SubZero0
Copy link
Member Author

SubZero0 commented Jun 1, 2020

While browsing the discord api docs github, I noticed this pull request: discord/discord-api-docs#1621

It seems discord provides a key that allows multiple shards to concurrently identify in Get Gateway Bot.

I'll see if there's a better way to implement it.

- Add session_start_limit to GetBotGatewayResponse
- Add GetBotGatewayAsync to IDiscordClient
- Add master/slave semaphores to enable concurrency
- Not store semaphore name as static
- Clone GatewayLimits when cloning the Config
- Remove GatewayLimits
- Transfer a few properties to DiscordSocketConfig
- Remove unnecessary usings
@SubZero0
Copy link
Member Author

SubZero0 commented Jun 5, 2020

After some talk and more consideration, I reached the conclusion that Discord wouldn't change specific limits for the gateway since what needs to be changed is sent by REST (max concurrency) or can be solved by adding more shards (gateway global limits).

Taking this into consideration, I removed GatewayLimits.
I left the semaphore-related properties in DiscordSocketConfig, I'm also considering removing these since the limited use-case would be automatic identify control by independent DiscordSocketClients (without a DiscordShardedClient as mediator).

@SubZero0
Copy link
Member Author

SubZero0 commented Jun 6, 2020

I decided to remove the named semaphore since it would add a lot more complexity. If someone is sharding for themselves, they have entire control when a client is going to identify.

Now only IdentifyMaxConcurrency is present at the config, since it's needed to create the "slave semaphores" for concurrent identify when automatic sharding is disabled.

@SubZero0 SubZero0 merged commit ec673e1 into discord-net:dev Nov 19, 2020
@SubZero0 SubZero0 deleted the gateway-ratelimit branch November 19, 2020 02:57
SubZero0 added a commit to SubZero0/Discord.Net that referenced this pull request Feb 11, 2021
* fix: limit request members batch size

Discord is actually enforcing v8 limits on v6 according to discord/discord-api-docs#2184

* fix: Missing AddReactions permission for DM channels. (discord-net#1244)

Co-authored-by: Samuel <33796679+Samrux@users.noreply.github.com>

* fix: GuildEmbed.ChannelId as nullable per API documentation (discord-net#1532)

* (ifcbrk) feature: Add includeRoleIds to PruneUsersAsync (discord-net#1581)

* Implemented include_roles for guilds/id/prune get&post

* Unnecessary using

Co-authored-by: Paulo <pnmanjos@hotmail.com>

* feature: Add "View Guild Insights" to GuildPermission (discord-net#1619)

* feature: Add "View Guild Insights" permission

* Add missing summary

Co-authored-by: Paulo <pnmanjos@hotmail.com>

* fix: Audio stream dispose (discord-net#1667)

* Fix audio dispose

* Missed a few

* feature: Add INVITE_CREATE and INVITE_DELETE events (discord-net#1491)

* Add invite events (create and delete)

* Removed unused using

* Fixing IInviteMetadata properties

* Add two new fields to the gateway event

* Better event summary and remarks

* Change how to assign to target variable

Co-Authored-By: Joe4evr <jii.geugten@gmail.com>

* Applying suggested changes to TargetUserType

* Renaming NotDefined to Undefined

* Fixing xml docs

* Changed the summary style format

Co-authored-by: Joe4evr <jii.geugten@gmail.com>

* feature: Add missing application properties (including Teams) (discord-net#1604)

* Add missing application properties

Add IsBotPublic, BotRequiresCodeGrant, and Team properties to IApplication

* To immutable list

* Change list to array

* feature: Add GetStreams to AudioClient (discord-net#1588)

* Added GetStreams

* Change return type

* Change return type on the interface

Co-authored-by: Paulo <pnmanjos@hotmail.com>

* fix: Crosspost throwing InvalidOperationException (discord-net#1671)

* Add INewsChannel

* Renaming variable to match the new type

* fix: Discord sends null when there's no team

* fix: Team is nullable, not optional (discord-net#1672)

* misc: Change ratelimit messages (discord-net#1678)

* misc: Missing summary tag

* fix: Emoji url encode (discord-net#1681)

* feature: Add missing properties to Guild and deprecate GuildEmbed (discord-net#1573)

* Add missing properties to Guild, related methods, and deprecate GuildEmbed endpoints

- Add missing guild properties: `discovery_splash`, `widget_enabled`, `widget_channel_id`, `rules_channel_id`, `max_presences`, `max_presences`, `max_members`, `public_updates_channel_id`, `max_video_channel_users`, `approximate_member_count`, `approximate_presence_count`
- Update guild properties: `embed_enabled`, `embed_channel_id`
- Add `GetGuildDiscoverySplashUrl` to `CDN`
- Add classes related to the guild widget
- Add `withCounts` parameter to `GetGuild(s)Async`
- Make GuildEmbed related methods obsolete with a message redirecting to widget ones

* Change xml docs and PremiumSubscriptionCount type

* Changed some xml docs

* fix: Revert PremiumSubscriptionCount type (discord-net#1686)

* feature: Implement gateway ratelimit (discord-net#1537)

* Implement gateway ratelimit

* Remove unused code

* Share WebSocketRequestQueue between clients

* Add global limit and a way to change gateway limits

* Refactoring variable to fit lib standards

* Update xml docs

* Update xml docs

* Move warning to remarks

* Remove specific RequestQueue for WebSocket and other changes

The only account limit is for identify that is dealt in a different way (exclusive semaphore), so websocket queues can be shared with REST and don't need to be shared between clients anymore.

Also added the ratelimit for presence updates.

* Add summary to IdentifySemaphoreName

* Fix spacing

* Add max_concurrency and other fixes

- Add session_start_limit to GetBotGatewayResponse
- Add GetBotGatewayAsync to IDiscordClient
- Add master/slave semaphores to enable concurrency
- Not store semaphore name as static
- Clone GatewayLimits when cloning the Config

* Add missing RequestQueue parameter and wrong nullable

* Add RequeueQueue paramater to Webhook

* Better xml documentation

* Remove GatewayLimits class and other changes

- Remove GatewayLimits
- Transfer a few properties to DiscordSocketConfig
- Remove unnecessary usings

* Remove unnecessary using and wording

* Remove more unnecessary usings

* Change named Semaphores to SemaphoreSlim

* Remove unused using

* Update branch

* Fix merge conflicts and update to new ratelimit

* Fixing merge, ignore limit for heartbeat, and dispose

* Missed one place and better xml docs.

* Wait identify before opening the connection

* Only request identify ticket when needed

* Move identify control to sharded client

* Better description for IdentifyMaxConcurrency

* Add lock to InvalidSession

* feature: Send presence on Identify payload (discord-net#1688)

* Send presence on identify

* Change CurrentUser presence

* feature: Add inline replies (discord-net#1659)

* Add inline replies

* Missed a few things

* Change xml docs, IUserMessage, and other changes

* Missed one when changing

* Fix referencedMessage author

* fix: Possible NullReferenceException when receiving InvalidSession (discord-net#1695)

* fix: UserMentions throwing NullRef

* fix: Wrong author for SocketUserMessage.ReferencedMessage

* fix: Rollback Activities to Game (discord-net#1702)

* docs: Update summary of SocketVoiceChannel.Users (discord-net#1714)

The inherited summary incorrectly stated that all users who _see_ the channel are returned when in reality only the ones _connected_ are.

* feature: Add max bitrate value to SocketGuild

* Update SocketGuild.cs

* Fix returns value docs

Signed-off-by: Still Hsu <dev@stillu.cc>

Co-authored-by: Still Hsu <dev@stillu.cc>

* fix: SocketGuild.HasAllMembers is false if a user left a guild (discord-net#1683)

* Fix: HasAllMembers is false if a user left

* Fix: Correct boolean logic

* Add new method of role ID copy

Signed-off-by: Still Hsu <5843208+Still34@users.noreply.github.com>

* Add missing asset

Signed-off-by: Still Hsu <5843208+Still34@users.noreply.github.com>

* Resolve inheritdocs warning

* Move bulk deletes remarks from <summary> to <remarks>

* Mark null as a specific langword in summary

* Cleanup GatewayReconnectException docs

* Update Docfx.Plugins.LastModified to v1.2.4

Signed-off-by: Still Hsu <5843208+Still34@users.noreply.github.com>

* Add updated libraries for LastModified

Signed-off-by: Still Hsu <5843208+Still34@users.noreply.github.com>

* Update project link

* Add initial StyleCops style enforcement

* Update framework version for tests to Core 3.1 to comply with LTS

* docs: Fix MaxWaitBetweenGuildAvailablesBeforeReady docs string

Signed-off-by: Still Hsu <dev@stillu.cc>

* docs: Add minor tweaks to DiscordSocketConfig docs strings

* docs: Fix the missing "In This Article" section

* Temporarily disable StyleCops until all the fixes are impl'd

* fix: Invite and InviteMetadata properties (discord-net#1639)

* fixes discord-net#1495

* keep obsolete properties and return types for compatibility

* missing properties for SocketInvite

* Restore xml docs and change obsolete message

Co-authored-by: Paulo <pnmanjos@hotmail.com>

* docs: Add alternative documentation link

* fix: IMessage.Embeds docs remarks

* fix: Move and fix internal AllowedMentions object (discord-net#1727)

* misc: VoiceRegions and related changes (discord-net#1720)

* feature: Add role tags (discord-net#1721)

* feature: Add user public flags (discord-net#1722)

* feature: Add MessageFlags and AllowedMentions to message modify (discord-net#1724)

* feature: Add MessageFlags and AllowedMentions to Modify

* Change exception message

* feature: Add GuildUser IsPending property (discord-net#1731)

* Implemented Pending property

* Implemented changes

* fix: Missing MessageReference when sending files

* meta: 2.3.0

* meta: Bump version to 2.3.1-dev

* fix: Deadlock in DiscordShardedClient when Ready is never received (discord-net#1761)

* fixed a deadlock in DiscordShardedClient during a failed Identify due to InvalidSession

* fixed log

* Don't wait ready before releasing semaphore

Co-authored-by: Paulo <pnmanjos@hotmail.com>

* fix: Private methods aren't added as commands (discord-net#1773)

Fix breaking change introduced by discord-net#1521

Co-authored-by: Samuel <33796679+orchidalloy@users.noreply.github.com>
Co-authored-by: Samuel <33796679+Samrux@users.noreply.github.com>
Co-authored-by: Matt Smith <me@mattthedev.codes>
Co-authored-by: Radka Gustavsson <radka.janek@redhat.com>
Co-authored-by: bakabaka.bunbun <54648150+bakabun@users.noreply.github.com>
Co-authored-by: Joe4evr <jii.geugten@gmail.com>
Co-authored-by: Jack Fox <0xdeadbeef1@gmail.com>
Co-authored-by: Joel Liechti <52202086+Joelius300@users.noreply.github.com>
Co-authored-by: vrachv <52053491+vrachv@users.noreply.github.com>
Co-authored-by: Still Hsu <dev@stillu.cc>
Co-authored-by: Daniel Baynton <49287178+230Daniel@users.noreply.github.com>
Co-authored-by: Still Hsu <5843208+Still34@users.noreply.github.com>
Co-authored-by: Fyers <Fyers@web.de>
Co-authored-by: quinchs <49576606+quinchs@users.noreply.github.com>
Co-authored-by: Yeba <31899118+yebafan@users.noreply.github.com>
Co-authored-by: Antonio Zdravkov Nikolov <antonioznikolov@gmail.com>
anonymousheadless pushed a commit to anonymousheadless/Discord.Net that referenced this pull request Mar 29, 2021
* Implement gateway ratelimit

* Remove unused code

* Share WebSocketRequestQueue between clients

* Add global limit and a way to change gateway limits

* Refactoring variable to fit lib standards

* Update xml docs

* Update xml docs

* Move warning to remarks

* Remove specific RequestQueue for WebSocket and other changes

The only account limit is for identify that is dealt in a different way (exclusive semaphore), so websocket queues can be shared with REST and don't need to be shared between clients anymore.

Also added the ratelimit for presence updates.

* Add summary to IdentifySemaphoreName

* Fix spacing

* Add max_concurrency and other fixes

- Add session_start_limit to GetBotGatewayResponse
- Add GetBotGatewayAsync to IDiscordClient
- Add master/slave semaphores to enable concurrency
- Not store semaphore name as static
- Clone GatewayLimits when cloning the Config

* Add missing RequestQueue parameter and wrong nullable

* Add RequeueQueue paramater to Webhook

* Better xml documentation

* Remove GatewayLimits class and other changes

- Remove GatewayLimits
- Transfer a few properties to DiscordSocketConfig
- Remove unnecessary usings

* Remove unnecessary using and wording

* Remove more unnecessary usings

* Change named Semaphores to SemaphoreSlim

* Remove unused using

* Update branch

* Fix merge conflicts and update to new ratelimit

* Fixing merge, ignore limit for heartbeat, and dispose

* Missed one place and better xml docs.

* Wait identify before opening the connection

* Only request identify ticket when needed

* Move identify control to sharded client

* Better description for IdentifyMaxConcurrency

* Add lock to InvalidSession
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.

Clients don't respect the gateway identify rate-limit.
4 participants