Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign updon't store more remote device lists if they have more than 1K devices #4397
Conversation
ara4n
and others
added some commits
Jan 15, 2019
richvdh
requested a review
from matrix-org/synapse-core
Jan 15, 2019
This comment has been minimized.
This comment has been minimized.
codecov-io
commented
Jan 15, 2019
Codecov Report
@@ Coverage Diff @@
## develop #4397 +/- ##
===========================================
- Coverage 73.65% 73.65% -0.01%
===========================================
Files 300 300
Lines 29815 29818 +3
Branches 4897 4898 +1
===========================================
+ Hits 21960 21961 +1
Misses 6414 6414
- Partials 1441 1443 +2
Continue to review full report at Codecov.
|
erikjohnston
approved these changes
Jan 16, 2019
LGTM, though we should also probably do something so that synapse won't get to the stage of having so many devices for a user, somehow. |
richvdh
merged commit 05e1296
into
develop
Jan 16, 2019
5 checks passed
ci/circleci: sytestpy2merged
Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2postgresmerged
Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3merged
Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3postgresmerged
Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr
The Travis CI build passed
Details
richvdh
deleted the
rav/bodge_device_update_dos
branch
Jan 16, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
richvdh commentedJan 15, 2019
•
edited
Backport of #4396 to develop.
If the remote server has more than ~1000 devices for this user we assume that something is going horribly wrong (e.g. a bot that logs in and creates a new device every time it tries to send a message). Maintaining lots of devices per user in the cache can cause serious performance issues as if this request takes more than 60s to complete, internal replication from the inbound federation worker to the synapse master may time out causing the inbound federation to fail and causing the remote server to retry, causing a DoS. So in this scenario we give up on storing the total list of devices and only handle the delta instead.