v5 extensions and events design

[akiraveliara]

Description

This is intended to be a bit of an open discussion issue about extensions and events in v5. Of all extensions, this primarily affects Commands and Interactivity, which heavily rely on both the extension and event systems, but when designing, we should keep other extensions in mind.

There are many flaws in the current event system, starting with being able to mutate the event handlers after the gateway starts, which leads to quite some overhead: https://github.com/DSharpPlus/DSharpPlus/blob/master/DSharpPlus/AsyncEvents/AsyncEvent%602.cs#L91-L105
This also includes legacy code such as https://github.com/DSharpPlus/DSharpPlus/blob/master/DSharpPlus/AsyncEvents/AsyncEventArgs.cs#L17 that we should remove, and of course the dreaded Task vs ValueTask issue.

We have also for a while now envisioned events participating in DI in some way or another, #1683 touches upon that topic and it comes up from time to time in support.

---

Enlightening the main client about DI now ties into extensions. First, we could see extensions participating in DI, both in their creation and in users being able to request the extension in their own services. The main use-case coming to mind here is naturally Interactivity, but users are only limited by their own creativity here; but this provides significant advantages in creating the extension objects too - from trivially customizing parts of how the extension works, such as ICommandExecutor in Commands to error handling using a default-implemented service users can shim individual methods of or replace entirely.

Generalizing this concept then begs the question of how to treat DiscordClient. On one hand, DiscordClient (and DiscordShardedClient) are the main entrypoints of any DSharpPlus bot, it is nearly impossible to interact with library code in any meaningful way without using one of those two, on the other hand, they would benefit from participating in DI in much the same ways as extensions, and exposing more components of the library as customizable services would bring a plethora of improvements - caching, ratelimits, the gateway transport layer (for example, supporting ETF as an extension to v5) come to mind.

This would lead us to changing the creation model, away from DiscordClient..ctor(...). There is plenty of design space here, and I'm entirely open to suggestions. I'll pitch the following API for discussion here:

• static DiscordClient.Create(string token, DiscordIntents intents); for simplest use-cases
• static DiscordClient.Create(DiscordConfiguration configuration); for slightly more advanced client-only use-cases (we can consider additional overloads)
• static DiscordClient.Create(Action<IServiceCollection> configuration); exposing the underlying service collection, which would allow more complex cases including registering extensions, but wouldn't be particularly clean for complex setup
• DSharpPlus.DiscordClientBuilder..ctor(); creating a new builder with an empty service collection,
• DSharpPlus.DiscordClientBuilder..ctor(IServiceCollection); creating a new builder from a provided service collection,
• and DSharpPlus.DiscordClientBuilder.Build(); creating a DiscordClient instance. We can then expose configuration on the builder, as well as letting extensions provide their own extension methods (for instance, setting the text command prefix could be something like DiscordClientBuilderExtensions.SetCommandPrefix(this DiscordClientBuilder builder, string prefix);)

Specify the libraries you want this feature request to affect

DSharpPlus, the core library, DSharpPlus.Commands, DSharpPlus.Interactivity

Other considerations

This is a fairly complex task and will most definitely take a while to implement, no matter what design we settle on. There are some preparatory tasks we can already work on, though, such as preparing the default logger to be used in more places and ways, migrating event dispatch to use ValueTask by default (optionally keeping compatibility for Task-based events, though the compiler error here should be obvious enough to let people migrate trivially) and to use immutable event handler collections.

As for design, there are many questions to solve here, starting with the elephant in many rooms - the deprecated extensions. Since they're deprecated, not removed, we'll have to update them to compile and ideally work. Do we care enough to update them to a completely new model or do we find some hacky minimum-effort solution, leaving them dangling between the old and new models? Do we fix the old extension classes up to be able to participate in DI?

I do think this is important to get done one of these days, because changing the extension model marks a huge breaking change for libraries such as Lavalink4NET to adapt to, and the sooner we can stabilize these things for the v5 release cycle, the more painless it will be to update.

As for events, what surface do we want to support? How do we want to provide event handlers with DI? Here, too, is a lot of open design space that will influence almost every bot and almost every extension targeting the library (note that 'almost' is a disclaimer in case there exists one that wouldn't be affected; I cannot provide any examples of either).

---

From here on, I will be aggregating feedback and the results of design discussions so we can move forward with implementing after we're confident in our design.

[ecrocombe]

I wish to ask that if we are going to use DI for incoming WS events/commands, that could we please create a scope and resolve dependencies through the scope instead? 🙏

[Plerx2493]

Yeah i think thats a good request. Question is how we want to scope it, one scope for all handler per event?

[Plerx2493]

For commands we already have a new di system in our new DSP.Commands package

[ecrocombe]

Yeah i think thats a good request. Question is how we want to scope it, one scope for all handler per event?

imo, if the handlers run concurrently (Task.WhenAll), they should get their own scope.
However, if they run synchronously, (1 after the other), they could use the same scope.

[Plerx2493]

they are concurrently

[ecrocombe]

First thing that come to mind for me is the use of EFCore and its DbContext. It doesn't like playing concurrently. By using a scope per concurrent handler, it would solve this issue for me (most likely others too).

[Kaoticz]

and of course the dreaded Task vs ValueTask issue.

Could you provide a bit more context on this? What's the issue around them, exactly?

[akiraveliara]

@Kaoticz Task pretty badly impacts the gateway in terms of allocations (and by extension, speed - event dispatch is pretty badly affected, for example, though there's other bad allocations in there too, as you can probably see) - in my prototyping (which was already a lot faster than the present, abysmal, implementation), swapping out Tasks for ValueTasks yielded a good 5-10% speed and memory footprint improvement, but handling ValueTasks properly makes the code a fair bit more complex (since we don't want to box/AsTask/etc anywhere) and they're easier to break (at least, when people ignore their warnings, which is unfortunately fairly commonplace judging by our support channel)

@ecrocombe that's a good point. i've put this on my list of things to invite discussion over.

[Kaoticz]

If ValueTasks are used internally in the library, I see no problem as long as library maintainers are willing to deal with the few extra quirks that ValueTask has. However, I'd be far more cautious to expose an API with ValueTasks to the user. As you've mentioned, users are often not aware of the implications that using ValueTask brings and will thus treat them as regular Tasks, which might lead to severe bugs and performance degradation.

Also, as pointed out by Stephen Toub, ValueTasks also take a little bit longer to await compared to Tasks, so if you are aiming for the ultimate performance, you probably want to use cached instances of Task wherever you can. The runtime already caches Task and Task<bool> instances automatically for you, so you don't need to worry about these. For other Task<T> types, you can create some global Task.FromResult(...) instances and return them wherever applicable.

This will naturally increase the overall maintenance burden of the library, so you should evaluate if the extra performance is worth the extra complexity.

[moorehousew]

Just a few thoughts here:

Scoping, if DSharpPlus wants to offer that functionality, should consistently be per-event, as services are lazily initialized and this is conceptually the cleanest EDIT: If multiple event handlers are running concurrently, then there's a strong argument for the scope to be per-handler. It should also be entirely optional, and the library/extensions should not rely on any sort of scope existing at all- if the library or an extension needs state to have a lifetime for some portion of an async flow, they can always use AsyncLocal<T>. Scope is really meant to be defined by the application/application host i.e. ASP.NET.

It would be nice if events had a signature where sender was of type object in all cases, just like the BCL.

In addition to what @Kaoticz has said, I would suggest using TaskCompletionSource<T> instead of Task<T> where applicable- especially for caching the results of IO-bound operations that can be performed on the thread pool.

As far as DI goes, it would be best if DSharpPlus adopted the standard mechanism .NET libraries use for supporting DI (again coming from ASP.NET here) and didn't get in the way of the application defining its own service provider lifetime. This is the hardest hurdle when developers want to also use, say, EF Core cleanly- or run their bot inside of an ASP.NET host. Something like:

IServiceCollection ServiceCollectionExtensions.AddDiscordClient(
    this IServiceCollection services,
    Action<DiscordClientBuilder> configure)

Where DiscordClientBuilder contains the properties/methods for configuring and constructing a DiscordClient, which is then registered with the service collection. See Microsoft.Extensions.*, most ASP.NET Core libraries, etc.

[akiraveliara]

• DSharpPlus already offers scoping... in some places. inconsistently. with different quirks depending on where. generally, save for larger/more involved bots, we are the application host and therefore are the people meant to be defining scope; but even if not, the fact DSharpPlus dispatches at times the host has little to no control over means that we're still logically the root of the operation and therefore probably defining scope
• why should the library not rely on any sort of scope at all? it's not a question currently or with anything we have planned; but if that is to be a rule we employ in designing the library, we need rock-solid justification for that
• considering senders are reference types, what benefit would changing to object bring?
• most of what we do can't really benefit from TCS much, but if you have specific suggestions i'll be happy to look at them
• adopting the 'standard' mechanism while also providing a way to do it without - which is what most people do - is the problem here, not generally adopting said 'standard' mechanism; but i've noted the proposed API - i like it in principle, it's how what i have of v6 so far is designed

[ecrocombe]

As a user of DSharpPlus (not a dev), I have my own ideas on how events should be done. which right now is a mixture of everyone here's opinion.

What are everyone's thoughts on using a mediator pattern?

Pro's of having our own "EventBus"/"EventPublisher"/"Mediator"

• Support for "ScopePerEvent", "ScopePerEventHandler" or "360_NoScope" options in startup config. Allow the user to define if and/or when to use a scope.
• Support for "TaskForEach" or "TaskWhenAll" event handler execution defined in startup config. Allow the user to configure event handler concurrency.
• Support for users to somehow override the IMediator? to send events to external systems such as RabbitMQ or MSMQ with ease.
• Ability to unregister handlers.

Con's:

• ?

Some examples of the mediator pattern can be found:

• MediatR https://github.com/jbogard/MediatR
• ASP.net Boilerplate https://github.com/aspnetboilerplate/aspnetboilerplate/blob/dev/src/Abp/Events/Bus/EventBus.cs

[akiraveliara]

i can promise you right here and now that there will not be default configuration for handler concurrency or the ability to unregister handlers.

that said, allowing users to override the dispatcher is absolutely a thing we can do under this issue; and if people want to have bad ideas like synchronous handlers, they can themselves

[ecrocombe]

i can promise you right here and now that there will not be default configuration for handler concurrency or the ability to unregister handlers.

I'm not sure if you're saying it's not possible, or it's not going to be done under this GitHub issue, either way ill just leave this here as food for thought

• https://github.com/jbogard/MediatR/blob/88c4d73ce514a64a9083aeb8df2e2c3692049b7d/src/MediatR/MicrosoftExtensionsDI/MediatrServiceConfiguration.cs#L27
• https://github.com/jbogard/MediatR/tree/master/src/MediatR/NotificationPublishers

I'm not sure unregistering event handlers would be of high demand anyway tbf

that said, allowing users to override the dispatcher is absolutely a thing we can do under this issue; and if people want to have bad ideas like synchronous handlers, they can themselves

Very true, and D#+ can not/will not be responsible for others code, only its own code.

[moorehousew]

generally, save for larger/more involved bots, we are the application host and therefore are the people meant to be defining scope [..]

I think the distinction you have here is important. If the application developer doesn't care/know anything about scope, DSharpPlus is free to decide what Scoped means for their application.

why should the library not rely on any sort of scope at all?

I really just want configurability here- if you want to use scopes, I'd like to be able to configure that use to align with what the other libraries I'm using (and my application logic) expects. @ecrocombe's ScopePerEvent / ScopePerEventHandler / NoScope is sufficient configuration for my use-case. Overriding the dispatcher to facilitate that works as well.

most of what we do can't really benefit from TCS much, but if you have specific suggestions i'll be happy to look at them

I might poke around in the library some more to find a better suggestion, but wouldn't the cached users in DiscordClient be a good candidate? If you have a concurrent dictionary of TCS instead of just DiscordUser, you spare the end-user from wasting multiple API requests for the same user ID when the method is called concurrently. Any similar caching patterns could benefit from the same.

[VelvetToroyashi]

If the application developer doesn't care/know anything about scope, DSharpPlus is free to decide what Scoped means for their application.

This only holds true if all services are either

• Singletons, and thusly always resolved from the root scope
• Scoped/Transient AND are stateless

Generally, it's the former, but the library should still take care in not being reckless with scoping, as it's both expensive at runtime, and can cause issues that happen specifically because of how we instantiate these scopes

ScopePerEvent / ScopePerEventHandler / NoScope is sufficient configuration for my use-case. Overriding the dispatcher to facilitate that works as well.

This is a feasible implementation to have, but muddies dispatch code because of the extra state handling, and having to conditionally create a scope in two different areas. Perhaps this could be delegated to something along the lines of ConfigureableScopedDispatcher, cc @akiraveliara?

Having it as the default would also bring in to question what the default should in that case

but wouldn't the cached users in DiscordClient be a good candidate? If you have a concurrent dictionary of TCS instead of just DiscordUser, you spare the end-user from wasting multiple API requests for the same user ID when the method is called concurrently.

TCS carries a Task object with it, which is a rather heavy cost to incur when it comes to scaling.

Request coalescing is something that's more apt to be handled at the higher-level anyway, such as in the ratelimit handler, and even then, is only beneficial if you're making LOTS of requests.

Moreover, only one request to a given endpoint (to my knowledge) can proceed at a time to ensure consistent and correct handler behavior, so subsequent requests to the same endpoint would (if enabled) pull from cache

[akiraveliara]

ScopePerEvent / ScopePerEventHandler / NoScope is sufficient configuration for my use-case.

that isn't happening, not because of library complexity (it's fairly trivial) but because it introduces a pitfall for users where changing a setting may silently break logic without being immediately obvious. we can let users shim the dispatcher if they want to customize scoping.

Moreover, only one request to a given endpoint (to my knowledge) can proceed at a time to ensure consistent and correct handler behavior,

this is incorrect, requests are by design not synchronized - this would result in very bad caching code and in weird behaviour where a request can sit around queued for a while before pulling from cache. we can try to coalesce near-simultaneous requests, but that's both complex and very limited in what we could do.

If the application developer doesn't care/know anything about scope, DSharpPlus is free to decide what Scoped means for their application.

how do we know they don't know, under the condition of "no configuration setting"?

[moorehousew]

that isn't happening, not because of library complexity (it's fairly trivial) but because it introduces a pitfall for users where changing a setting may silently break logic without being immediately obvious. we can let users shim the dispatcher if they want to customize scoping.

That's acceptable. Shimming the dispatcher should work fine for just about everybody if we can code something functionally equivalent to ScopePerEvent, ScopePerEventHandler or NoScope ourselves.

how do we know they don't know, under the condition of "no configuration setting"?

You don't really know what they want/need, which is the underlying point I was trying to make- but you could assume that if they haven't shimmed the dispatcher, they'll take whatever it is you're serving without complaint. As it is with all other defaults.