Skip to content

Fix MessageUpdated events in DMs throwing exception

Mateusz Brawański requested to merge github/fork/uwx/patch-68 into master

Created by: uwx

Summary

MessageUpdated events in a DM channel will trigger an exception. In previous versions this would be swallowed and ignored by the lib. As of the latest few commits the exception will be logged to console. This will not crash the client, but will cause the event to never fire and, in extreme cases, will slow down the client for a few seconds, due to all the logging.

Details

This is a sample exception body:

[2018-04-05 23:50:01 +00:00] [Websocket] [Error] Socket swallowed an exception: System.ArgumentNullException: Value cannot be null.
Parameter name: collection
at System.Collections.Generic.List`1..ctor(IEnumerable`1 collection)
at DSharpPlus.Entities.DiscordMessage..ctor(DiscordMessage other)
at DSharpPlus.DiscordClient.<OnMessageUpdateEventAsync>d__93.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
at System.Runtime.CompilerServices.TaskAwaiter.ValidateEnd(Task task)
at DSharpPlus.DiscordClient.<HandleDispatchAsync>d__69.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
at DSharpPlus.DiscordClient.<HandleSocketMessageAsync>d__68.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
at DSharpPlus.DiscordClient.<<InternalConnectAsync>g__SocketOnMessage|53_1>d.MoveNext() This is bad!!!!

The error occurs due to DiscordMessage._mentionedRoles and DiscordMessage._mentionedChannels being null in DM, which is an intended feature, see here: https://github.com/DSharpPlus/DSharpPlus/blob/6ff22ee20195cf6c5f4d5c0ff8f1eae55e9f0a11/DSharpPlus/DiscordClient.cs#L1507-L1509 When you edit a message, inside the MessageUpdated event, the message is cloned so _mentionedRoles and _mentionedChannels are cloned: https://github.com/DSharpPlus/DSharpPlus/blob/6ff22ee20195cf6c5f4d5c0ff8f1eae55e9f0a11/DSharpPlus/Entities/DiscordMessage.cs#L34-L35 Because the List constructor does not accept a null List, an exception is raised and it bubbles up to the try-catch for socket errors.

Changes proposed

  • Add a null check for DiscordMessage._mentionedChannels and _mentionedRoles

Notes

Thanks to Kef on Discord for finding this issue, since it's such an absurd edge-case scenario.

Additionally, I added a note that MentionedChannels and MentionedRoles would likely throw an exception when accessed in DM, since there is a lazy ReadOnlyCollection wrapper for the internal list, which would be null. I haven't come up with a good solution for that, however it's a very low priority bug and a fix would probably be to just fail fast in that case.

Ref: #268

Merge request reports