Roadmap: consolidating Actions and gateway handlers

[kyranet]

Probably our oldest standing task in our roadmap, dating back from 12-dev's early days, in fact it was supposed to be removed when we implemented internal sharding.

At the moment, we have actions and websocket handlers, the former are methods that are called in various places, such as in structures, in managers, and in other actions. The latter literally just calls actions with the exception of CHANNEL_PINS_UPDATE, CHANNEL_UPDATE, GUILD_CREATE, GUILD_MEMBER_ADD, GUILD_MEMBERS_CHUNK, MESSAGE_UPDATE, READY, RESUMED, THREAD_UPDATE, and VOICE_SERVER_UPDATE. There's also a lot of inconsistency here, some of those handlers emit events, where others delegate that job to the actions, which may be called in other places of the code. In fact, when we add support for more gateway events, we have to decide whether or not we make an action and emit the code there, or just handle the code in the websocket handler, there isn't really a rule for where it should be at, and has never been.

In fact, actions are so confusing, we even had to write a large comment to explain their purpose (and are still quite unclear on their responsabilities, as seen above).

Actions and websocket events have a lot in common, the only difference is that actions have side effects, which we are slowly removing, #7076 as example. In fact, actions caused many issues in the past, such as messages returning the wrong data, double-patching, etc.

Let's suppose we remove actions and move all their code to the websocket handlers, now what? Well, I have 2 great news:

1. This change will simplify internals a lot, as they'll be more predictable thanks to the removal of side effects.
2. We are one closer to supporting async caches! This is one of the most requested features (see the following link for some information: Roadmap: cache and sweeping improvements #6539 (comment)), and has been put off due to many technical issues, from overhead (forcing async everywhere that used to be sync) to increasingly complex code. Turns out, gateway handlers already allow to be overridden and made async without a performance in the library, that's because unlike actions, their return value does not matter.

Solution

Consolidate actions and websocket handlers, we'll probably need to turn the latter into classes so we can benefit from the shared cache methods.

We'll also need to remove all references to client.actions, which is referenced 72 times as of 7959a68, 45 of the usages are websocket handlers, which are straightforward to translate, but the remaining 27 references may have some usage of the actions that can be tricky and require further rethinking/refactoring.

[mgmolisani]

I'm not sure if this is related but I was trying to better understand the internals of when events were emitted. I used message creation as my case study. I know from experience, the messageCreate event is emitted even when it came from the channel send method. However, after reviewing the code, this was actually surprising as the handler intends to bail out if the message is in the cache which send should be doing. Correct me if I'm wrong, but this code for the send method on a text channel has potential issues with race conditions that would potentially affect if a messageCreate event gets emitted.

async send() {
  ...
  const d = await this.client.api.channels[this.id].messages.post({ data, files });
  // event loop could return here before handler is run and affect the cache the handler uses
  return this.messages.cache.get(d.id) ?? this.messages._add(d);
}

It looks like in the last two lines it awaits the API sending the message but adds it to the cache after it returns with the payload. This means the gateway has a chance to trigger a messageCreate event and the message will not have been cached yet. This is actually usually what happens and feels like it's what was intended. However, if the gateway was slower than when the event loop handled the return of the API call, the behavior would change as the message would be considered cached.

So scenarios to recap are:

1. Bot sends message request -> handles that messages gateway creation event -> request comes back -> sees ID already in cache from gateway handler
2. Bot sends message request -> request comes back -> adds message to cache -> handles that messages gateway creation event (bails out early, no emit occurs)

Am I right in my review? Is this something to think about with the future of these action handlers?

[kyranet]

The issue you're mentioning isn't related at all to actions or gateway handlers, just about an specific structure doing something outside of them.

To answer you, the cache retrieval is most likely useless, REST is generally a lot faster than WS is, but it's there in case Discord decides to change the order.

Also, if the entry is added to cache, _add will merely retrieve and call message._patch, so it's updated in-place.