Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: revert to weakrefs for user cache #858

Merged
merged 4 commits into from Dec 12, 2022

Conversation

shiftinv
Copy link
Member

@shiftinv shiftinv commented Nov 7, 2022

Summary

This largely reverts cb2363f.

Users were not being evicted as expected, resulting in objects being retained solely in the user cache, with no other strong references.
See https://docs.python.org/3.10/library/weakref.html#weakref.WeakValueDictionary and https://docs.python.org/3.10/reference/datamodel.html#object.del.
The difference in performance and memory usage is insignificant, at least in 3.10; haven't tested other versions, but this is a bug regardless of version.

To reproduce, anything that uses state.store_user can be used, for example the GUILD_BAN_REMOVE handler:

print(bot.get_user(21414249976823808))
await ctx.guild.ban(disnake.Object(21414249976823808))
await ctx.guild.unban(disnake.Object(21414249976823808))
await asyncio.sleep(0.5)
print(bot.get_user(21414249976823808))

With the previous implementation, the second print would output the user (assuming members intent is enabled) - with the changes in this PR, both print None as expected.

(for context, this was originally noticed a while ago in an internal channel here)

Checklist

  • If code changes were made, then they have been tested
    • I have updated the documentation to reflect the changes
    • I have formatted the code properly by running task lint
    • I have type-checked the code by running task pyright
  • This PR fixes an issue
  • This PR adds something new (e.g. new method or parameters)
  • This PR is a breaking change (e.g. methods or parameters removed/renamed)
  • This PR is not a code change (e.g. documentation, README, ...)

This largely reverts cb2363f.

Users were not being evicted as expected, resulting in objects being retained solely in the user cache, with no other references.
@shiftinv shiftinv added s: needs review Issue/PR is awaiting reviews t: bugfix labels Nov 7, 2022
@shiftinv shiftinv added this to the disnake v2.8 milestone Nov 7, 2022
disnake/state.py Show resolved Hide resolved
@onerandomusername onerandomusername merged commit e24e7a4 into master Dec 12, 2022
@onerandomusername onerandomusername deleted the fix/user-reference-leaks branch December 12, 2022 02:44
@onerandomusername onerandomusername removed the s: needs review Issue/PR is awaiting reviews label Feb 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants