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 some cache bugs and ensure member is always returned on delete calls #892

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

davfsa
Copy link
Member

@davfsa davfsa commented Nov 7, 2021

Checklist

  • I have run nox and all the pipelines have passed.
  • I have made unittests according to the code I have added/modified/deleted.

@davfsa davfsa added the bug Something isn't working label Nov 7, 2021
@@ -820,8 +820,6 @@ def _garbage_collect_member(
guild_record.members = None
self._remove_guild_record_if_empty(member.object.guild_id, guild_record)

return member
Copy link
Collaborator

@FasterSpeeding FasterSpeeding Nov 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should probably just change this to return the object if it wasn't originally marked as has_been_deleted

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I changed this to not return anything is because there is no need, as the information we return in the same as the one we take in. The cases we use this for atm don't even need what we return

Copy link
Collaborator

@FasterSpeeding FasterSpeeding Nov 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clear and delete methods should only be returning objects which were actually marked as deleted not objects which were only being kept alive by references, the way you've changed the semantics still leads to erroneous behaviour and simply changing the behaviour of the garbage collect method will instruct both the delete and clear methods on what they should be saying was deleted and avoids a ton of other code changes

hikari/impl/cache.py Outdated Show resolved Hide resolved
@@ -1459,12 +1455,13 @@ def _garbage_collect_message(
*,
decrement: typing.Optional[int] = None,
override_ref: bool = False,
) -> typing.Optional[cache_utility.RefCell[cache_utility.MessageData]]:
) -> bool:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change seems unnecessary and breaks the standard pattern for these methods

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other methods currently only return None. I changed this since I dont think it makes sense to return the exact same thing that was sent in, considering how we are using this function atm

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The methods which were previously only returning None are doing this because they have no relevant delete and clear methods, they are not the same case

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

think it makes sense to return the exact same thing that was sent in,

Look at how clear members was using it before

@@ -1485,7 +1482,7 @@ def _garbage_collect_message(
if message.object.id in self._referenced_messages:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should return None/False/whatever you feel like if it was already marked as deleted

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The uses for this function stay the same, so either its a bug I didnt fix or I did in fact miss something. I dont get the point for an extra return tho

Copy link
Collaborator

@FasterSpeeding FasterSpeeding Nov 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clear and delete methods should only be returning objects which were actually marked as deleted not objects which were only being kept alive by references

@davfsa davfsa marked this pull request as draft November 9, 2021 08:39
@davfsa davfsa self-assigned this Nov 9, 2021
@davfsa
Copy link
Member Author

davfsa commented Jan 1, 2022

Will let @norinorin take care of this pr

@davfsa davfsa closed this Jan 1, 2022
@davfsa
Copy link
Member Author

davfsa commented Jan 1, 2022

Sorry, wrong pr 😅

@davfsa davfsa reopened this Jan 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants