Enhance event handling system including waitForEvents.

ire4ever1190 commented 1 year ago

Generic event awaiting system that stems from the discussions in #106. One big change is that we don't store Future in the wait table so that we have more flexibility in what we want to return to the user. We also don't run inside onDispatch so that the cache and everything is properly up-to-date

System is built via WaitHandlers which are stored in a WaitTable which is in the client object. The WaitTable is a mapping of event type (e.g. "MESSAGE_CREATE", "MESSAGE_DELETE) to a list of handlers which are awaiting that event. I originally had a mapping of object ID's to list of handlers but then I ran into a problem where if we have an object 1234, how do we know if its getting deleted or reacted to? (If there is a way then I can change it back, which would be nicer since it means we aren't running every handler when we only care about a few).

The table could also be Table[string, Table[string, seq[WaitHandler]]] where the first key is the event, and second is the object ID.

I use inheritance so that the handlers can get the actual value and don't need to parse it themselves, we just need to downcast within the object. This could be implemented with raw pointers and casting instead of inheritance, but imo that is very unclean and could cause problems

This could work for ON_DISPATCH also by making a type like so

type 
  RawEventData = ref object of DimscordObject
    data: JsonNode

Example: User

In this example we wait for a message to be deleted and then message that it has been deleted. When that message we sent is replied to, we just parrot back what the user said

proc messageCreate(s: Shard, m: Message) {.event(discord).} =
  # First wait for the message sent to be deleted
  let delMsgFut = discord.waitForDeletion(m)
  # This is how timeouts would be handled
  if not await delMsgFut.withTimeout(10 * 1000): # Give them 10 seconds
    return
  # Send a message back and then wait for a response
  let msg = await discord.api.sendMessage(m.channelID, "Message was deleted")
  let replyFut = discord.waitForReply(msg)
  if not await replyFut.withTimeout(10 * 1000): # 10 seconds again
    return
  # Didn't timeout, so we can now await the reply and get the message sent
  let response = await replyFut
  discard await discord.api.sendMessage(m.channelID, fmt"'{response.content}'")

# Connect to Discord and run the bot.
waitFor discord.startSession()

They can also use waitFor and waitForObject to easily write their own

# Wait for a specific person to respond
await discord.waitFor("MESSAGE_CREATE") do (m: Message) -> bool:
    m.author.id == "Some person ID"
# Wait for a specific person to respond, and return the object
let msg = await discord.waitForObject("MESSAGE_CREATE") do (m: Message) -> bool:
    m.author.id == "Some person ID"

Example: Developer

This is how the handlers need to be implemented

proc waitForReply*(client: DiscordClient, to: Message): Future[Message] =
  ## Waits for a message to reply to a message
  # We make a future which is returned immediately.
  # This future is then completed later on once our handler passes
  let createFuture = newFuture[Message]("waitForReply")
  result = createFuture
  # We register a handler with the client.
  # This captures the future we made and completes it if our conditions hold
  client.addHandler("MESSAGE_CREATE") do (m: DimscordObject) -> bool:
    # Remove handler if future is completed (probably timedout)
    if createFuture.finished(): return true
    # We need to downcast to get the actual values.
    # Should be safe to use without `if m of Message` since it only runs for a certain event
    let newMsg = Message(m)
    # Now we do our checks
    if newMsg.referencedMessage.isSome():
      let referenced = newMsg.referencedMessage.unsafeGet()
      if referenced.id == to.id:
        # If everything is fine then we complete with the information we want to pass back to the user
        createFuture.complete(newMsg)
        # We also need to return true to tell the wait table to stop waiting for the event
        return true

TODO

[x] Find out if it works with shards. Might need to make it a reference table that gets shared between shards
[x] Add procs for more events (pinned, reaction added, etc)
[x] ~~Add timeouts. Maybe return Option[T] instead which is none if the await timed out. We could also just show in the docs that the user should use withTimeout in their code.~~: Sticking with just leading the user handle timeouts themselves with withTimeout
[x] Add checkIfAwaiting to all dispatch locations so that the system works for all events
[x] Add proper parameter names to dispatch. Not a breaking change but will make documentation clearer. Also makes it easier to work with since those are the names used for the data tuple

ire4ever1190 commented 1 year ago

The boilerplate of downcasting + handling already completed futures can easily be fixed by a small macro or template

ire4ever1190 commented 1 year ago

Also note that this is just to test the design. More than happy to rewrite/close it depending on how the discussion goes

nayr7 commented 1 year ago

Nice ! A couple of comments:

One big change is that we don't store Future in the wait table so that we have more flexibility in what we want to return to the user.

Interesting, this makes sense for purpose-built procs like waitForDeletion etc, but what about truly generic wait handlers ? Do we just return the raw JsonNode ? (which is fine btw imo).

We also don't run inside onDispatch so that the cache and everything is properly up-to-date

I think we really could, it seems like a easy bug to fix afaik, feel free to correct me but it should be a matter of swapping the order of execution of on_dispatch and handleEventDispatch (currently on_dispatch runs first, handleEventDispatch is where cache updating happens).

originally had a mapping of object ID's to list of handlers but then I ran into a problem where if we have an object 1234, how do we know if its getting deleted or reacted to?

A more reliable way would be to take author_id, guild_id and channel_id if they are available in the object (Message for example) and store them inside WaitHandler. Then try to see if the event being dispatched match them. But to take advantage of Table you should "hash" them together instead of iterating through every waits to see if the event match the fields. Just "hash" them again when there is a dispatch and check if the key is inside, in Nim this is a O(1) operation.

The table could also be Table[string, Table[string, WaitHandler]] where the first key is the event, and second is the object ID.

Proposal: Make it array[EventKind, Table[string, WaitHandler]] where EventKind is an enum mapping every event kind. array is the length of EventKind if that make sense... string could be an "hash" like I said earlier.

This could be implemented with raw pointers and casting instead of inheritance, but imo that is very unclean and could cause problems

Well you're right, but what happens if we generalize waiting for an event ? (which is what is going to happen unless we create a bazillion procs for every use-case)

let check = proc(m: Message): bool = m.content == "yes"
let myWait = waitBuilder(m, chec
let answer = discord.waitFor("MESSAGE_CREATE", myWait)
if not await answer.withTimeout(10 * 1000):
    discard await discord.api.sendMessage(m.channel_id, "You haven't replied on time!")
# do stuff ...

The example is kinda funky and could be better but I hope you get the point, check could be any proc signature, maybe with object variants ?

Timeout could also be implemented internally by having the user pass the time duration but this is fine this way too.

ire4ever1190 commented 1 year ago

Interesting, this makes sense for purpose-built procs like waitForDeletion etc, but what about truly generic wait handlers ? Do we just return the raw JsonNode ? (which is fine btw imo).

Well it is truely generic since any type can be returned (even JsonNode). The handler can use a Future for any type it wants

I think we really could, it seems like a easy bug to fix afaik, feel free to correct me but it should be a matter of swapping the order of execution of on_dispatch and handleEventDispatch (currently on_dispatch runs first, handleEventDispatch is where cache updating happens).

Should just be a matter of that yeah. But this requires the object to be parsed (So that we don't need to deal with raw JsonNode) so doing it in the event dispatch means the parsing doesn't need to be done in multiple places. on_dispatch is a user event anyways so we would need to place a checkIfAwaiting call after it to mimic the same effect

A more reliable way would be to take author_id, guild_id and channel_id if they are available in the object (Message for example) and store them inside WaitHandler. Then try to see if the event being dispatched match them. But to take advantage of Table you should "hash" them together instead of iterating through every waits to see if the event match the fields. Just "hash" them again when there is a dispatch and check if the key is inside, in Nim this is a O(1) operation.

Would it not be better to just use the ID of the message? Hashing author, guild, and channel together just seems like more effort when the ID of the object is already unique. We will still need to iterate through waits anyways since for example a single message might be waiting on both a response and a reaction (I made a mistake in the description with my second WaitTable idea, will fix that up). Also tables in Nim are O(n) worst case iirc but I might be wrong.

I still do plan to do some sort of ID mapping though since the waits list probably will get largish for big bots, just thinking it might not work well for ON_DISPATCH handlers

Proposal: Make it array[EventKind, Table[string, WaitHandler]] where EventKind is an enum mapping every event kind. array is the length of EventKind if that make sense... string could be an "hash" like I said earlier.

Yeah was planning that but since EventKind doesn't exist yet I was planning to make another PR to switch dispatch over to an enum and then use that here :+1:. One issue with having a second table is that it then won't work for ON_DISPATCH since if the user is wanting to check raw data, what should we use for the second key? (Unless we just use "" as the key for ON_DISPATCH)

Well you're right, but what happens if we generalize waiting for an event ? (which is what is going to happen unless we create a bazillion procs for every use-case) The example is kinda funky and could be better but I hope you get the point, check could be any proc signature, maybe with object variants ?

Very possible with the current system. I have this proc locally (Will push in a bit, was just testing different things)

proc waitFor*[T: DimscordObject](client: DiscordClient, event: string,
                                 handler: proc (obj: T): bool): Future[void] =
  ## Generic waiting proc that allows you to write your own condition to wait for
  # Implemented like most others
  let fut = newFuture[void]("waitFor")
  result = fut
  client.addHandler(event) do (obj: DimscordObject) -> bool:
    if fut.finished(): return true
    if obj of T: # Just for safety reasons, shouldn't really be needed
      # Except we automatically handle down casting and handling the future
      if handler(T(obj)):
        fut.complete()
        return true

Then your example can be written like

let check = proc(m: Message): bool = m.content == "yes"
let answer = discord.waitFor("MESSAGE_CREATE", check)
if not await answer.withTimeout(10 * 1000):
    discard await discord.api.sendMessage(m.channel_id, "You haven't replied on time!")

The argument still needs to correspond with the event, but this allows someone to wait for any condition to pass. I also made a version called waitForObject that returns a Future[T] instead if the user wants to get the object that passed the condition

With those two procs it will then be easy to write a series of helpers so that users aren't constantly rewriting common handlers. e.g. waitForDeletion is now

proc waitForDeletion*(client: DiscordClient, msg: Message): Future[void] =
  ## Waits for a message to be deleted
  client.waitFor("MESSAGE_DELETE") do (m: Message) -> bool:
    m.id == msg.id

ire4ever1190 commented 1 year ago

Will work on switching dispatch to enum now. Since it will be nice to get type safety instead of using strings for the events

Now based on #111 so that I'm not using strings for events anymore :+1:

nayr7 commented 1 year ago

Would it not be better to just use the ID of the message?

This wouldn't work in some case, what if you're waiting for a reply but the replier doesn't ping you back and you have nothing in message_reference as a consequence ? You're also very likely to be checking against author_id anyways, it's such a common thing to do that hashing would save the trouble of writing if m.author.id == user.id everytime imo.

... Hashing author, guild, and channel together just seems like more effort when the ID of the object is already unique.

Yes it's more effort but I don't understand what the "Object ID" is, all i've found that sounds like it is that it's the object address in memory, surely not what you want then. Whatever it is, you'll need to have it back when you're going to be checking the waits if you're not iterating, if you are then maybe seq would be better instead ? It all depends on whether iterating > hashing or not. Besides, the "hashing" in question is not "real"

One issue with having a second table is that it then won't work for ON_DISPATCH since if the user is wanting to check raw data, what should we use for the second key? (Unless we just use "" as the key for ON_DISPATCH)

Hmmm. On second thought, maybe it's going to be a bad idea to let users wait for on_dispatch. I mean, they will have to be checking if not data["kind"].getStr == "DISCORD_EVENT": discard", or else this will blow up and result in a hard to find bug (mainly because the error messages are bad). Besides, you'll be checking for every dispatched event, sounds very inefficient and I don't think other libraries do this.

Also tables in Nim are O(n) worst case iirc but I might be wrong.

Surprisingly I found little data on the performance of the implementation of Tables in Nim other than the general dict/hashtable performance profile :(

Very possible with the current system. I have this proc locally (Will push in a bit, was just testing different things) ... handler: proc (obj: T): bool

Well, the issue i was thinking about is that most events handlers are not unary (if you exclude Shard), it's not an easy problem to solve and you will need some form of type erasure to deal with this. This is if you don't want to impose unary checks, it's perfectly fine if you and krips do (i think it's a shame though).

ire4ever1190 commented 1 year ago

Certainly wont work for everything, I'll need to check what to use has an ID before going ahead on implementing an inner table. e.g.

MessageDelete can definitely just be Message.id
MessageCreate could be the Channel.id

You're also very likely to be checking against author_id anyways, it's such a common thing to do that hashing would save the trouble of writing if m.author.id == user.id everytime imo.

So when you mean hashing the strings together you mean joining them and then hashing? Even then I don't see the use in doing that just to remove an if statement.

Yes it's more effort but I don't understand what the "Object ID" is, all i've found that sounds like it is that it's the object address in memory, surely not what you want then

I mean that most objects already have an ID attached to them (e.g. Guild, Message, User, etc all have an id field to identify them)

It all depends on whether iterating > hashing or not.

Hashing or no, I'm also going to need to iterate anyways. The hash table won't remove iterating but just partition the handlers so the iterating is minimised at times

sounds very inefficient and I don't think other libraries do this.

:+1:

Well, the issue i was thinking about is that most events handlers are not unary (if you exclude Shard), it's not an easy problem to solve and you will need some form of type erasure to deal with this. This is if you don't want to impose unary checks, it's perfectly fine if you and krips do (i think it's a shame though).

Already works fine, just a matter of passing an object that contains the fields of the event handler e.g. for GuildMemberUpdate

type
   GuildMemberUpdateData = object of DimscordObject
     guild: Guild
     member: Member
     old: Option[Member]

This was the one thing that made be consider pointer since it would mean would could just use a tuple and pass a pointer to that then cast it back. But imo the shenanigans that could arise from that just doesn't feel worth it

nayr7 commented 1 year ago

Heya @ire4ever1190 , I experimented with your PR a little bit and I think I have some ideas to improve it:

Using waitFor as it currently is limits the capabilities of the event awaiting system because a user may expect to match an event against different type depending on the event in question (for now i'll try to focus on waitFor and not on waitForObject) :

 ## testbot wants to ban a member and waits for your confirmation !
 client.waitFor(MessageReactionAdd) do (m: Message, u: User, e: Emoji) -> bool:
   if (m.id == prompt.id) and (u.id == mod.id):
     # we just check if the mod actually responded, the bot would check from the returning data what emoji the mod reacted with and proceed accordingly. (i know `waitFor` returns `Future[void]` and we should use `waitForObject`  instead for that...)
     if (get(e.name) == "✅") or (get(e.name) == "❎"): return true

#[
currently fails with:
Error: type mismatch
Expression: waitFor(client, MessageReactionAdd, proc (m: Message; u: User;
    e: Emoji): bool =
  if m.id == and u.id == :
    if get(e.name) == "✅" or get(e.name) == "❎":
      return true
  )
  [1] client: DiscordClient
  [2] MessageReactionAdd: DispatchEvent
  [3] proc (m: Message; u: User; e: Emoji): bool =
  if m.id == and u.id == :
    if get(e.name) == "✅" or get(e.name) == "❎":
      return true: proc (m: Message, u: User, e: Emoji): bool{.noSideEffect, gcsafe.}

Expected one of (first mismatch at [position]):
[3] proc waitFor[T: DimscordObject](client: DiscordClient; event: DispatchEvent;
                                handler: proc (obj: T): bool): Future[void]
]#

Many such cases like this one so i think this is a pretty big limitation, this should be solved by using the auto generic to infer the type passed to the handler field but requires some changes here and there on your code:

type WaitHandler* = proc(){.closure.}

proc waitFor*(client: DiscordClient, event: DispatchEvent, handler: auto): Future[void] =
  let fut = newFuture[void]("waitFor(" & $event & ")")
  result = fut
  client.waits[event].add cast[proc(){.closure.}](handler)
  ...

Because we're casting to nullary typed proc, we should be able to fit in our wait handlers without using pointers, but casting it back is not trivial but we can work around that using macros to recover the type information. This way it should be possible to get the type info from client.events.someEvent, where someEvent is the event name injected by the macro sort of like how the event macro pragma does but instead of the proc name, we just use the DispatchEvent enum, it should look like this:

macro `**`(args: untyped): untyped = 
  # used to apply a tuple values as individual proc arguments
  ...

macro getEventType(client: DiscordClient, eventKind: DispatchEvent): untyped =
  # gives out the typedesc of `client.events.'eventKind' without `Shard`
  ...

proc checkIfAwaiting[T: tuple or object](client: DiscordClient, event: DispatchEvent, data: T) =
  var handlers = addr client.waits[event]
  let evt_type = client.getEventType(event)

  for i in countdown(handlers[].len - 1, 0):
    # casting to the type obtained by `getEventType`
    if cast[evt_type](handlers[i])(**data): # since `data` can be a tuple, we should use a macro to unpack and spread its value as arguments.
      handlers[].del(i)

Im not sure how but we should also remove Shard from the type information we get since we don't actually use it with waitFor and would then make us cast to the wrong type... checkIfAwaiting would then be used like this when an event is dispatched:

# dispatch.nim
import sugar

proc messageReactionAdd(s: Shard, data: JsonNode) {.async.} =
  ## some code blablabla
  s.client.checkIfAwaiting(MessageReactionAdd, (msg, user, emoji, exists))
  asyncCheck s.client.events.message_reaction_add(s, msg, user, emoji, exists)

Doing this should give us enough flexibility to have a really powerful event awaiting system without even using pointers, cast is unsafe but macros take care of getting the right type back. One big issue arising from this would be users who would waitFor using the wrong types. This is solved by having documentation or emitting clear error messages (harder than it sounds). Not sure what WaitForObject should return, most probably a tuple.

Note that this does not rely on inheritance so DimscordObject would be seldom used, if ever. This means minimal breaking changes 👍🏾 .

ire4ever1190 commented 1 year ago

Looks good 👍 will start tinkering with that.

I should be able to check that the proc passed is valid, so think users shouldn't run into any problems

ire4ever1190 commented 1 year ago

Bit messy but got initial version of that. Had to do some weird casts to stop cgen issues with the closures, will do more testing but seems alright.

Though might see if just a pointer to a tuple of arguments is easier :thinking: though that might cause issues if the event handlers need to become async

ire4ever1190 commented 1 year ago

Actually thinking more about it I don't think the pointer to tuple will have problems.

Will probably just restrict users to the waitFor/waitForObject api though since it will be easy to shoot themselves in the foot with it

krisppurg commented 1 year ago

I think waitForEvent with voice would also be useful along voice event pragma.

ire4ever1190 commented 1 year ago

Yeah had a few plans for voice stuff :+1: I'll probably leave all the event pragma stuff for a followup PR though

ire4ever1190 commented 1 year ago

Does anyone have a list of gateway events and intents required for them? Think it would be handy to add asserts to make sure that the client has the correct events enabled (~~Plan to put this in the base waitForObject helper so that it works for everything~~ nvm, looking at the list it seems it will need to be a case by case basis depending on what I need)

Implemented waitForReaction and while testing I wasn't getting anything until I realised I needed to add the giGuildMessageReactions intent

ire4ever1190 commented 1 year ago

nvm, found the list here :+1: https://discord.com/developers/docs/topics/gateway#list-of-intents

nayr7 commented 1 year ago

Does anyone have a list of gateway events and intents required for them? Think it would be handy to add asserts to make sure that the client has the correct events enabled (~~Plan to put this in the base waitForObject helper so that it works for everything~~ nvm, looking at the list it seems it will need to be a case by case basis depending on what I need)

Implemented waitForReaction and while testing I wasn't getting anything until I realised I needed to add the giGuildMessageReactions intent

I've had this idea of making a PR to add asserts everywhere intents are needed, too many footguns out there, I'll do it soon

nayr7 commented 1 year ago

For raw/unknown events, assuming we're using JsonNode there is an important design choice we need to make: Should we store the event "type" in the closure environment or outside of it ?

inside the environment

pros:

case match the event type from inside the closure like how you would do it with on_dispatch, this mean we can wait for multiple events from within a single wait which is super powerful

cons:

case matching multiple events in this case is actually a giant shotgun, how do you actually know which event got matched ? what kind of fields the JsonNode contains ?
the closure will be called at every dispatch, since you cannot determine which event the closure actually expects from outside it's environment.

proposed syntax:

 client.waitFor(Unknown) do (event_kind: string, data: JsonNode) -> bool:
    ...

outside the environment

pros:

scales better: you dont need to iterate through every raw waits at every dispatch, just the ones that matter if you use Table
safer: you know what kind of data is returned inside JsonNode

cons:

requires a second raw_waits data structure
not as powerful (you can use an helper proc to wait for multiple events at once if needed)
requires more code to write

proposed syntax:

 client.waitFor("EVENT_KIND") do (data: JsonNode) -> bool:
    ...

ire4ever1190 commented 1 year ago

Think first proposal works better. User probably won't be using it often anyways. Only times I ever used onDispatch were to implement waiting for events anyways

krisppurg commented 1 year ago

I would prefer outside the environment due to one of the reasons.

EVENT_KIND is would be any event that is being received whether it be MESSAGE_CREATE, etc. This would be useful if the user wants to waitfor any raw event before dimscord handles it. Would also be helpful with unknown events, which we would kill 2 birds with one stone.
If the user would use the first proposal (inside the environment), the user would only just use it for just unknown events that are not implemented into the lib yet and if the user wants to wait for raw message creates or other events that are implemented before dimscord handles it, they'd be unable to do so.

ire4ever1190 commented 1 year ago

I'd probably place the call to check the handlers here https://github.com/krisppurg/dimscord/blob/c1dae26ada05d01c71fdece66addca2f254df557/dimscord/gateway.nim#L298 So it would work for both events that dimscord hasn't implemented along with running before dimscord handles the event.

krisppurg commented 1 year ago

But if the user would want to use MessageCreate, it would be after dimscord handles them and if the user would want to use "MESSAGE_CREATE", it would be before dimscord handles them.

ire4ever1190 commented 1 year ago

client.waitFor(Unknown) do (event_kind: string, data: JsonNode) -> bool:
  if event_kind == "MESSAGE_CREATE": 
    # ...

Would run before dimscord handles it (When I make Unknown get ran in that place I mentioned, currently they don't run at all)

krisppurg commented 1 year ago

client.waitFor(Unknown) do (event_kind: string, data: JsonNode) -> bool:
  if event_kind == "MESSAGE_CREATE": 
    # ... 
Would run before dimscord handles it (When I make Unknown get ran in that place I mentioned, currently they don't run at

hm well for that use-case Unknown should be called Any, since it's a bit misleading.

ire4ever1190 commented 1 year ago

Think the name is fine. Its a bit of a stretch, but you can consider it that its "unknown" what event you want to wait for.

krisppurg commented 1 year ago

Hm, I guess so then.

nayr7 commented 1 year ago

For what is worth, I don't agree that the event we're waiting for is unknown. The user do (and should) know what event they're going to be waiting for as demonstrated by checking if event_kind == "EVENT_KIND", or else you're probably misusing the feature by doing something really weird. It's subjective but it's why I favor "outside the environment" approach because it cognitively make sense to me, why the waitFor(Unknown) when the event is actually not Unknown but just "Unsupported" ? Why not make the check unnecessary in the first place then? This is also how discordpy/pycord and maybe some other libraries do it.

ire4ever1190 commented 1 year ago

Looks like discord.py just ignores the event.

I really don't want to add another table just to support a very niche feature, but the semantics of the second option can be done via

proc waitFor(client: DiscordClient, event: string, handler: proc (data: JsonNode): bool): Future[void] =
  client.waitFor(Unknown) do (eventKind: string, data: JsonNode) -> bool:
    if event == eventKind:
      return handler(data)

nayr7 commented 1 year ago

Looks like discord.py just ignores the event.

I really don't want to add another table just to support a very niche feature, but the semantics of the second option can be done via
proc waitFor(client: DiscordClient, event: string, handler: proc (data: JsonNode): bool): Future[void] =
  client.waitFor(Unknown) do (eventKind: string, data: JsonNode) -> bool:
    if event == eventKind:
      return handler(data)

That's best of both world imo :)

ire4ever1190 commented 1 year ago

Working on that

Delay is combination of uni + discovered an issue where it will use a different proc that has same name as the handler

ire4ever1190 commented 1 year ago

Ok fixed that issue

Just need to clean up and then it should be ready

Might be best to add any more helpers in future PRs if everyone is fine with the design of the system (so this can be merged and tested more)

krisppurg / dimscord