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

Improve appservice handler to send only the most recent ephemeral events when no stream_id is stored. #8744

Merged
merged 8 commits into from
Nov 18, 2020

Conversation

Half-Shot
Copy link
Collaborator

@Half-Shot Half-Shot commented Nov 11, 2020

Fixes #8743

This changes the AS handler code so that it will appropriately handle new appservices and not hammer the database with large queries. Appservices store their stream_id for read receipts in the database, so they can keep track of how much they need to send in each transaction. However, this value will default to 0 if not found which means the first transaction of read receipts would contain every change made since the creation of the homeserver.

This change aims to make it so that it starts from a reasonable position, which is the last 100 read receipts.

@Half-Shot Half-Shot changed the title Hs/as edu perf improve Improve appservice handler to send only the most recent ephemral events when no stream_id is stored. Nov 11, 2020
changelog.d/8744.bugfix Outdated Show resolved Hide resolved
@Half-Shot Half-Shot force-pushed the hs/as-edu-perf-improve branch from 7af42fa to 3daef70 Compare November 11, 2020 16:18
@Half-Shot Half-Shot changed the title Improve appservice handler to send only the most recent ephemral events when no stream_id is stored. Improve appservice handler to send only the most recent ephemeral events when no stream_id is stored. Nov 11, 2020
@Half-Shot Half-Shot force-pushed the hs/as-edu-perf-improve branch from 20db863 to f76461a Compare November 11, 2020 18:12
@Half-Shot Half-Shot requested a review from a team November 11, 2020 20:33
changelog.d/8744.bugfix Outdated Show resolved Hide resolved
synapse/handlers/appservice.py Outdated Show resolved Hide resolved
@Half-Shot Half-Shot force-pushed the hs/as-edu-perf-improve branch 3 times, most recently from 77402bb to 2354641 Compare November 18, 2020 12:28
Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

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

So we've effectively changed the entire focus of this PR. Is starting from stream position 0 no longer a concern?

synapse/handlers/appservice.py Show resolved Hide resolved
synapse/storage/databases/main/receipts.py Show resolved Hide resolved
@Half-Shot
Copy link
Collaborator Author

So we've effectively changed the entire focus of this PR

I don't think so. The PR's focus is to "to send only the most recent ephemeral events when no stream_id is stored". I believe that's what this code does.

Is starting from stream position 0 no longer a concern?

Nope, starting from 0 (or indeed, any position far enough behind the mark) will cause Synapse to fetch the "latest" 100 events rather than all events the appservice has missed.

This is by design, as asking synapse to fetch events from 0, 1, 100, 1000 or whatever when your stream position is at 65535 is going to cause massive latency in both synapse and the bridge while it desperately tries to forward everything recorded. Typically it's not useful to see all read receipts anyway, as appservices are typically only interested in "recent" information.

Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

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

Nope, starting from 0 (or indeed, any position far enough behind the mark) will cause Synapse to fetch the "latest" 100 events rather than all events the appservice has missed.

Ahhh, didn't twig this would also solve the issue in my last read-through, but of course the DESC makes all the difference. Awesome!

A couple more questions:

  • It would still be nice to have a test that checks that only 100 events are returned when having a stream_id gap that's larger than 100, but I won't hold you to it.
  • Is this only a problem for read receipts, what about other EDUs?

synapse/handlers/appservice.py Show resolved Hide resolved
@Half-Shot
Copy link
Collaborator Author

Is this only a problem for read receipts, what about other EDUs?

Typing is okay because we only ever send the latest events anyway, we don't even store the stream token because appservices aren't interested in "old" typing events (how would that even work).

Presence is okay because we look it up per user, so typically even a stream_id of 0 would mean fetching one presence event.

Read receipts are the outlier because you might well have tons of events come back from a value of 0

@Half-Shot Half-Shot force-pushed the hs/as-edu-perf-improve branch from efdb610 to cd306a0 Compare November 18, 2020 13:30
@Half-Shot Half-Shot force-pushed the hs/as-edu-perf-improve branch from cd306a0 to 72c6165 Compare November 18, 2020 18:14
@Half-Shot Half-Shot merged commit 5133849 into develop Nov 18, 2020
@Half-Shot Half-Shot deleted the hs/as-edu-perf-improve branch November 18, 2020 18:54
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Dec 9, 2020
Synapse 1.24.0 (2020-12-09)
===========================

Due to the two security issues highlighted below, server administrators are
encouraged to update Synapse. We are not aware of these vulnerabilities being
exploited in the wild.

Security advisory
-----------------

The following issues are fixed in v1.23.1 and v1.24.0.

- There is a denial of service attack
  ([CVE-2020-26257](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-26257))
  against the federation APIs in which future events will not be correctly sent
  to other servers over federation. This affects all servers that participate in
  open federation. (Fixed in [#8776](matrix-org/synapse#8776)).

- Synapse may be affected by OpenSSL
  [CVE-2020-1971](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-1971).
  Synapse administrators should ensure that they have the latest versions of
  the cryptography Python package installed.

To upgrade Synapse along with the cryptography package:

* Administrators using the [`matrix.org` Docker
  image](https://hub.docker.com/r/matrixdotorg/synapse/) or the [Debian/Ubuntu
  packages from
  `matrix.org`](https://github.com/matrix-org/synapse/blob/master/INSTALL.md#matrixorg-packages)
  should ensure that they have version 1.24.0 or 1.23.1 installed: these images include
  the updated packages.
* Administrators who have [installed Synapse from
  source](https://github.com/matrix-org/synapse/blob/master/INSTALL.md#installing-from-source)
  should upgrade the cryptography package within their virtualenv by running:
  ```sh
  <path_to_virtualenv>/bin/pip install 'cryptography>=3.3'
  ```
* Administrators who have installed Synapse from distribution packages should
  consult the information from their distributions.

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

- Add a maximum version for pysaml2 on Python 3.5. ([\#8898](matrix-org/synapse#8898))


Synapse 1.24.0rc2 (2020-12-04)
==============================

Bugfixes
--------

- Fix a regression in v1.24.0rc1 which failed to allow SAML mapping providers which were unable to redirect users to an additional page. ([\#8878](matrix-org/synapse#8878))


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

- Add support for the `prometheus_client` newer than 0.9.0. Contributed by Jordan Bancino. ([\#8875](matrix-org/synapse#8875))


Synapse 1.24.0rc1 (2020-12-02)
==============================

Features
--------

- Add admin API for logging in as a user. ([\#8617](matrix-org/synapse#8617))
- Allow specification of the SAML IdP if the metadata returns multiple IdPs. ([\#8630](matrix-org/synapse#8630))
- Add support for re-trying generation of a localpart for OpenID Connect mapping providers. ([\#8801](matrix-org/synapse#8801), [\#8855](matrix-org/synapse#8855))
- Allow the `Date` header through CORS. Contributed by Nicolas Chamo. ([\#8804](matrix-org/synapse#8804))
- Add a config option, `push.group_by_unread_count`, which controls whether unread message counts in push notifications are defined as "the number of rooms with unread messages" or "total unread messages". ([\#8820](matrix-org/synapse#8820))
- Add `force_purge` option to delete-room admin api. ([\#8843](matrix-org/synapse#8843))


Bugfixes
--------

- Fix a bug where appservices may be sent an excessive amount of read receipts and presence. Broke in v1.22.0. ([\#8744](matrix-org/synapse#8744))
- Fix a bug in some federation APIs which could lead to unexpected behaviour if different parameters were set in the URI and the request body. ([\#8776](matrix-org/synapse#8776))
- Fix a bug where synctl could spawn duplicate copies of a worker. Contributed by Waylon Cude. ([\#8798](matrix-org/synapse#8798))
- Allow per-room profiles to be used for the server notice user. ([\#8799](matrix-org/synapse#8799))
- Fix a bug where logging could break after a call to SIGHUP. ([\#8817](matrix-org/synapse#8817))
- Fix `register_new_matrix_user` failing with "Bad Request" when trailing slash is included in server URL. Contributed by @angdraug. ([\#8823](matrix-org/synapse#8823))
- Fix a minor long-standing bug in login, where we would offer the `password` login type if a custom auth provider supported it, even if password login was disabled. ([\#8835](matrix-org/synapse#8835))
- Fix a long-standing bug which caused Synapse to require unspecified parameters during user-interactive authentication. ([\#8848](matrix-org/synapse#8848))
- Fix a bug introduced in v1.20.0 where the user-agent and IP address reported during user registration for CAS, OpenID Connect, and SAML were of the wrong form. ([\#8784](matrix-org/synapse#8784))


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

- Clarify the usecase for a msisdn delegate. Contributed by Adrian Wannenmacher. ([\#8734](matrix-org/synapse#8734))
- Remove extraneous comma from JSON example in User Admin API docs. ([\#8771](matrix-org/synapse#8771))
- Update `turn-howto.md` with troubleshooting notes. ([\#8779](matrix-org/synapse#8779))
- Fix the example on how to set the `Content-Type` header in nginx for the Client Well-Known URI. ([\#8793](matrix-org/synapse#8793))
- Improve the documentation for the admin API to list all media in a room with respect to encrypted events. ([\#8795](matrix-org/synapse#8795))
- Update the formatting of the `push` section of the homeserver config file to better align with the [code style guidelines](https://github.com/matrix-org/synapse/blob/develop/docs/code_style.md#configuration-file-format). ([\#8818](matrix-org/synapse#8818))
- Improve documentation how to configure prometheus for workers. ([\#8822](matrix-org/synapse#8822))
- Update example prometheus console. ([\#8824](matrix-org/synapse#8824))


Deprecations and Removals
-------------------------

- Remove old `/_matrix/client/*/admin` endpoints which were deprecated since Synapse 1.20.0. ([\#8785](matrix-org/synapse#8785))
- Disable pretty printing JSON responses for curl. Users who want pretty-printed output should use [jq](https://stedolan.github.io/jq/) in combination with curl. Contributed by @tulir. ([\#8833](matrix-org/synapse#8833))


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

- Simplify the way the `HomeServer` object caches its internal attributes. ([\#8565](matrix-org/synapse#8565), [\#8851](matrix-org/synapse#8851))
- Add an example and documentation for clock skew to the SAML2 sample configuration to allow for clock/time difference between the homserver and IdP. Contributed by @localguru. ([\#8731](matrix-org/synapse#8731))
- Generalise `RoomMemberHandler._locally_reject_invite` to apply to more flows than just invite. ([\#8751](matrix-org/synapse#8751))
- Generalise `RoomStore.maybe_store_room_on_invite` to handle other, non-invite membership events. ([\#8754](matrix-org/synapse#8754))
- Refactor test utilities for injecting HTTP requests. ([\#8757](matrix-org/synapse#8757), [\#8758](matrix-org/synapse#8758), [\#8759](matrix-org/synapse#8759), [\#8760](matrix-org/synapse#8760), [\#8761](matrix-org/synapse#8761), [\#8777](matrix-org/synapse#8777))
- Consolidate logic between the OpenID Connect and SAML code. ([\#8765](matrix-org/synapse#8765))
- Use `TYPE_CHECKING` instead of magic `MYPY` variable. ([\#8770](matrix-org/synapse#8770))
- Add a commandline script to sign arbitrary json objects. ([\#8772](matrix-org/synapse#8772))
- Minor log line improvements for the SSO mapping code used to generate Matrix IDs from SSO IDs. ([\#8773](matrix-org/synapse#8773))
- Add additional error checking for OpenID Connect and SAML mapping providers. ([\#8774](matrix-org/synapse#8774), [\#8800](matrix-org/synapse#8800))
- Add type hints to HTTP abstractions. ([\#8806](matrix-org/synapse#8806), [\#8812](matrix-org/synapse#8812))
- Remove unnecessary function arguments and add typing to several membership replication classes. ([\#8809](matrix-org/synapse#8809))
- Optimise the lookup for an invite from another homeserver when trying to reject it. ([\#8815](matrix-org/synapse#8815))
- Add tests for `password_auth_provider`s. ([\#8819](matrix-org/synapse#8819))
- Drop redundant database index on `event_json`. ([\#8845](matrix-org/synapse#8845))
- Simplify `uk.half-shot.msc2778.login.application_service` login handler. ([\#8847](matrix-org/synapse#8847))
- Refactor `password_auth_provider` support code. ([\#8849](matrix-org/synapse#8849))
- Add missing `ordering` to background database updates. ([\#8850](matrix-org/synapse#8850))
- Allow for specifying a room version when creating a room in unit tests via `RestHelper.create_room_as`. ([\#8854](matrix-org/synapse#8854))
@bradtgmurray bradtgmurray mentioned this pull request Apr 26, 2022
4 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants