SinisterRectus / Discordia

Discord API library written in Lua for the Luvit runtime environment
MIT License
698 stars 143 forks source link

Fix issue with Message.lua causing bot to crash #290

Closed mrparkerlol closed 3 years ago

mrparkerlol commented 3 years ago

This fixes an issue with my friend's bot where reaction is nil and causes the bot to crash. The check should be before the definition of reaction to fix the issue.

truemedian commented 3 years ago

This doesn't fix anything and coincidentally introduces a crash when reactions:get() returns nil, which the check you deemed useless checks for. _removeReaction is intended for internal use only and as such is not documented and should not be used. If your friend's code is calling this function specifically, they should not expect any help in debugging or fixing it.

mrparkerlol commented 3 years ago

This doesn't fix anything and coincidentally introduces a crash when reactions:get() returns nil, which the check you deemed useless checks for. _removeReaction is intended for internal use only and as such is not documented and should not be used. If your friend's code is calling this function specifically, they should not expect any help in debugging or fixing it.

It does not use this directly, the stack trace is entirely within Discordia when it crashes. Also, just moving that check should not introduce a crash when the line in which the error occurs on is before that check - specifically where reaction is defined. If the check is before, reaction is not indexed if it is nil, therefore doing what it should've been doing originally - which is the function returning nil if it is indeed nil.

mrparkerlol commented 3 years ago

Oh wait, I just realized my mistake. My bad, let me commit a fix. Apologies @truemedian, I will apply a fix.

truemedian commented 3 years ago

If they would like help fixing whatever issue they're having, posting the stack trace would be helpful to try and find the actual bug.

mrparkerlol commented 3 years ago

If they would like help fixing whatever issue they're having, posting the stack trace would be helpful to try and find the actual bug.

The issue with the code specifically is reactions is nil, causing an uncaught error. It has happened twice now in the past 24 hours with my friend's bot. The uncaught error involves trying to index 'reactions' when it is nil. The bot does not use anything internal to Discordia, so it isn't my friend's bot.

SinisterRectus commented 3 years ago

I'm not sure how it's possible for _reactions to be nil. As far as I can see, everywhere that _reactions is used in Discordia, there is a check for its existence.

mrparkerlol commented 3 years ago

I'm not sure how it's possible for _reactions to be nil. As far as I can see, everywhere that _reactions is used in Discordia, there is a check for its existence.

Here's the full stack trace, if I am reading this correctly it is in Discordia. I had to get it from the server, sorry for not posting it originally!

Uncaught Error: /bot/deps/discordia/libs/containers/Message.lua:128: attempt to index local 'reactions' (a nil value)
stack traceback:
    /bot/deps/discordia/libs/containers/Message.lua: in function '_removeReaction'
    /bot/deps/discordia/libs/client/EventHandler.lua:379: in function </bot/deps/discordia/libs/client/EventHandler.lua:374>
    /bot/deps/discordia/libs/client/Shard.lua:110: in function 'handlePayload'
    /bot/deps/discordia/libs/client/WebSocket.lua:50: in function </bot/deps/discordia/libs/client/WebSocket.lua:35>
stack traceback:
    [C]: in function 'error'
    /bot/deps/coro-channel.lua:16: in function 'assertResume'
    /bot/deps/coro-channel.lua:69: in function 'onPlain'
    /bot/deps/secure-socket/biowrap.lua:76: in function </bot/deps/secure-socket/biowrap.lua:61>
    [C]: in function 'run'
    [string "bundle:init.lua"]:52: in function <[string "bundle:init.lua"]:47>
    [C]: in function 'xpcall'
    [string "bundle:init.lua"]:47: in function 'fn'
    [string "bundle:deps/require.lua"]:310: in function <[string "bundle:deps/require.lua"]:266>
truemedian commented 3 years ago

Looking back at the issue: I think what is happening is that for some reason they're receiving MESSAGE_REACTION_REMOVE on a message that initially had no reactions (so self._reactions is never initialized) and no MESSAGE_REACTION_ADD events were received between then and now. (so self._reactions never gets initialized).

Eventual consistency is probably part of the issue here.

SinisterRectus commented 3 years ago

Oh I do not have a nil check there. Hmm.

SinisterRectus commented 3 years ago

This doesn't fix anything and coincidentally introduces a crash when reactions:get() returns nil, which the check you deemed useless checks for.

I think the fix is fine. Why would it introduce a new crash?

truemedian commented 3 years ago

570d4b8 fixed the issue I was pointing out.

SinisterRectus commented 3 years ago

Oh okay.

SinisterRectus commented 3 years ago

Instead of:

local reactions = self._reactions

local emoji = d.emoji
local k = emoji.id ~= null and emoji.id or emoji.name
local reaction = reactions and reactions:get(k) or nil

can you just make it:

local reactions = self._reactions
if not reactions then return nil end

local emoji = d.emoji
local k = emoji.id ~= null and emoji.id or emoji.name
local reaction = reactions:get(k)
SinisterRectus commented 3 years ago

I should add that a more robust fix for this would create a reaction object from the event payload, but there is a quirk in the API where the animated property for emojis is not provided on reaction remove, so it's probably better that we just go with this quick fix to prevent the crash.

mrparkerlol commented 3 years ago

Instead of:

local reactions = self._reactions

local emoji = d.emoji
local k = emoji.id ~= null and emoji.id or emoji.name
local reaction = reactions and reactions:get(k) or nil

can you just make it:

local reactions = self._reactions
if not reactions then return nil end

local emoji = d.emoji
local k = emoji.id ~= null and emoji.id or emoji.name
local reaction = reactions:get(k)

I applied the changes as requested, I also agree a more permanent fix needs to be found. The bot my friend made doesn't use any of the internal APIs as far as I am aware, however the bot is consistently crashing multiple times a day. In the past day alone, I've had to restart it more than 4 times.

SinisterRectus commented 3 years ago

I'm not sure what's going on with reactions right now. This is not the only bug that people are seeing. For example, see discord/discord-api-docs#2896 and discord/discord-api-docs#2910.

This quick patch will at least prevent bots from crashing, although they may miss the reaction remove events. Thank you for your contribution!

SinisterRectus commented 3 years ago

Looks like the problem was found: https://github.com/discord/discord-api-docs/issues/2896#issuecomment-843614515