new async events

[abstract flint]

https://github.com/DSharpPlus/DSharpPlus/issues/1477

hans tag use pr artifacts

[haughty folio]

To run the library off a pull request, you will need a GitHub account. Navigate to the pull request you want to use and click on the green check mark next to the latest commit: https://cdn.discordapp.com/attachments/379379415475552276/1075485948281888788/image.png

Then, go to "Details" and scroll all the way down to "Artifacts". Download the zip file and extract it into a folder of your choice.

Now, you need to add the packages to your project.

Using CLI:

Run dotnet add package <package-name> --source "path/to/your/package/folder"

Using Visual Studio:

Add your local folder as a package source in this dialog, reached through the little gear icon in your NuGet dialogue, see https://cdn.discordapp.com/attachments/959804202744422453/962070642432311366/unknown.png and https://cdn.discordapp.com/attachments/959804202744422453/962070737148051536/unknown.png. You'll want to click the plus icon to create a new package source, then enter your folder name at "Source".

Using your .csproj file:

(do not.)
Add <RestoreAdditionalPackageSources>path/to/your/package/folder</RestoreAdditionalPackageSources> to your head PropertyGroup.

In the latter two cases, you should then be able to update D#+ to the pull request build through your NuGet dialogue.

[abstract flint]

This PR aims to replace the current sequential async events with parallel async events. please as usual report any issues you find below

[deep sierra]

hopping in

[dusty rampart]

I thought sequential event handling was a discord api limitation, how is this handled differently now?

[abstract flint]

its... not?

[dusty rampart]

I had been told this much

[deep sierra]

it never has been a limitation

[abstract flint]

that's just not correct

[deep sierra]

it's a limitation of emzi's event handling system :^)

[dusty rampart]

oh well, considered me been lied to then

[abstract flint]

in fact, D#+ used to handle events in parallel before using emzis common lib

[deep sierra]

dang

it's not even that complex to make them offload to tasks

[abstract flint]

well

you cant do .Handled with offloading them

the reason i found while digging around originally was to ensure cache worked

but cache is done before the event is dispatched anyways

[deep sierra]

that does make sense actually

[abstract flint]

i don't know whether it was an actual issue back then, but it theoretically shouldn't be today

theoretically

[deep sierra]

that's what this thread is for

[abstract flint]

exactly

[onyx igloo]

that's…quite a breaking change

so, now if I had some anti-spam and anti-piracy filters first, now I have to roll my own chain system

have you thought about making this opt-in new system instead of replacing the existing one

[abstract flint]

firstly any toggle would be opt-out, not opt-in

secondly, event sequentiality was always more of an impl detail that caused issues for people, not something we expressly wanted (as mentioned, it also wasn't always a thing)

if this causes too many issues i'll consider making a config toggle for it

[onyx igloo]

it was documented, and it is a public api, whether you like it or not, and people rely on this

[abstract flint]

it was documented, and it is a public API, but it's also a common source of issues and we're okay with taking breaking changes if the situation merits it

as for "people rely on this", i personally doubt it's a very common issue, but feel free to prove me wrong

[abstract flint]

oh yeah @idle willow if you find any issues

[deep sierra]

I can't imagine in what situation you'd rely on events arriving in sequence

[idle willow]

Voice

[deep sierra]

solid point

though even them, wouldn't discord send such data sequentially from their end anyway

[idle willow]

However the voice received event has a sequence property, so the user should create a voice state manager and order the events through that

[deep sierra]

aha

[idle willow]

Also yes

[deep sierra]

I don't think (no offense) that spam prevention is really something that has to be in sequence

we could always poll for it

[idle willow]

Discord API limitation is... a stretch. Assuming it was Emzi who said this, what he probably meant was that we can only receive events from each shard sequentially because each gateway event is a websocket message. However this doesn't mean that we have to wait until the event handlers are done executing before accepting the next gateway event, which is what we're currently doing

Also sharding

[deep sierra]

if you really must rely on finishing events before calling a new handler: semaphore or lock

[idle willow]

So this PR will simply receive the event, fire the event handlers, NOT wait for it to finish and move onto the next event

(I think anyways)

[deep sierra]

if you're smart enough to think of a reason events should wait for each other, you're probably also smart enough to know how the Semaphore works (lighthearted)

[idle willow]

State machines is an important concept that others should learn (myself included)

[deep sierra]

fairly sure that's what this was about. for example: right now long running event handlers throw when they exceed x seconds

[idle willow]

Yup

[deep sierra]

why? like state machines are not that complex but still why is it relevant? :3

[idle willow]

I meant thread safety, however state machines tie in with this too, I think

A thread safe state machine I guess?

It sounded better in my head

[idle willow]

For ordering messages I mean

[deep sierra]

switch case there's your state machine

get a State enum that you switch whenever state changes :^^^)

[idle willow]

It's a valid option tbf

[deep sierra]

from a very very global perspective, that is essentially how state machines operate

they just switch out their behavior when state changes

unless I am thinking of the wrong design pattern

no no it's correct

I generally use this site whenever I need to refresh my design pattern knowledge :^) https://refactoring.guru/design-patterns/csharp

[abstract flint]

arguably the voice gateway shouldnt use async events at all

it's less of a "hey something interesting happened, have a payload" and more of a constant stream of data that is being piped into interop code

[abstract flint]

sure

[idle willow]

Yeah, we should be using pipes or Streams instead of events for voice

Wait isn't that what we currently do

We have voice events for user mute, deafened, etc

But the actual voice data should be a Stream

[abstract flint]

well it should be a pipeline

in an ideal world

but yes

[abstract flint]

pretty much

but itll also fire multiple event handlers at once

previously itd only fire one at a time

[dusty rampart]

yes ty

[idle willow]

Time to debug

Aha

Cc @noble grove

Oh my God Discord

Sorry for the wrong ping

[idle willow]

Cc @abstract flint

Yeah I'm gonna be honest I have no idea why this doesn't work lol

Which handler is releasing the lock...

What

None of the following are null

[abstract flint]

whot

[abstract flint]

exceptionhandler is probably null here

which can happen, come to think of it, in some places we just dont register it

idk what the fuck that lock is about, though

[abstract flint]

actually i take it back, it can't

[abstract flint]

so i was right

HOWEVER

i guess it could be set to null

bitch

[idle willow]

Lemme know when you push and I'll pull

[abstract flint]

that was an easy fix but

the silly lock issue is still there

ok @idle willow i can no longer repro

commit pushed

[idle willow]

[2023-03-14 14:43:11.487 -05:00] [EROR] DSharpPlus.BaseDiscordClient: Event handler exception for event READY thrown from System.Threading.Tasks.Task DiscordClient_ReadyAsync(DSharpPlus.DiscordClient, DSharpPlus.EventArgs.ReadyEventArgs) (defined in DSharpPlus.CommandAll.CommandAllExtension)
System.Threading.LockRecursionException: Write lock may not be acquired with read lock held. This pattern is prone to deadlocks. Please ensure that read locks are released before taking a write lock. If an upgrade is necessary, use an upgrade lock in place of the read lock.
   at System.Threading.ReaderWriterLockSlim.TryEnterWriteLockCore(TimeoutTracker timeout)
   at System.Threading.ReaderWriterLockSlim.EnterWriteLock()
   at DSharpPlus.AsyncEvents.AsyncEvent`2.Unregister(AsyncEventHandler`2 handler) in /home/lunar/Code/Lunar/Tomoe/libs/DSharpPlus.CommandAll/libs/DSharpPlus.AsyncEvents/DSharpPlus/AsyncEvents/AsyncEvent`2.cs:line 81
   at DSharpPlus.DiscordClient.remove_Ready(AsyncEventHandler`2 value) in /home/lunar/Code/Lunar/Tomoe/libs/DSharpPlus.CommandAll/libs/DSharpPlus.AsyncEvents/DSharpPlus/Clients/DiscordClient.Events.cs:line 82
   at DSharpPlus.CommandAll.CommandAllExtension.DiscordClient_ReadyAsync(DiscordClient sender, ReadyEventArgs eventArgs) in /home/lunar/Code/Lunar/Tomoe/libs/DSharpPlus.CommandAll/src/CommandAllExtension.cs:line 156
   at DSharpPlus.AsyncEvents.AsyncEvent`2.<>c__DisplayClass7_0.<<InvokeAsync>b__0>d.MoveNext() in /home/lunar/Code/Lunar/Tomoe/libs/DSharpPlus.CommandAll/libs/DSharpPlus.AsyncEvents/DSharpPlus/AsyncEvents/AsyncEvent`2.cs:line 116

[abstract flint]

whot

ok firstly that should absolutely not occur if you dont register events post startup

[idle willow]

All events are registered before DiscordShardedClient.StartAsync is called

[abstract flint]

wild

uuh let me think

okay i dont understand how this is happening but you know

WS_ERROR throwing unconditionally is also not something i understand

@idle willow c:

[idle willow]

This would deadlock if an event handler invoked another event, wouldn't it?

Oh

I see how

this._lock is locked because it's executing the ready event. Because the ready event is calling the unregister method, and the method waits until the _lock is free...

Well instead of locking and then iterating, why don't we lock, copy, unlock and then iterate over the copy

@abstract flint

public async Task InvokeAsync(TSender sender, TArgs args)
{
    if (this._handlers.Count == 0)
        return;

    await this._lock.WaitAsync();
    List<AsyncEventHandler<TSender, TArgs>> copiedHandlers = new(this._handlers);
    this._lock.Release();

    try
    {
        await Task.WhenAll(copiedHandlers.Select(async (handler) =>
        {
            try
            {
                await handler(sender, args);
            }
            catch (Exception ex)
            {
                this._exceptionHandler?.Invoke(this, ex, handler, sender, args);
            }
        }));
    }
    finally
    {
        this._lock.Release();
    }

    return;
}

It works now

[abstract flint]

because thats two iterations

[abstract flint]

i guess we could uh

public async Task InvokeAsync(TSender sender, TArgs args)
{
    if (this._handlers.Count == 0)
        return;

    await this._lock.WaitAsync();

    var tasks = this._handlers.Select(async (handler) =>
    {
        try
        {
            await handler(sender, args);
        }
        catch (Exception ex)
        {
            this._exceptionHandler?.Invoke(this, ex, handler, sender, args);
        }
    });

    this._lock.Release();

    await Task.WhenAll(tasks);

    return;
}```

actually i just realized @idle willow how your code deadlocked on the old impl

when trying to remove the event in your handler you were waiting for a lock that was still locking on the handler

well, commit pushed

let me know if the sharded client explodes again

[idle willow]

Yeah that's what I was trying to say here

Poorly explained, but we still got the problem solved

Thanks Aki

[abstract flint]

oooh i see

[idle willow]

Interesting

[abstract flint]

oh wait

late bound enumeration?

i think what's happening here is that the Select is only actually evaluated after the lock is released

uuh try adding a .ToArray call, that should enumerate it, and see whether it reproduces

if only the remove method didnt exist, we could just use a ConcurrentBag and be done with it

and if only i wasn't at uni and could actually do things myself

[idle willow]

ConcurrentBag doesn't exist iirc

[abstract flint]

it does

[idle willow]

Wait no it does

Why aren't we using it then lol

[abstract flint]

because you cant remove from it

[idle willow]

oh

[abstract flint]

and yeeting event handler removing is a bit too much of a breaking change imo

[idle willow]

Deadlocks

Which is... weird

[abstract flint]

where are you ToArray-ing?

[idle willow]

After the select

var tasks = this._handlers.Select(async (handler) =>
{
    try
    {
        await handler(sender, args);
    }
    catch (Exception ex)
    {
        this._exceptionHandler?.Invoke(this, ex, handler, sender, args);
    }
}).ToArray();

[abstract flint]

oh uh

ToArray calls a different overload

try ToList then

[idle willow]

Same behavior lol

[abstract flint]

could you try replacing it with a manual foreach, then?

[idle willow]

Interestingly, it still deadlocks even when I put it into a new list

[abstract flint]

the remove handler is something alright

[idle willow]

I'm just gonna go back to my solution lol

[abstract flint]

double enumeration 😭

[idle willow]

It's fine if it iterates twice, I highly doubt there will be a lot of event handlers registered anyways

[abstract flint]

that is a fair point

yeah feel free to push to my fork

[idle willow]

Aye

Done