Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Avoid sharing room hierarchy responses between users #11355

Merged
merged 3 commits into from Nov 16, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/11355.bugfix
@@ -0,0 +1 @@
Fix a bug introduced in 1.41.0 where space hierarchy responses would be incorrectly reused if multiple users were to make the same request at the same time.
11 changes: 9 additions & 2 deletions synapse/handlers/room_summary.py
Expand Up @@ -97,7 +97,7 @@ def __init__(self, hs: "HomeServer"):
# If a user tries to fetch the same page multiple times in quick succession,
# only process the first attempt and return its result to subsequent requests.
self._pagination_response_cache: ResponseCache[
Tuple[str, bool, Optional[int], Optional[int], Optional[str]]
Tuple[str, str, bool, Optional[int], Optional[int], Optional[str]]
] = ResponseCache(
hs.get_clock(),
"get_room_hierarchy",
Expand Down Expand Up @@ -282,7 +282,14 @@ async def get_room_hierarchy(
# This is due to the pagination process mutating internal state, attempting
# to process multiple requests for the same page will result in errors.
return await self._pagination_response_cache.wrap(
(requested_room_id, suggested_only, max_depth, limit, from_token),
(
requester,
requested_room_id,
suggested_only,
max_depth,
limit,
from_token,
),
self._get_room_hierarchy,
requester,
requested_room_id,
Expand Down
55 changes: 55 additions & 0 deletions tests/handlers/test_room_summary.py
Expand Up @@ -14,6 +14,8 @@
from typing import Any, Iterable, List, Optional, Tuple
from unittest import mock

from twisted.internet.defer import ensureDeferred

from synapse.api.constants import (
EventContentFields,
EventTypes,
Expand Down Expand Up @@ -316,6 +318,59 @@ def test_visibility(self):
AuthError,
)

def test_room_hierarchy_cache(self) -> None:
"""In-flight room hierarchy requests are deduplicated."""
# Run two `get_room_hierarchy` calls up until they block.
Copy link
Contributor

Choose a reason for hiding this comment

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

"up until they block" is why we manually call ensureDeferred instead of letting get_success do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, otherwise they'd run sequentially and the cache wouldn't do anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's what I thought, just double checking!

deferred1 = ensureDeferred(
self.handler.get_room_hierarchy(self.user, self.space)
)
deferred2 = ensureDeferred(
self.handler.get_room_hierarchy(self.user, self.space)
)

# Complete the two calls.
result1 = self.get_success(deferred1)
result2 = self.get_success(deferred2)

# Both `get_room_hierarchy` calls should return the same result.
expected = [(self.space, [self.room]), (self.room, ())]
self._assert_hierarchy(result1, expected)
self._assert_hierarchy(result2, expected)
self.assertIs(result1, result2)

# A subsequent `get_room_hierarchy` call should reuse the result.
Copy link
Contributor

Choose a reason for hiding this comment

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

How are we testing that it re-uses it? We assert that result1 != result3 so I'm not sure what we're checking here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, that comment is missing a not.

result3 = self.get_success(
self.handler.get_room_hierarchy(self.user, self.space)
)
self._assert_hierarchy(result3, expected)
self.assertIsNot(result1, result3)

def test_room_hierarchy_cache_sharing(self) -> None:
"""Room hierarchy responses for different users are not shared."""
user2 = self.register_user("user2", "pass")

# Make the room within the space invite-only.
self.helper.send_state(
self.room,
event_type=EventTypes.JoinRules,
body={"join_rule": JoinRules.INVITE},
tok=self.token,
)

# Run two `get_room_hierarchy` calls for different users up until they block.
deferred1 = ensureDeferred(
self.handler.get_room_hierarchy(self.user, self.space)
)
deferred2 = ensureDeferred(self.handler.get_room_hierarchy(user2, self.space))

# Complete the two calls.
result1 = self.get_success(deferred1)
result2 = self.get_success(deferred2)

# The `get_room_hierarchy` calls should return different results.
self._assert_hierarchy(result1, [(self.space, [self.room]), (self.room, ())])
self._assert_hierarchy(result2, [(self.space, [self.room])])

def _create_room_with_join_rule(
self, join_rule: str, room_version: Optional[str] = None, **extra_content
) -> str:
Expand Down