Skip to content

Fix for invalid DiscordMember instance when user is not cached

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

Created by: uwx

Summary

Fixes an issue reported by @BloodfallenTear (AFO)#6685 on Discord, where attempting to resolve User-backed properties in DiscordMember in the GuildMemberRemoved event would throw a KeyNotFoundException.

Details

This is the stack trace:

System.AggregateException: Exceptions occured within one or more event handlers. Check InnerExceptions for details. (The given key 'KEY' was not present in the dictionary.)
 ---> System.Collections.Generic.KeyNotFoundException: The given key 'KEY' was not present in the dictionary.
   at System.Collections.Concurrent.ConcurrentDictionary`2.ThrowKeyNotFoundException(Object key)
   at System.Collections.Concurrent.ConcurrentDictionary`2.get_Item(TKey key)
   at DSharpPlus.Entities.DiscordMember.get_Username()
   at AFOBotV2.Program.DiscordClient_GuildMemberRemoved(GuildMemberRemoveEventArgs e) in C:\Users\NAME\source\repos\AFOBot\AFOBotV2\Program.cs:line 675
   at DSharpPlus.AsyncEvent`1.InvokeAsync(T e)
   --- End of inner exception stack trace ---

This issue is caused by a design flaw that is better outlined in #Notes because it is too specific to detail here. The issue was fixed by updating the cache prior to calling the event handler so that the DiscordMember instance has an User to lookup when trying to get one of its properties.

Changes proposed

  • Update UserCache entry for the removed member in OnGuildMemberRemoveEventAsync

Notes

This issue highlights a different problem which is the DiscordUser parameter for the DiscordMember constructor serving no purpose other than assigning an ID. Whether or not the user should be added to the cache is a dubitable assumption, but the bigger issue is that the cache parity should have no effect on a DiscordMember object backed by a DiscordUser reference.

This fix may cause a memory leak, or (more likely) it might highlight an existing memory leak in the user cache caused by endless user addition. To fix this, either:

  • Add reference counting to the user cache (and other relevant caches) and decrement it whenever a member is removed
  • Use a ConditionalWeakTable (I would prefer this because it would keep in the cache any users that the library consumer might have held a reference to, at very little extra cost, but I'm not sure if it's thread-safe) that way the GC stops tracking the reference as soon as the member is removed from the guild's memberlist

This PR is untested. I do not have the time to test it right now.

Merge request reports