Breaking: `Event` is no longer a `CoroutineScope`

[lukellmann]

Problem

Currently, the dev.kord.core.event.Event interface extends CoroutineScope, but this is problematic.

The reason is that events don't have a well-defined lifecycle, as we never call CoroutineScope.cancel() for them (and there is no obvious place to do so). The documentation even highlights that the "key part of custom usage of CoroutineScope is cancelling it at the end of the lifecycle".

This wouldn't be too much of a problem in itself, but it leads to a memory leak, as @Stonedestroyer discovered.

Memory Leak Details

The way we create the CoroutineScope instances for use by Events (via delegation) is the following:

fun kordCoroutineScope(kord: Kord) = CoroutineScope(kord.coroutineContext + SupervisorJob(kord.coroutineContext.job))

This means that we create a parent-child hierarchy between the Jobs of the scope of Kord (parent) and the newly created scope of Event (child).

The hierarchy has a relevant impact: the parent now holds a reference to the child. When the child is cancelled, this reference is released so that the child can be garbage collected. But we actually never cancel the child, which results in a memory leak as the parent keeps accumulating references to children that will never be released.

It can even happen that multiple scopes will leak for a single event, as this for loop creates a new scope with the described parent-child hierarchy for every listener it tries to call.

Also see this thread on the Kord Discord Server.

Solution

As a solution, this PR removes CoroutineScope as a supertype of Event.

This is a breaking change, however there are easy replacements for cases where Event was used as a CoroutineScope:

• for coroutines that need a local scope, the coroutineScope { ... } scoping function can be used, it "is designed for parallel decomposition of work" and enforces structured concurrency
• for coroutines that need a scope which is more global, the Kord instance can be used, it is also a CoroutineScope but one that has a well-defined lifecycle (see Kord.shutdown())

[gdude2002]

It's possible that this will be an irreversibly breaking change for KordEx, as our event handling launches event handlers in the coroutine scope provided by the event.

This might seem a bit odd at first, but because Kord doesn't provide a way to add or modify contextual event data, this was the hack that @ByteAlex used to make KordEx even close to viable with his Dispers gateway.

Before this PR is merged, I would prefer to see ways to make Kord objects viable without logging in, and ways to intercept and modify events, as has been promised by the team for months.

[HopeBaron]

I've been short on time lately to look into anything that would keep the sealed nature of the events while allowing for extensiblity of the events. Essentially we could assume that we can open the event's tree AKA open the event classes while keeping its inherited properties on our side closed.

As per my last comment on Discord it seems to be that we will have to open the events some day However I believe this would've lots of disadvantages down the line. Edit: In regards to allowing custom interception of the current event life cycle I don't see any problems with it on the first glance CC @BartArys @Lukellmann @DRSchlaubi

[gdude2002]

One way to band-aid events for people with frameworks would be to assign a UUID to each event. Another (potentially more accessible) option would be to add a property which is a MutableMap<String, Any>, since that would be removed at the end of the event's lifecycle - that's what KordEx did for its check contexts and, while it feels like a bit of a hack, it's been pretty invaluable for some users.

You can see precisely what that entailed here.

[Stonedestroyer]

After 6 hours of testing with an example bot linked at https://github.com/Stonedestroyer/kord-example on a token with 3500 servers I can confirm that the coroutines are getting garbage cleaned and the bot is staying at a consistent 1.7 GB RAM compared to before where it was spiking always.

[HopeBaron]

After 6 hours of testing with an example bot linked at https://github.com/Stonedestroyer/kord-example on a token with 3500 servers I can confirm that the coroutines are getting garbage cleaned and the bot is staying at a consistent 1.7 GB RAM compared to before where it was spiking always.

That's nice to hear

[HopeBaron]

One way to band-aid events for people with frameworks would be to assign a UUID to each event. Another (potentially more accessible) option would be to add a property which is a MutableMap<String, Any>, since that would be removed at the end of the event's lifecycle - that's what KordEx did for its check contexts and, while it feels like a bit of a hack, it's been pretty invaluable for some users.

You can see precisely what that entailed here.

You are suggesting to add a map like so

interface Event { 
  ...
  val extras: MutableMap<String, Any>
}

But how would the user add these extras

[gdude2002]

No, I'm suggesting a MutableMap, not just a Map.

Getters and setters can be added via extension functions.

[HopeBaron]

I've modfied the example just before you commented Tho in a real scenario wouldn't you wrap those extras into another objects later on?

[gdude2002]

Not really - this is really something that Kotlin doesn't have a good solution for, so I don't see why a set of extension functions for get/set and early handling of the event to add data wouldn't work well enough. You don't get compiler-enforced safety for the cast, but that ultimately doesn't matter with some well-written functions.

[lukellmann]

This might seem a bit odd at first, but because Kord doesn't provide a way to add or modify contextual event data, this was the hack that @ByteAlex used to make KordEx even close to viable with his Dispers gateway.

Can you explain this hack a bit more? I'd like to understand the problem better.

[gdude2002]

So, as you may have heard, the really big bots don't actually connect directly to Discord. Most of them use some kind of gateway proxy or load balancing for command processing, which ultimately means that bots don't have access to an actual Discord token. For this reason, those bots often need to attach extra context to an event before the command handlers see it - for example, to pass in data that the command may need, but that isn't provided in the event, since the bot has no way of requesting it later.

This was the primary reason @ByteAlex suggested the way KordEx now handles coroutine contexts for events - there are (even more hacky) ways to associate data with a specific coroutine context, so he was assigning a new context to every event he was passing into KordEx, in order to store and later retrieve that data.

[lukellmann]

One way to band-aid events for people with frameworks would be to assign a UUID to each event.

Wouldn't the object identity of the event serve the same purpose?

[lukellmann]

My last 2 commits made the diff clearer, reviews should be easier now.