Adds better abilities to check, what exactly was rate limited #2901

TheCataliasTNT2k · 2024-06-12T22:47:11Z

Add "OwnedRoute" to "RatelimitInfo" which allows to check the cause of the rate limit and which identifiers were affected (e.g. which channel).

TheCataliasTNT2k · 2024-06-12T22:51:21Z

The failing check seems broken, at least I do not get any error when looking at the details...

jamesbt365 · 2024-06-12T22:54:02Z

The failing check seems broken, at least I do not get any error when looking at the details...

cargo +nightly fmt --all

src/http/routing.rs

GnomedDev · 2024-06-16T15:28:34Z

src/http/routing.rs

+        #[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
+        enum RouteKind {
+            $($name,)+
+        }


Why was this moved?

To have two similar blocks next to another (Route and OwnedRoute).
I could move it back, but in my opinion it is more readable like this.

GnomedDev · 2024-06-16T15:29:21Z

src/http/routing.rs

+            /// please match the variants you are interested in.
+            #[must_use]
+            #[allow(unused_variables)] // prevent compiler from complaining about unused variables
+            pub fn get_common_identifiers(&self) -> RatelimitCause {


Suggested change

pub fn get_common_identifiers(&self) -> RatelimitCause {

pub fn cause(&self) -> RatelimitCause {

This does not really return the "cause" of the rate limit but the parts of the path, which was rate limited.
The "cause" would be the deletion of a message or something like that.

So the type should not be caused RatelimitCause, it should be called RatelimitPathParts or something else.

That is better, yes.
Can the method name stay then?

GnomedDev · 2024-06-16T15:30:55Z

src/http/routing.rs

    ($lt:lifetime, {
        $(
-            $name:ident $({ $($field_name:ident: $field_type:ty),* })?,
+            $name:ident $({ $($field_name_route:ident: $field_type:ty | $field_type_owned:ty),* })?,


Instead of making the macro syntax messier, can you make this use traits instead? I'm pretty sure ToOwned should handle this, but if it doesn't you can just write a simple private trait.

How? I somehow need to know the type, which the OwnedRoute should use. I do not know, how a trait would be able to do that.
The problem is the "&str" type. Can you please provide an example for how to do it?

The owned type is simply <$field_type as ToOwned>::Owned, and if that doesn't return the correct types you can duplicate the definition of ToOwned and implement it for the types needed.

Will do that.

The field_type_owned is actually needed, I did not find a way to get compiling code without it.

Reason:
The macro would expand into something like this (irrelevant code removed):

#[derive(Clone, Debug)] pub enum OwnedRoute { ChannelMessageReaction { channel_id: <ChannelId as ToOwned>::Owned, message_id: <MessageId as ToOwned>::Owned, user_id: <UserId as ToOwned>::Owned, reaction: <&'a str as ToOwned>::Owned}, }

<&'a str as ToOwned>::Owned is not valid, because 'a does not exist. I could add the lifetime to the enum, but then we would need to carry this useless lifetime though the whole code. This can not be fixed by replacing the <&'a str as ToOwned>::Owned with a custom macro, I already tried:

macro_rules! get_owned_type { (&'a str) => {String}; ($type:ty) => {<$type as ToOwned>::Owned}; } #[derive(Clone, Debug)] pub enum OwnedRoute { ChannelMessageReaction { channel_id: get_owned_type!(ChannelId), message_id: get_owned_type!(MessageId), user_id: get_owned_type!(UserId), reaction: get_owned_type!(&'a str)}, }

will not compile for the same reason.

The compiler is not smart enough to ignore the unused lifetime at the moment.

GnomedDev · 2024-06-16T15:33:45Z

src/http/routing.rs

+            }
+
+            #[must_use]
+            pub fn get_owned_route(&self) -> OwnedRoute {


Rust convention is to name getters without the prefix, but since this is a &self -> Owned I think the convention would be to_owned_route... however that's just ToOwned, so implement ToOwned instead.

I did not use toOwned, because this is not really a clone and should not be treated as one, which is expected by the trait however.

With that logic ToOwned should not be a thing, but this is exactly what it's for. This is internally just converting str -> String which is exactly the point of ToOwned.

This can not be done using ToOwned, because this would require the "copied" object to carry the lifetime with it.

I already tried that sometime, and it makes the whole code a mess.

I renamed the method though (not pushed yet because of the other change requests).

GnomedDev

I cannot review this PR without it being rebased correctly on-top of next.

TheCataliasTNT2k · 2024-11-25T00:23:38Z

I cannot review this PR without it being rebased correctly on-top of next.

Was doing that right as you wrote that, lol xD

…erenity-rs#2646) This avoids having to allocate to store fixed length (replaced with normal array) or fixed capacity (replaced with `ArrayVec`) collections as vectors for the purposes of putting them through the `Request` plumbing. Slight behavioral change - before, setting `params` to `Some(vec![])` would still append a question mark to the end of the url. Now, we check if the params array `is_empty` instead of `is_some`, so the question mark won't be appended if the params list is empty. Co-authored-by: Michael Krasnitski <[email protected]>

These are unnecessary. Accepting `impl Into<Arc<T>>` allows passing either `T` or `Arc<T>`.

Fixes CI failure

…rs#3075) Similar to message URLs, Discord also provides URLs for guild channels. Additionally, the function is added as an alternative for parsing a `Channel` from a string. Private channels are not affected by this change.

…rs#3084) https://discord.com/developers/docs/events/gateway-events#thread-create

The `Deserialize` implementation neglects to add the `Bot ` prefix to the string when it is deserialised. This adds `TryFrom` implementations for `&str` and `String` and tells serde to deserialise `Token` using the `TryFrom<&str>` implementation, which will prepend the `Bot ` prefix. Fixes serenity-rs#3085

…#3061)

This commit refactors how the gateway connection being closed gets handled, and also reworks how resuming is performed. If a resume fails, or if the session id is invalid/doesn't exist, the shard will fall back to restart + reidentify after a 1 second delay. This behavior was only present in some circumstances before. Also, cleaned up the loop in `ShardRunner::run` by adding a `ShardAction::Dispatch` variant, since event dispatch was already mutually exclusive to hearbeating, identifying, and restarting. The overall effect is less interleaving of control flow. Plus, removed the `Shard::{reconnect, reset}` functions as they were unused. A notable change is that 4006 is no longer considered a valid close code as it is undocumented, and neither is 1000, which tungstenite assigns as `Normal` or "clean". We should stick to the [table of close codes](https://discord.com/developers/docs/topics/opcodes-and-status-codes#gateway-gateway-close-event-codes) provided by Discord.

A regression introduced by serenity-rs#3099 was that successful resumes will break out of the loop inside `ShardRunner::run`, but they shouldn't (or rather, didn't before). Therefore, only break out of the loop if the resume failed and we had to fallback to reidentifying.

…enity-rs#3110)

…3111) As the title notes, this commit replaces fxhash for foldhash as used in the cache. dashmap, due to it's sharding, has to share entropy with what's handed down to internal maps. Since `hashbrown` and by extension `std` use various sections of the high bit range for special grouping & sorting, dashmap is left with the only option to shard on low bits. This, however, presents problems, because fxhash outputs hashes of very bad quality, with only the high bits having any real entropy. This was probably a solid choice back in 2018 when we lacked other good fast alternatives. But since then `ahash` matured and we've had significant research and development in "good enough" hashing for datastructures with short keys, [the most recent step forward coming from a rather well known face][foldhash]. This improves shard selection quite a bit and reduces contention significantly. Using fxhash in a dashmap specific benchmark causes contention to go up by 3-8x when keys are k-sortable with time (Discord snowflakes) on an M1 Pro. [foldhash]: https://github.com/orlp/foldhash

…s#3088) This replaces the time unit that determines how many messages in a period of X time should be deleted when a user is banned. Discord expresses this time in seconds, whereas Serenity exposed it in days. The limit is still 7 days, but with this, users of Serenity can be more precise with which messages they want to delete.

…#3115)

…ap tree (serenity-rs#3114) Avoid temporarily deserializing gateway messages to a `serde_json::Map<String, Value>` before typed deserialization to an `Event`. Previously, this meant the creation of a `serde_json::Value` tree, causing creation and immediately after the destruction of upwards of hundreds to thousands of owned strings and btreemaps for every handled gateway event.

It is just a copy of Route without any lifetimes.

TheCataliasTNT2k changed the base branch from current to next June 12, 2024 22:47

github-actions bot added the http Related to the `http` module. label Jun 12, 2024

GnomedDev requested changes Jun 16, 2024

View reviewed changes

GnomedDev force-pushed the next branch 2 times, most recently from c01e57d to 842373b Compare June 22, 2024 20:38

arqunis force-pushed the next branch from adf4673 to 61d6ddb Compare July 30, 2024 17:27

GnomedDev force-pushed the next branch from 9d41131 to b167b4d Compare August 16, 2024 13:22

GnomedDev force-pushed the next branch from 2645fcc to 4d54465 Compare October 7, 2024 22:47

GnomedDev force-pushed the next branch from 4d54465 to 391b234 Compare October 20, 2024 17:32

GnomedDev force-pushed the next branch 3 times, most recently from 9296017 to efb5820 Compare November 15, 2024 19:29

TheCataliasTNT2k requested a review from GnomedDev November 24, 2024 15:34

GnomedDev requested changes Nov 25, 2024

View reviewed changes

TheCataliasTNT2k force-pushed the better_ratelimits branch 2 times, most recently from cf3701f to ec3a62e Compare November 25, 2024 00:17

TheCataliasTNT2k force-pushed the better_ratelimits branch 2 times, most recently from 7e6b73b to 9b2d176 Compare November 25, 2024 00:35

TheCataliasTNT2k requested a review from GnomedDev November 25, 2024 18:01

arqunis force-pushed the next branch from d7c5ee3 to 7e90b9d Compare December 8, 2024 18:51

arqunis force-pushed the next branch from 5943323 to 5f07257 Compare January 16, 2025 20:57

arqunis force-pushed the next branch from e71723e to 872e19c Compare February 1, 2025 14:44

GnomedDev and others added 5 commits February 13, 2025 18:32

Remove deprecated items (serenity-rs#2645)

026fa18

Remove more deprecated items (serenity-rs#2651)

bc8e362

Remove *_arc methods (serenity-rs#2654)

473ee03

These are unnecessary. Accepting `impl Into<Arc<T>>` allows passing either `T` or `Arc<T>`.

Clean up ShardManager/ShardQueuer/ShardRunner (serenity-rs#2653)

f394d5c

mkrasnitski and others added 15 commits February 13, 2025 20:44

Fix clippy warning on next (serenity-rs#3078)

6f2fc48

Fixes CI failure

Fix clippy warning (serenity-rs#3081)

cbfc682

Add newly_created bool field to the ThreadCreate event (serenity-…

56a7dec

…rs#3084) https://discord.com/developers/docs/events/gateway-events#thread-create

Fix issues from rebase

b59b693

Remove ContentSafeOptions::show_discriminator

9c69934

Change MessageUpdateEvent to just contain a full Message (serenity-rs…

4b76dd2

…#3061)

Update tokio-tungstenite to v0.26.1

06d2352

Fix clippy

6bc1e5b

Deserialise Token from Strings instead of &strs (serenity-rs#3104)

ce38b8f

Fix clippy

38ac76b

Fix compilation when the tracing_instrument feature is enabled (ser…

3923997

…enity-rs#3110)

arqunis force-pushed the next branch from 55e6901 to 3923997 Compare February 14, 2025 18:46

xacrimon and others added 5 commits February 14, 2025 21:21

Remove deprecated field inside GatewayEvent::Heartbeat (serenity-rs…

e5f8014

…#3115)

Adds "OwnedRoute".

9b7a73a

It is just a copy of Route without any lifetimes.

TheCataliasTNT2k force-pushed the better_ratelimits branch from 9b2d176 to 9b7a73a Compare February 16, 2025 23:16

arqunis force-pushed the next branch 2 times, most recently from 57c79ff to 9a811a7 Compare March 11, 2025 22:17

GnomedDev force-pushed the next branch from d29b7a6 to da271c8 Compare March 26, 2025 21:28

GnomedDev force-pushed the next branch from 6e9fe28 to 4a180eb Compare April 28, 2025 12:35

GnomedDev force-pushed the next branch from 956f5f1 to f085918 Compare May 19, 2025 20:26

arqunis force-pushed the next branch from 361e2fc to ffaf185 Compare June 30, 2025 17:17

arqunis force-pushed the next branch from d5024bc to 580c6b9 Compare July 28, 2025 19:49

arqunis force-pushed the next branch from 756ddf4 to 90514ac Compare October 7, 2025 17:31

	pub fn get_common_identifiers(&self) -> RatelimitCause {
	pub fn cause(&self) -> RatelimitCause {

Adds better abilities to check, what exactly was rate limited #2901

Are you sure you want to change the base?

Adds better abilities to check, what exactly was rate limited #2901

Uh oh!

Conversation

TheCataliasTNT2k commented Jun 12, 2024

Uh oh!

TheCataliasTNT2k commented Jun 12, 2024

Uh oh!

jamesbt365 commented Jun 12, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheCataliasTNT2k Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheCataliasTNT2k Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GnomedDev left a comment

Choose a reason for hiding this comment

Uh oh!

TheCataliasTNT2k commented Nov 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

21 participants

TheCataliasTNT2k Nov 22, 2024 •

edited

Loading

TheCataliasTNT2k Nov 22, 2024 •

edited

Loading