Removal of `.deleted` in structures

kyranet commented 2 years ago

Reverts the proposal from #2489.

How did we implement `deleted`?

Currently, storing the deleted field requires 2 approaches, one which adds a substantial amount of MBs in memory but is fast for the CPU, and another which solves the RAM usage, but increases CPU usage significantly; this leads us to a point where no matter what we do, tracking this field has a serious negative impact in either CPU or RAM.

I think that after 3 years, we're at a point where we can determine whether or not we want to proceed using this, and while .deleted makes status tracking easy, it's also extremely flawed and unreliable.

Where do we track and set `deleted`?

What classes do we track with .deleted, and when do we assign them?

Channel:
- In the ChannelDelete event.
- ⚠️ Not set as deleted in GuildDelete.
Guild:
- In the GuildDelete event.
GuildEmoji:
- In the GuildEmojiDelete event.
- ⚠️ Not set as deleted in GuildDelete.
GuildMember:
- In the GuildMemberRemove event.
- ⚠️ Not set as deleted in GuildDelete.
Role:
- In the GuildRoleDelete event.
- ⚠️ Not set as deleted in GuildDelete.
Sticker:
- In the GuildStickerDelete event.
- ⚠️ Not set as deleted in GuildDelete.
Message:
- In the MessageDelete event.
- In the MessageDeleteBulk event.
- In the ChannelDelete event, where it checks whether or not a channel has messages, then iterates through all of them to mark them as deleted.
- In the ThreadDelete event, where it iterates through all the messages in cache to mark them as deleted.
- ⚠️ Not set as deleted in GuildDelete.
StageInstance:
- In the StageInstanceDelete event.
- ⚠️ Not set as deleted in GuildDelete.
ThreadDelete:
- In the ThreadDelete event.
- ⚠️ Not set as deleted in ChannelDelete.
- ⚠️ Not set as deleted in GuildDelete.

Why don't we set all those structures as deleted when we delete a guild? While we could, the performance implication of that would be huge, imagine a guild where you fetch all the members, and it happens to have around a million entries, alongside hundreds of channels (with threads), a lot of roles, etc. You see the issue here? If you don't, this package would render your bot unresponsive for at least a solid second. Do we want to offer this? Definitely not.

Also... we track those states with those events, but... aren't we missing things? Yeah, we are:

GuildBan.
Invite.
MessageReaction.

Ok, we track a lot, but can't we just track deleted in those? We could. But should we? Is there any benefit in this? Now imagine if we marked everything as deleted from the guild, we'd quickly have to delete possibly dozens of thousands of bans, hundreds of invites, and even thousands of message reactions if the caches have a huge pool (which happens quite often for moderation bots that log deleted messages, since they need the cache to get the content).

Raceconditions

Also, sometimes the value is incorrect due to race-conditions as it relies on WS events, which may fire after REST succeeds replying, that is the case for the following two blocks of code returning a misleading value:

await message.delete();
message.deleted; // false

const m = await message.delete();
m.deleted; // false

Setting them to false in the method would lead to double-setting, and we always mutate the state in WS events over REST responses when possible. Obviously, this is not the only place where this happens.

Integration with sweepers

Now there's another critical issue, with v13, we have upped our memory management game with LimitedCollection, first in #6013 by limiting the amount of entries we store in each store, and later with #6110 adding a sweeper in most of the LC's, something that only the message cache had. In the near future, we'll also have #6825, which improves the memory usage by not storing hundreds of thousands of intervals in mid-sized bots (and a lot more in large bots). Now, here goes another issue down the lane, we only set deleted when entries are in discord.js's managed cache, we just cannot do that for entries that are swept or removed manually by the user, so once that happens, what deleted returns can be... incorrect. Furthermore, clones of all structures do not spread the value of deleted, and we'd need to make manual checks so they're passed, but of course, that comes with a performance cost, see https://github.com/discordjs/discord.js/pull/7074#issuecomment-989903132 where it shows that WeakSet accesses are up to 70 times slower than properties.

`deleted`'s usage in the ecosystem

Also, for the majority of users, deleted is not checked, and when they do, it's usually always true for them unless on *Delete events, where they're set as false. If they need to know whether or not a structure is deleted, why can't we just make it a plugin of sorts? We could stop tracking deleted just like we did for typings. Besides, a more reliable way to handle state and always make sure that entries are deleted no matter the state in cache, is to listen to use partials or raw events to get the IDs and remove them from a Map, if there was any need of doing such thing, but as it stands right now, third-party tools that depend on cache are often made requiring the existence of that field, and as such, they also often don't implement fail-safes or recovery code to ensure the system continues functioning even when a message, channel, or something else, is deleted.

Maybe users won't check it, but maybe Discord.js will! Well, out of the 9 structures that have the field, only 1 actually reads it, and it's in the following 4 utility getters, where they all return false when deleted is also false:

Message#editable
Message#deletable
Message#pinnable
Message#crosspostable

Admittedly, this field was proposed because of some requirements in the now-archived Klasa framework, where their editable messages and their paginated message utility required to know whether or not a message was deleted, which also led to other issues such as gotchas and issues when you wanted to run a paginated message longer than the message sweeper allowed (or delayed in a way that alongside the timer, exceeds the sweeper's maximum time), even though the framework could have attached listeners to sweep those entries without the need of forcing something to everyone using the library, even when just the 0.01% of the users benefit from it.

Note that I have listed 2 use-cases of the property, but they're both easily solvable:

Editable commands are disappearing now that Discord is pushing bots to use slash commands over message-based commands, also not a use-case we'd support after this fact.
Paginated messages are already mapped somewhere so they may be updated according to events, they can just have a couple more listeners to listen to deletions and clean themselves up.

Summary

Due to all of this, from increased CPU and RAM usage, as well as higher development cost and all the gotchas the field has, I believe its issues greatly outweight its usefulness, and should be removed (v14).

KhafraDev commented 2 years ago

Klasa framework, where their editable messages and their paginated message utility required to know whether or not a message was deleted

I use similar functionality in my bot, where I check if a message is deleted before editing it (to prevent an extraneous api call if possible). Should I replace this by trying to edit the message and catching an error if one occurs instead, or is there a better solution?

kyranet commented 2 years ago

The simplest way is to use the WeakSet trick from #7074 and do the following:

const deletedMessages = new WeakSet<Message>();

export function isMessageDeleted(message: Message) {
    return deletedMessages.has(message);
}

export function markMessageAsDeleted(message: Message) {
    deletedMessages.add(message);
}

Then in the events you want to listen, do isMessageDeleted(message) where previously it'd be message.deleted.

Another alternative is, if you have access to the messages by their IDs, you can either use events with partials, or use raw events to get the ID of deleted messages and use them to clear the command responses and/or re-create/stop the message pagination, whichever you're most comfortable with. If your pagination works with discord.js's collectors, you won't need anything special because stop will be emitted when the message, channel, or guild, is deleted.

Obviously, remember to always include a recover strategy just in case, because even if a message isn't marked as deleted, other bot (or something else) could delete your bot's message faster than your bot edits it, and in this case, it'd be nice if you could have such system.

If you don't want to do any of that, I guess you could use Sapphire's packages, it powers part of Discord.js and its official framework: there's @sapphire/plugin-editable-commands and PaginatedMessage from @sapphire/discord.js-utilities, which are actively maintained.

switz commented 2 years ago

I get not tracking it for high volume stuff like messages, but it's super handy for channels – which should be much lower volume. Would you consider keeping it in for channels?

kyranet commented 2 years ago

No, you can track that quite easily, just implement a WeakMap and fill/remove as you need.

suneettipirneni commented 2 years ago

Klasa framework, where their editable messages and their paginated message utility required to know whether or not a message was deleted

I use similar functionality in my bot, where I check if a message is deleted before editing it (to prevent an extraneous api call if possible). Should I replace this by trying to edit the message and catching an error if one occurs instead, or is there a better solution?

I was just looking into this and I realized that you don't even need to manually track deletions to make pagination work. If you use a collector you can just check the end event and check if the end reason is messageDelete.

Kief5555 commented 2 years ago

What would we use if .deleted is unavailable?

kyranet commented 2 years ago

You just literally read the messages in this issue, @Kief5555, it's full of solutions.

discordjs / discord.js