cronokirby / alchemy

A discord library for Elixir
MIT License
151 stars 34 forks source link

Add on_role_update event handler #77

Closed curz46 closed 4 years ago

curz46 commented 4 years ago

This does not work correctly at the moment. old_role is very often nil, and it appears to be random whether or not the role is already cached. If it is nil at the start, it will always be nil, even though I am updating the guilds cache. Here's the test I'm using:

defmodule AlchemyTest do
  use Application
  use Alchemy.Cogs

  alias Alchemy.Client
  alias Alchemy.Events

  defmodule Events do
    use Alchemy.Events

    Alchemy.Events.on_role_update(:handler)
    def handler(old_role, new_role, guild_id) do
      IO.inspect old_role # nil like 70% of the time
      IO.inspect new_role
      IO.inspect guild_id
      IO.inspect "============="
    end
  end

  @spec start(any, any) :: {:ok, pid}
  def start(_type, _args) do
    run = Client.start(System.get_env("TOKEN"))

    use Events

    run
  end
end

Note: the above code also seems to receive an update for the @everyone role whenever any other role is updated, hopefully that's expected behaviour.

curz46 commented 4 years ago

Using IO.inspect Alchemy.Cache.Guilds.call(guild_id, :show) I've determined it's nil whenever the guild state is %{"id" => "437730559486328839", "unavailable" => true}. This is expected however the guild remains unavailable, as far as I can tell, forever.

curz46 commented 4 years ago

I believe there's a race condition with the Guilds cache being started and GUILD_CREATE being received.

curz46 commented 4 years ago

This fixes a bug that I can only assume will have existed in Alchemy for a long time, which makes the Guild cache useless about 70% of the time due to the READY event sometimes taking long enough for GUILD_CREATE to fire before it finishes. It's a bit of a hack, but it successfully forces READY's handler to occur before any other events. Credit: https://discordapp.com/channels/269508806759809042/269508806759809042/603280763357757443

cronokirby commented 4 years ago

Hmm, can you try and move these bug fixes to another PR? I'd like to merge/think about them independent of the feature request.

OvermindDL1 commented 4 years ago

I believe there's a race condition with the Guilds cache being started and GUILD_CREATE being received.

I think this explains a bug I've been seeing for years!

This fixes a bug that I can only assume will have existed in Alchemy for a long time, which makes the Guild cache useless about 70% of the time due to the READY event sometimes taking long enough for GUILD_CREATE to fire before it finishes.

Yes this! My bot just restarts repeatedly until it works as it needed that functionality... >.>

curz46 commented 4 years ago

@OvermindDL1 I created an issue regarding the bug here #79 if you're interested.

curz46 commented 4 years ago

@cronokirby is there a reason why you're holding back on merging this one? Asking because I have more changes to events lined up fixing channel caching. Edit: by the way, I think we should change all update handlers in event to give the old entity so that it's consistent and because it's very useful to have.