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.

Sign up for GitHub

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
+55 −40
Draft

Fix some cache bugs and ensure member is always returned on delete calls #892

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f480e49
Fix some cache bugs and ensure member is always returned on delete calls
davfsa Nov 7, 2021
d855276
Create 892.bugfix.md
davfsa Nov 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions changes/892.bugfix.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Several cache bugfixes
- `CacheImpl.clear_members` and `CacheImpl.delete_member` only returning the members that have been fully deleted
- This was contrary to all the other methods were the objects would be returned regardlesss of the internal references to it
- `CacheImpl.clear_voice_states_for_channel` not actually deleting the voice states
40 changes: 18 additions & 22 deletions hikari/impl/cache.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -799,7 +799,7 @@ def _garbage_collect_member(
*,
decrement: typing.Optional[int] = None,
deleting: bool = False,
) -> typing.Optional[cache_utility.RefCell[cache_utility.MemberData]]:
) -> None:
if deleting:
member.object.has_been_deleted = True

Expand All @@ -808,10 +808,10 @@ def _garbage_collect_member(

user_id = member.object.user.object.id
if not guild_record.members or user_id not in guild_record.members:
return None
return

if not self._can_remove_member(member):
return None
return

del guild_record.members[user_id]
self._garbage_collect_user(member.object.user, decrement=1)
Expand All @@ -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 •
edited
Loading

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 •
edited
Loading

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


def clear_members(
self,
) -> cache.CacheView[snowflakes.Snowflake, cache.CacheView[snowflakes.Snowflake, guilds.Member]]:
Expand All @@ -843,9 +841,10 @@ def clear_members_for_guild(
return cache_utility.EmptyCacheView()

cached_members = guild_record.members.freeze()
members_gen = (self._garbage_collect_member(guild_record, m, deleting=True) for m in cached_members.values())
# _garbage_collect_member will only return the member data object if they could be removed, else None.
cached_members = {member.object.user.object.id: member for member in members_gen if member}

for m in cached_members.values():
self._garbage_collect_member(guild_record, m, deleting=True)

self._remove_guild_record_if_empty(guild_id, guild_record)
return cache_utility.CacheMappingView(cached_members, builder=self._build_member) # type: ignore[type-var]

Expand All @@ -868,13 +867,8 @@ def delete_member(
if not member_data:
return None

if not guild_record.members:
guild_record.members = None
self._remove_guild_record_if_empty(guild_id, guild_record)

# _garbage_collect_member will only return the member data object if they could be removed, else None.
garbage_collected = self._garbage_collect_member(guild_record, member_data, deleting=True)
return self._build_member(member_data) if garbage_collected else None
self._garbage_collect_member(guild_record, member_data, deleting=True)
return self._build_member(member_data)

def get_member(
self,
Expand Down Expand Up @@ -1297,8 +1291,9 @@ def clear_voice_states_for_channel(

cached_voice_states = {}

for user_id, voice_state in guild_record.voice_states.items():
for user_id, voice_state in tuple(guild_record.voice_states.items()):
if voice_state.channel_id == channel_id:
del guild_record.voice_states[user_id]
cached_voice_states[user_id] = voice_state
self._garbage_collect_member(guild_record, voice_state.member, decrement=1)

Expand Down Expand Up @@ -1348,11 +1343,12 @@ def delete_voice_state(
if not voice_state_data:
return None

self._garbage_collect_member(guild_record, voice_state_data.member, decrement=1)

if not guild_record.voice_states:
guild_record.voice_states = None
self._remove_guild_record_if_empty(guild_id, guild_record)

FasterSpeeding marked this conversation as resolved.
Show resolved Hide resolved
self._garbage_collect_member(guild_record, voice_state_data.member, decrement=1)
self._remove_guild_record_if_empty(guild_id, guild_record)
return self._build_voice_state(voice_state_data)

def get_voice_state(
Expand Down Expand Up @@ -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

# A bool is returned to inform whether the message was removed or not
if decrement is not None:
self._increment_ref_count(message, -decrement)

if not self._can_remove_message(message) or override_ref:
return None
return False

self._garbage_collect_user(message.object.author, decrement=1)

Expand All @@ -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 •
edited
Loading

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

del self._referenced_messages[message.object.id]

return message
return True

def _on_message_expire(self, message: cache_utility.RefCell[cache_utility.MessageData], /) -> None:
if not self._garbage_collect_message(message):
Expand Down Expand Up @@ -1520,7 +1517,6 @@ def delete_message(

if not self._garbage_collect_message(message_data):
self._referenced_messages[message_id] = message_data
return None

return self._build_message(message_data)

Expand Down
2 changes: 1 addition & 1 deletion hikari/voices.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ class VoiceState:
session_id: str = attr.field(hash=True, repr=True)
"""The string ID of this voice state's session."""

requested_to_speak_at: typing.Optional[datetime.datetime] = attr.field(eq=False, hash=False, repr=True)
requested_to_speak_at: typing.Optional[datetime.datetime] = attr.field(eq=False, hash=False, repr=False)
"""When the user requested to speak in a stage channel.
Will be `builtins.None` if they have not requested to speak.
Expand Down
49 changes: 32 additions & 17 deletions tests/hikari/impl/test_cache.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1651,7 +1651,7 @@ def test_delete_member_for_unknown_member_cache(self, cache_impl):

assert result is None

def test_delete_member_for_known_member(self, cache_impl):
def test_delete_member(self, cache_impl):
mock_member = mock.Mock(guilds.Member)
mock_user = cache_utilities.RefCell(mock.Mock(id=snowflakes.Snowflake(67876)))
mock_member_data = mock.Mock(
Expand All @@ -1672,21 +1672,6 @@ def test_delete_member_for_known_member(self, cache_impl):
cache_impl._garbage_collect_user.assert_called_once_with(mock_user, decrement=1)
cache_impl._remove_guild_record_if_empty.assert_called_once_with(snowflakes.Snowflake(42123), guild_record)

def test_delete_member_for_known_hard_referenced_member(self, cache_impl):
mock_member = cache_utilities.RefCell(mock.Mock(has_been_deleted=False), ref_count=1)
cache_impl._guild_entries = collections.FreezableDict(
{
snowflakes.Snowflake(42123): cache_utilities.GuildRecord(
members=collections.FreezableDict({snowflakes.Snowflake(67876): mock_member})
)
}
)

result = cache_impl.delete_member(StubModel(42123), StubModel(67876))

assert result is None
assert mock_member.object.has_been_deleted is True

def test_get_member_for_unknown_member_cache(self, cache_impl):
cache_impl._guild_entries = collections.FreezableDict(
{snowflakes.Snowflake(1234213): cache_utilities.GuildRecord()}
Expand Down Expand Up @@ -2222,11 +2207,41 @@ def test_delete_voice_state(self, cache_impl):

assert result is mock_voice_state
cache_impl._garbage_collect_member.assert_called_once_with(guild_record, mock_member_data, decrement=1)
cache_impl._remove_guild_record_if_empty.assert_called_once_with(snowflakes.Snowflake(43123), guild_record)
cache_impl._remove_guild_record_if_empty.assert_not_called()
assert cache_impl._guild_entries[snowflakes.Snowflake(43123)].voice_states == {
snowflakes.Snowflake(6541234): mock_other_voice_state_data
}

def test_delete_voice_state_when_no_voice_states_left(self, cache_impl):
mock_member_data = object()
mock_voice_state_data = mock.Mock(cache_utilities.VoiceStateData, member=mock_member_data)
mock_voice_state = mock.Mock(voices.VoiceState)
cache_impl._build_voice_state = mock.Mock(return_value=mock_voice_state)
guild_record = cache_utilities.GuildRecord(
voice_states=collections.FreezableDict({snowflakes.Snowflake(12354345): mock_voice_state_data}),
members=collections.FreezableDict(
{snowflakes.Snowflake(12354345): mock_member_data, snowflakes.Snowflake(9955959): object()}
),
)
cache_impl._user_entries = collections.FreezableDict(
{snowflakes.Snowflake(12354345): object(), snowflakes.Snowflake(9393): object()}
)
cache_impl._guild_entries = collections.FreezableDict(
{
snowflakes.Snowflake(65234): mock.Mock(cache_utilities.GuildRecord),
snowflakes.Snowflake(43123): guild_record,
}
)
cache_impl._remove_guild_record_if_empty = mock.Mock()
cache_impl._garbage_collect_member = mock.Mock()

result = cache_impl.delete_voice_state(StubModel(43123), StubModel(12354345))

assert result is mock_voice_state
cache_impl._garbage_collect_member.assert_called_once_with(guild_record, mock_member_data, decrement=1)
cache_impl._remove_guild_record_if_empty.assert_called_once_with(snowflakes.Snowflake(43123), guild_record)
assert cache_impl._guild_entries[snowflakes.Snowflake(43123)].voice_states is None

def test_delete_voice_state_unknown_state(self, cache_impl):
mock_other_voice_state_data = mock.Mock(cache_utilities.VoiceStateData)
cache_impl._build_voice_state = mock.Mock()
Expand Down