v5 extensions and events design about dsharpplus

[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.

[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

[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.

[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.

[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.