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

Add linearizer on user ID to push rule PUT/DELETE requests #16052

Merged
merged 3 commits into from
Aug 11, 2023
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/16052.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Linearize push rule modification endpoints to prevent deadlocks. Contributed by Nick @ Beeper (@fizzadar).
Fizzadar marked this conversation as resolved.
Show resolved Hide resolved
28 changes: 22 additions & 6 deletions synapse/rest/client/push_rule.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
from synapse.rest.client._base import client_patterns
from synapse.storage.push_rule import InconsistentRuleException, RuleNotFoundException
from synapse.types import JsonDict
from synapse.util.async_helpers import Linearizer

if TYPE_CHECKING:
from synapse.server import HomeServer
Expand All @@ -53,26 +54,32 @@ def __init__(self, hs: "HomeServer"):
self.notifier = hs.get_notifier()
self._is_worker = hs.config.worker.worker_app is not None
self._push_rules_handler = hs.get_push_rules_handler()
self._push_rule_linearizer = Linearizer(name="push_rules")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if it matters, but this won't fix the issue if there are multiple workers involved. Would it make more sense to lock the user's push rule rows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's a good point, I'm basing the change entirley on the current state where push rules can only be modified on the main process, this only works while that limitation remains (does that block merging as-is, not sure?).

Copy link
Contributor

Choose a reason for hiding this comment

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

I had assumed we could move these to workers, my fault for not checking!


async def on_PUT(self, request: SynapseRequest, path: str) -> Tuple[int, JsonDict]:
if self._is_worker:
raise Exception("Cannot handle PUT /push_rules on worker")

requester = await self.auth.get_user_by_req(request)
user_id = requester.user.to_string()

async with self._push_rule_linearizer.queue(user_id):
return await self.handle_put(request, path, user_id)

async def handle_put(
self, request: SynapseRequest, path: str, user_id: str
) -> Tuple[int, JsonDict]:
spec = _rule_spec_from_path(path.split("/"))
try:
priority_class = _priority_class_from_spec(spec)
except InvalidRuleException as e:
raise SynapseError(400, str(e))

requester = await self.auth.get_user_by_req(request)

if "/" in spec.rule_id or "\\" in spec.rule_id:
raise SynapseError(400, "rule_id may not contain slashes")

content = parse_json_value_from_request(request)

user_id = requester.user.to_string()

if spec.attr:
try:
await self._push_rules_handler.set_rule_attr(user_id, spec, content)
Expand Down Expand Up @@ -126,11 +133,20 @@ async def on_DELETE(
if self._is_worker:
raise Exception("Cannot handle DELETE /push_rules on worker")

spec = _rule_spec_from_path(path.split("/"))

requester = await self.auth.get_user_by_req(request)
user_id = requester.user.to_string()

async with self._push_rule_linearizer.queue(user_id):
return await self.handle_delete(request, path, user_id)

async def handle_delete(
self,
request: SynapseRequest,
path: str,
user_id: str,
) -> Tuple[int, JsonDict]:
spec = _rule_spec_from_path(path.split("/"))

namespaced_rule_id = f"global/{spec.template}/{spec.rule_id}"

try:
Expand Down
Loading