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

Fix cross-worker ratelimiting #16558

Merged
merged 8 commits into from
Oct 27, 2023
Merged

Conversation

erikjohnston
Copy link
Member

c.f. #16481

synapse/handlers/message.py Outdated Show resolved Hide resolved
synapse/handlers/message.py Outdated Show resolved Hide resolved
synapse/handlers/message.py Outdated Show resolved Hide resolved
@erikjohnston erikjohnston marked this pull request as ready for review October 26, 2023 18:47
@erikjohnston erikjohnston requested a review from a team as a code owner October 26, 2023 18:47
Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

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

Overall seems reasonable, but a couple sanity checks.

synapse/handlers/message.py Outdated Show resolved Hide resolved
synapse/handlers/message.py Show resolved Hide resolved
@@ -1508,6 +1527,18 @@ async def _persist_events(
first_event.room_id
)
if writer_instance != self._instance_name:
# Ratelimit before sending to the other event persister, to
# ensure that we correctly have ratelimits on both the event
# creators and event persiters.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# creators and event persiters.
# creators and event persisters.

Copy link
Member

Choose a reason for hiding this comment

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

If there are multiple creators going to the same persister (Is that possible?) than we might pass this rate check, but fail the one on the persister.

I think the opposite isn't true since persisters are sharded by room so we either have a 1-to-1 mapping or a many-to-1 mapping? But the creater could fill the bucket faster if for some reason the send_events below fails (e.g. the persister is offline for a period?)

I guess restarts would also make these go out of sync?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, these can totally get out of sync. However, if they do get out of sync then the worst that is going to happen is that you're being ratelimited when you're already over the threshold, so 🤷

@erikjohnston erikjohnston merged commit 928e964 into develop Oct 27, 2023
41 checks passed
@erikjohnston erikjohnston deleted the erikj/fix_worker_ratelimits branch October 27, 2023 11:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants