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

Fix ratelimiting for federation /send requests. #8342

Merged
merged 7 commits into from
Sep 18, 2020
Merged

Fix ratelimiting for federation /send requests. #8342

merged 7 commits into from
Sep 18, 2020

Conversation

erikjohnston
Copy link
Member

c.f. #8295 for rationale

@erikjohnston
Add ResponseCache to incoming transactions path
71a36f1
@erikjohnston
Move ratelimit of /send/ after queues
cc8a26d 
Otherwise we fill up the ratelimiter with blocked requests, which isn't
terribly helpful.
@erikjohnston
Newsfile
d34d7d7
@erikjohnston erikjohnston changed the base branch from develop to release-v1.20.0 September 17, 2020 16:23
@erikjohnston erikjohnston requested a review from a team September 17, 2020 16:24
@clokep clokep self-assigned this Sep 17, 2020
clokep
clokep reviewed Sep 17, 2020
View reviewed changes
synapse/federation/federation_server.py

self._server_linearizer = Linearizer("fed_server")
self._transaction_linearizer = Linearizer("fed_txn_handler")

# We cache results for transaction with the same ID
self._transaciton_resp_cache = ResponseCache(
hs, "fed_txn_handler", timeout_ms=30000
Copy link
Member

Choose a reason for hiding this comment

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

Curious about the reasoning about the timeout here. 30 seconds seems a bit short?

Copy link
Member Author

Choose a reason for hiding this comment

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

We tend to default to 30s for these I think. Note that this is 30s after the response is sent, though given the response won't generally be large we could probably up it some more.

Copy link
Member

@clokep clokep Sep 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, I thought it was 30s from when the request was sent. That makes more sense now!

synapse/federation/federation_server.py Outdated Show resolved Hide resolved
synapse/federation/federation_server.py Outdated Show resolved Hide resolved
synapse/federation/federation_server.py Outdated Show resolved Hide resolved
synapse/federation/federation_server.py Show resolved Hide resolved
synapse/federation/federation_server.py Outdated Show resolved Hide resolved
synapse/federation/federation_server.py
Comment on lines +171 to +172
# We rate limit here *after* we've queued up the incoming requests,
# so that we don't fill up the ratelimiter with blocked requests.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I'm following what this change is. Could you describe it a bit more? I see how the rate-limiter is moved deeper into the stack, but I'm not sure how a "blocked request" would fill it up previously that won't now? Does blocked mean a 429 is returned or that it got stuck in the linearizer?

@clokep
Copy link
Member

clokep commented Sep 17, 2020

Also #8295 gave four options:

  1. Linearize transactions based on origin rather than origin and transaction ID.
  2. Use a ResponseCache to deduplicate based on origin and transaction ID
  3. Remove requests from the linearizer and cache if the request has been cancelled.
  4. Do not include requests waiting in queues in the federation rate limiter. (This probably means moving the federation rate limiter down several layers for the transaction requests)

I think this does 2 and 4 right now?

erikjohnston and others added 3 commits September 17, 2020 18:30
@erikjohnston @clokep
Spelling
a98f8cb 
Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>
@erikjohnston
Only queue based on origin, not transaction ID
7323f7c
@erikjohnston
Comment
81f2b25
@erikjohnston
Copy link
Member Author

Also #8295 gave four options:

  1. Linearize transactions based on origin rather than origin and transaction ID.
  2. Use a ResponseCache to deduplicate based on origin and transaction ID
  3. Remove requests from the linearizer and cache if the request has been cancelled.
  4. Do not include requests waiting in queues in the federation rate limiter. (This probably means moving the federation rate limiter down several layers for the transaction requests)

I think this does 2 and 4 right now?

And now also 1

@clokep
Copy link
Member

clokep commented Sep 17, 2020

FYI I merged in release-v1.20.0 again to pick up the sytest blacklist changes I pushed.

clokep
clokep approved these changes Sep 17, 2020
View reviewed changes
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.

I'm pretty sure this is OK, it does seem we're changing the rate-limiting / linearization a bit to now essentially only allowing a single on_incoming_transaction request per origin at a time. I'm not sure if this is good or bad.

@erikjohnston
Copy link
Member Author

I'm pretty sure this is OK, it does seem we're changing the rate-limiting / linearization a bit to now essentially only allowing a single on_incoming_transaction request per origin at a time. I'm not sure if this is good or bad.

The other server should only be sending one transaction at a time, so in the majority of cases it shouldn't make a difference. when we're still processing older transactions then we probably do want to wait for that to finish before processing new ones, especially as the first txn may include events needed by the second

@erikjohnston erikjohnston merged commit 14b5b48 into release-v1.20.0 Sep 18, 2020
@erikjohnston erikjohnston deleted the erikj/429_fixes branch September 18, 2020 09:49
clokep added a commit that referenced this pull request Sep 18, 2020
@clokep
Merge tag 'v1.20.0rc5' into develop
00db778 
Synapse 1.20.0rc5 (2020-09-18)
==============================

In addition to the below, Synapse 1.20.0rc5 also includes the bug fix that was included in 1.19.3.

Features
--------

- Add flags to the `/versions` endpoint for whether new rooms default to using E2EE. ([\#8343](#8343))

Bugfixes
--------

- Fix rate limiting of federation `/send` requests. ([\#8342](#8342))
- Fix a longstanding bug where back pagination over federation could get stuck if it failed to handle a received event. ([\#8349](#8349))

Internal Changes
----------------

- Blacklist [MSC2753](matrix-org/matrix-spec-proposals#2753) SyTests until it is implemented. ([\#8285](#8285))
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Sep 22, 2020
@Midar
Update chat/matrix-synapse to 1.20.0
b9a5c7d 
Synapse 1.20.0 (2020-09-22)
===========================

No significant changes since v1.20.0rc5.

Removal warning
---------------

Historically, the [Synapse Admin
API](https://github.com/matrix-org/synapse/tree/master/docs) has been
accessible under the `/_matrix/client/api/v1/admin`,
`/_matrix/client/unstable/admin`, `/_matrix/client/r0/admin` and
`/_synapse/admin` prefixes. In a future release, we will be dropping support
for accessing Synapse's Admin API using the `/_matrix/client/*` prefixes. This
makes it easier for homeserver admins to lock down external access to the Admin
API endpoints.

Synapse 1.20.0rc5 (2020-09-18)
==============================

In addition to the below, Synapse 1.20.0rc5 also includes the bug fix that was included in 1.19.3.

Features
--------

- Add flags to the `/versions` endpoint for whether new rooms default to using E2EE. ([\#8343](matrix-org/synapse#8343))


Bugfixes
--------

- Fix rate limiting of federation `/send` requests. ([\#8342](matrix-org/synapse#8342))
- Fix a longstanding bug where back pagination over federation could get stuck if it failed to handle a received event. ([\#8349](matrix-org/synapse#8349))


Internal Changes
----------------

- Blacklist [MSC2753](matrix-org/matrix-spec-proposals#2753) SyTests until it is implemented. ([\#8285](matrix-org/synapse#8285))


Synapse 1.20.0rc4 (2020-09-16)
==============================

Synapse 1.20.0rc4 is identical to 1.20.0rc3, with the addition of the security fix that was included in 1.19.2.


Synapse 1.20.0rc3 (2020-09-11)
==============================

Bugfixes
--------

- Fix a bug introduced in v1.20.0rc1 where the wrong exception was raised when invalid JSON data is encountered. ([\#8291](matrix-org/synapse#8291))


Synapse 1.20.0rc2 (2020-09-09)
==============================

Bugfixes
--------

- Fix a bug introduced in v1.20.0rc1 causing some features related to notifications to misbehave following the implementation of unread counts. ([\#8280](matrix-org/synapse#8280))


Synapse 1.20.0rc1 (2020-09-08)
==============================

Removal warning
---------------

Some older clients used a [disallowed character](https://matrix.org/docs/spec/client_server/r0.6.1#post-matrix-client-r0-register-email-requesttoken) (`:`) in the `client_secret` parameter of various endpoints. The incorrect behaviour was allowed for backwards compatibility, but is now being removed from Synapse as most users have updated their client. Further context can be found at [\#6766](matrix-org/synapse#6766).

Features
--------

- Add an endpoint to query your shared rooms with another user as an implementation of [MSC2666](matrix-org/matrix-spec-proposals#2666). ([\#7785](matrix-org/synapse#7785))
- Iteratively encode JSON to avoid blocking the reactor. ([\#8013](matrix-org/synapse#8013), [\#8116](matrix-org/synapse#8116))
- Add support for shadow-banning users (ignoring any message send requests). ([\#8034](matrix-org/synapse#8034), [\#8092](matrix-org/synapse#8092), [\#8095](matrix-org/synapse#8095), [\#8142](matrix-org/synapse#8142), [\#8152](matrix-org/synapse#8152), [\#8157](matrix-org/synapse#8157), [\#8158](matrix-org/synapse#8158), [\#8176](matrix-org/synapse#8176))
- Use the default template file when its equivalent is not found in a custom template directory. ([\#8037](matrix-org/synapse#8037), [\#8107](matrix-org/synapse#8107), [\#8252](matrix-org/synapse#8252))
- Add unread messages count to sync responses, as specified in [MSC2654](matrix-org/matrix-spec-proposals#2654). ([\#8059](matrix-org/synapse#8059), [\#8254](matrix-org/synapse#8254), [\#8270](matrix-org/synapse#8270), [\#8274](matrix-org/synapse#8274))
- Optimise `/federation/v1/user/devices/` API by only returning devices with encryption keys. ([\#8198](matrix-org/synapse#8198))


Bugfixes
--------

- Fix a memory leak by limiting the length of time that messages will be queued for a remote server that has been unreachable. ([\#7864](matrix-org/synapse#7864))
- Fix `Re-starting finished log context PUT-nnnn` warning when event persistence failed. ([\#8081](matrix-org/synapse#8081))
- Synapse now correctly enforces the valid characters in the `client_secret` parameter used in various endpoints. ([\#8101](matrix-org/synapse#8101))
- Fix a bug introduced in v1.7.2 impacting message retention policies that would allow federated homeservers to dictate a retention period that's lower than the configured minimum allowed duration in the configuration file. ([\#8104](matrix-org/synapse#8104))
- Fix a long-standing bug where invalid JSON would be accepted by Synapse. ([\#8106](matrix-org/synapse#8106))
- Fix a bug introduced in Synapse v1.12.0 which could cause `/sync` requests to fail with a 404 if you had a very old outstanding room invite. ([\#8110](matrix-org/synapse#8110))
- Return a proper error code when the rooms of an invalid group are requested. ([\#8129](matrix-org/synapse#8129))
- Fix a bug which could cause a leaked postgres connection if synapse was set to daemonize. ([\#8131](matrix-org/synapse#8131))
- Clarify the error code if a user tries to register with a numeric ID. This bug was introduced in v1.15.0. ([\#8135](matrix-org/synapse#8135))
- Fix a bug where appservices with ratelimiting disabled would still be ratelimited when joining rooms. This bug was introduced in v1.19.0. ([\#8139](matrix-org/synapse#8139))
- Fix logging in via OpenID Connect with a provider that uses integer user IDs. ([\#8190](matrix-org/synapse#8190))
- Fix a longstanding bug where user directory updates could break when unexpected profile data was included in events. ([\#8223](matrix-org/synapse#8223))
- Fix a longstanding bug where stats updates could break when unexpected profile data was included in events. ([\#8226](matrix-org/synapse#8226))
- Fix slow start times for large servers by removing a table scan of the `users` table from startup code. ([\#8271](matrix-org/synapse#8271))


Updates to the Docker image
---------------------------

- Fix builds of the Docker image on non-x86 platforms. ([\#8144](matrix-org/synapse#8144))
- Added curl for healthcheck support and readme updates for the change. Contributed by @maquis196. ([\#8147](matrix-org/synapse#8147))


Improved Documentation
----------------------

- Link to matrix-synapse-rest-password-provider in the password provider documentation. ([\#8111](matrix-org/synapse#8111))
- Updated documentation to note that Synapse does not follow `HTTP 308` redirects due to an upstream library not supporting them. Contributed by Ryan Cole. ([\#8120](matrix-org/synapse#8120))
- Explain better what GDPR-erased means when deactivating a user. ([\#8189](matrix-org/synapse#8189))


Internal Changes
----------------

- Add filter `name` to the `/users` admin API, which filters by user ID or displayname. Contributed by Awesome Technologies Innovationslabor GmbH. ([\#7377](matrix-org/synapse#7377), [\#8163](matrix-org/synapse#8163))
- Reduce run times of some unit tests by advancing the reactor a fewer number of times. ([\#7757](matrix-org/synapse#7757))
- Don't fail `/submit_token` requests on incorrect session ID if `request_token_inhibit_3pid_errors` is turned on. ([\#7991](matrix-org/synapse#7991))
- Convert various parts of the codebase to async/await. ([\#8071](matrix-org/synapse#8071), [\#8072](matrix-org/synapse#8072), [\#8074](matrix-org/synapse#8074), [\#8075](matrix-org/synapse#8075), [\#8076](matrix-org/synapse#8076), [\#8087](matrix-org/synapse#8087), [\#8100](matrix-org/synapse#8100), [\#8119](matrix-org/synapse#8119), [\#8121](matrix-org/synapse#8121), [\#8133](matrix-org/synapse#8133), [\#8156](matrix-org/synapse#8156), [\#8162](matrix-org/synapse#8162), [\#8166](matrix-org/synapse#8166), [\#8168](matrix-org/synapse#8168), [\#8173](matrix-org/synapse#8173), [\#8191](matrix-org/synapse#8191), [\#8192](matrix-org/synapse#8192), [\#8193](matrix-org/synapse#8193), [\#8194](matrix-org/synapse#8194), [\#8195](matrix-org/synapse#8195), [\#8197](matrix-org/synapse#8197), [\#8199](matrix-org/synapse#8199), [\#8200](matrix-org/synapse#8200), [\#8201](matrix-org/synapse#8201), [\#8202](matrix-org/synapse#8202), [\#8207](matrix-org/synapse#8207), [\#8213](matrix-org/synapse#8213), [\#8214](matrix-org/synapse#8214))
- Remove some unused database functions. ([\#8085](matrix-org/synapse#8085))
- Add type hints to various parts of the codebase. ([\#8090](matrix-org/synapse#8090), [\#8127](matrix-org/synapse#8127), [\#8187](matrix-org/synapse#8187), [\#8241](matrix-org/synapse#8241), [\#8140](matrix-org/synapse#8140), [\#8183](matrix-org/synapse#8183), [\#8232](matrix-org/synapse#8232), [\#8235](matrix-org/synapse#8235), [\#8237](matrix-org/synapse#8237), [\#8244](matrix-org/synapse#8244))
- Return the previous stream token if a non-member event is a duplicate. ([\#8093](matrix-org/synapse#8093), [\#8112](matrix-org/synapse#8112))
- Separate `get_current_token` into two since there are two different use cases for it. ([\#8113](matrix-org/synapse#8113))
- Remove `ChainedIdGenerator`. ([\#8123](matrix-org/synapse#8123))
- Reduce the amount of whitespace in JSON stored and sent in responses. ([\#8124](matrix-org/synapse#8124))
- Update the test federation client to handle streaming responses. ([\#8130](matrix-org/synapse#8130))
- Micro-optimisations to `get_auth_chain_ids`. ([\#8132](matrix-org/synapse#8132))
- Refactor `StreamIdGenerator` and `MultiWriterIdGenerator` to have the same interface. ([\#8161](matrix-org/synapse#8161))
- Add functions to `MultiWriterIdGen` used by events stream. ([\#8164](matrix-org/synapse#8164), [\#8179](matrix-org/synapse#8179))
- Fix tests that were broken due to the merge of 1.19.1. ([\#8167](matrix-org/synapse#8167))
- Make `SlavedIdTracker.advance` have the same interface as `MultiWriterIDGenerator`. ([\#8171](matrix-org/synapse#8171))
- Remove unused `is_guest` parameter from, and add safeguard to, `MessageHandler.get_room_data`. ([\#8174](matrix-org/synapse#8174), [\#8181](matrix-org/synapse#8181))
- Standardize the mypy configuration. ([\#8175](matrix-org/synapse#8175))
- Refactor some of `LoginRestServlet`'s helper methods, and move them to `AuthHandler` for easier reuse. ([\#8182](matrix-org/synapse#8182))
- Fix `wait_for_stream_position` to allow multiple waiters on same stream ID. ([\#8196](matrix-org/synapse#8196))
- Make `MultiWriterIDGenerator` work for streams that use negative values. ([\#8203](matrix-org/synapse#8203))
- Refactor queries for device keys and cross-signatures. ([\#8204](matrix-org/synapse#8204), [\#8205](matrix-org/synapse#8205), [\#8222](matrix-org/synapse#8222), [\#8224](matrix-org/synapse#8224), [\#8225](matrix-org/synapse#8225), [\#8231](matrix-org/synapse#8231), [\#8233](matrix-org/synapse#8233), [\#8234](matrix-org/synapse#8234))
- Fix type hints for functions decorated with `@cached`. ([\#8240](matrix-org/synapse#8240))
- Remove obsolete `order` field from federation send queues. ([\#8245](matrix-org/synapse#8245))
- Stop sub-classing from object. ([\#8249](matrix-org/synapse#8249))
- Add more logging to debug slow startup. ([\#8264](matrix-org/synapse#8264))
- Do not attempt to upgrade database schema on worker processes. ([\#8266](matrix-org/synapse#8266), [\#8276](matrix-org/synapse#8276))
@richvdh richvdh mentioned this pull request Feb 24, 2021
Closed
babolivier pushed a commit that referenced this pull request Sep 1, 2021
@anoadragon453
Merge commit '43f2b67e4' into anoa/dinsic_release_1_21_x
70783a3 
* commit '43f2b67e4':
  Intelligently select extremities used in backfill. (#8349)
  Add flags to /versions about whether new rooms are encrypted by default. (#8343)
  Fix ratelimiting for federation `/send` requests. (#8342)
  blacklist MSC2753 sytests until it's implemented in synapse (#8285)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@clokep clokep clokep approved these changes

Assignees

@clokep clokep

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@erikjohnston @clokep